[asterisk-commits] rmudgett: branch 1.8 r359979 - in /branches/1.8: include/asterisk/ main/

[SVN commits to the Asterisk project]

Author: rmudgett
Date: Tue Mar 20 12:21:16 2012
New Revision: 359979

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=359979
Log:
Allow AMI action callback to be reentrant.

Fix AMI module reload deadlock regression from ASTERISK-18479 when it
tried to fix the race between calling an AMI action callback and
unregistering that action.  Refixes ASTERISK-13784 broken by
ASTERISK-17785 change.

Locking the ao2 object guaranteed that there were no active callbacks that
mattered when ast_manager_unregister() was called.  Unfortunately, this
causes the deadlock situation.  The patch stops locking the ao2 object to
allow multiple threads to invoke the callback re-entrantly.  There is no
way to guarantee a module unload will not crash because of an active
callback.  The code attempts to minimize the chance with the registered
flag and the maximum 5 second delay before ast_manager_unregister()
returns.

The trunk version of the patch changes the API to fix the race condition
correctly to prevent the module code from unloading from memory while an
action callback is active.

* Don't hold the lock while calling the AMI action callback.

(closes issue ASTERISK-19487)
Reported by: Philippe Lindheimer

Review: https://reviewboard.asterisk.org/r/1818/
Review: https://reviewboard.asterisk.org/r/1820/

Modified:
    branches/1.8/include/asterisk/manager.h
    branches/1.8/main/manager.c

Modified: branches/1.8/include/asterisk/manager.h
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/include/asterisk/manager.h?view=diff&rev=359979&r1=359978&r2=359979
==============================================================================
--- branches/1.8/include/asterisk/manager.h (original)
+++ branches/1.8/include/asterisk/manager.h Tue Mar 20 12:21:16 2012
@@ -160,6 +160,8 @@
 	 * function and unregestring the AMI action object.
 	 */
 	unsigned int registered:1;
+	/*! Number of active func() calls in progress. */
+	unsigned int active_count;
 };
 
 /*! \brief External routines may register/unregister manager callbacks this way 

Modified: branches/1.8/main/manager.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/manager.c?view=diff&rev=359979&r1=359978&r2=359979
==============================================================================
--- branches/1.8/main/manager.c (original)
+++ branches/1.8/main/manager.c Tue Mar 20 12:21:16 2012
@@ -1935,7 +1935,11 @@
 
 			ao2_lock(act_found);
 			if (act_found->registered && act_found->func) {
+				++act_found->active_count;
+				ao2_unlock(act_found);
 				ret = act_found->func(&s, &m);
+				ao2_lock(act_found);
+				--act_found->active_count;
 			} else {
 				ret = -1;
 			}
@@ -4635,8 +4639,12 @@
 			ao2_lock(act_found);
 			if (act_found->registered && act_found->func) {
 				ast_debug(1, "Running action '%s'\n", act_found->action);
+				++act_found->active_count;
+				ao2_unlock(act_found);
 				ret = act_found->func(s, m);
 				acted = 1;
+				ao2_lock(act_found);
+				--act_found->active_count;
 			}
 			ao2_unlock(act_found);
 		}
@@ -5144,6 +5152,8 @@
 	AST_RWLIST_UNLOCK(&actions);
 
 	if (cur) {
+		time_t now;
+
 		/*
 		 * We have removed the action object from the container so we
 		 * are no longer in a hurry.
@@ -5151,6 +5161,23 @@
 		ao2_lock(cur);
 		cur->registered = 0;
 		ao2_unlock(cur);
+
+		/*
+		 * Wait up to 5 seconds for any active invocations to complete
+		 * before returning.  We have to wait instead of blocking
+		 * because we may be waiting for ourself to complete.
+		 */
+		now = time(NULL);
+		while (cur->active_count) {
+			if (5 <= time(NULL) - now) {
+				ast_debug(1,
+					"Unregister manager action %s timed out waiting for %d active instances to complete\n",
+					action, cur->active_count);
+				break;
+			}
+
+			sched_yield();
+		}
 
 		ao2_t_ref(cur, -1, "action object removed from list");
 		ast_verb(2, "Manager unregistered action %s\n", action);