<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1818/">https://reviewboard.asterisk.org/r/1818/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 16th, 2012, 4:47 p.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't understand the point of counting the number of active function calls. Isn't this already taken care of inherently by the refcounting done on the actions? I fully understand not holding the action lock while the function call takes place, though.</pre>
</blockquote>
<p>On March 16th, 2012, 5:53 p.m., <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The reference count is not quite the same thing. The reference count is keeping track of how many references to the ao2 object. The active_count is keeping track of how many of those references are actually calling the callback function. The ast_manager_unregister() is usually called during a module unload so it is not a good thing if any of those references are still in the callback function when ast_manager_unregister() returns.
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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">So the concern is not refcounts for the action. Rather, if an ast_manager_unregister() returns when there is still an active action, this may result in a module unloading while code within that module is still executing.
The thing that bothers me about this patch is the arbitrary 5-second busy loop. If an active action takes longer than 5 seconds to complete, then a crash may still occur, and it will be even more difficult to pinpoint the cause than it may have prior to this change.
The "Real Fix(tm)" would be to change every manager action in every module to increment the module refcount when starting the action and then to decrement it when the action completes. That way if a manager action were still active in that module, then the module would wait to unload until all current actions were complete. Of course, such a change would be horrifically large and potentially error-prone, so I can understand why you would go for a simpler one that can be contained just within manager.c I'm having a great deal of difficulty thinking of a way to do this, though. Even if module refcounting were handled within the manager.c code, you are at a disadvantage that registering a manager action does not pass along which module is registering the action, plus not all manager actions are registered from loadable modules.
You also can't reasonably keep track of what thread is executing a manager action because an action can be executed from multiple threads at once...
I guess I'll have to give a grumbly bad-natured ship it to this because it's the least invasive, most likely to succeed, patch. I'd suggest we make some sort of issue in JIRA to get this fixed a proper way though, with less busy loops and non-determinism.</pre>
<br />
<p>- Mark</p>
<br />
<p>On March 15th, 2012, 6:13 p.m., rmudgett wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated March 15, 2012, 6:13 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Steps to invoke the deadlock:
1) Invoke an AMI Command action such as "dialplan reload" or "originate".
2) The command then causes an external script to request another AMI Command action.
3) Deadlock because the current code only allows each registered AMI action to have one active instance at a time.
This patch avoids the deadlock and tries to keep a module unload from potentially causing a crash by an active AMI action using that module.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Code inspection says it should work and the compiler agrees. :)</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19487">ASTERISK-19487</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/include/asterisk/manager.h <span style="color: grey">(359708)</span></li>
<li>/branches/1.8/main/manager.c <span style="color: grey">(359708)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1818/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>