[asterisk-dev] [Code Review] Sanely handle AMI connections during startup

[Sean Bright]

> On 2009-06-03 15:36:19, Tilghman Lesher wrote:
> > /branches/1.4/main/loader.c, line 541
> > <http://reviewboard.digium.com/r/272/diff/1/?file=5523#file5523line541>
> >
> >     For the sake of distinguishing where APIs reside, this function should probably be prefixed with astman_, to match other similar public functions from this module.

This isn't a manager function, it is a loader one.  Or maybe I am misunderstanding your suggestion?


> On 2009-06-03 15:36:19, Tilghman Lesher wrote:
> > /branches/1.4/main/loader.c, lines 601-612
> > <http://reviewboard.digium.com/r/272/diff/1/?file=5523#file5523line601>
> >
> >     This isn't the only place a reload request could occur, though.  If you ran Action: command\r\nCommand: module reload chan_sip.so\r\n\r\n, you'd get the same possible deadlock.

Sure it is, this is ast_module_reload() in loader.c.  It is called by 'module reload' regardless of where we are issuing it, be it from the CLI or AMI or wherever...


- Sean


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/272/#review820
-----------------------------------------------------------


On 2009-06-03 15:21:17, Sean Bright wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/272/
> -----------------------------------------------------------
> 
> (Updated 2009-06-03 15:21:17)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> During asterisk startup, a lock on the list of modules is obtained by the
> primary thread while each module is initialized.  Issue 13778 pointed out a
> problem with this approach, however.  Because the AMI is loaded before other
> modules, it is possible for a module reload to be issued by a connected client
> (via Action: Command), causing a deadlock.
> 
> The resolution for 13778 was to move initialization of the manager to happen
> after the other modules had already been lodaded.  While this fixed this
> particular issue, it caused a problem for users (like FreePBX) who call AMI
> scripts via an #exec in a configuration file (See issue 15189).
> 
> The solution I have come up with is to defer any reload requests that come in
> until after the server is fully booted.  When a call comes in to
> ast_module_reload (from wherever) before we are fully booted, the request is
> added to a queue of pending requests.  Once we are done booting up, we then
> execute these deferred requests in turn.
> 
> Note that I have tried to make this a bit more intelligent in that it will not
> queue up more than 1 request for the same module to be reloaded, and if a
> general reload request comes in ('module reload') the queue is flushed and we
> only issue a single deferred reload for the entire system.
> 
> As for how this will impact existing installations - Before 13778, a reload
> issued before module initialization was completed would result in a deadlock.
> After 13778, you simply couldn't connect to the manager during startup (which
> causes problems with #exec-that-calls-AMI configuration files).  I believe this
> is a good general purposes solution that won't negatively impact existing
> installations.
> 
> 
> This addresses bugs 13778 and 15189.
>     https://issues.asterisk.org/view.php?id=13778
>     https://issues.asterisk.org/view.php?id=15189
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/include/asterisk.h 198953 
>   /branches/1.4/main/asterisk.c 198953 
>   /branches/1.4/main/loader.c 198953 
> 
> Diff: http://reviewboard.digium.com/r/272/diff
> 
> 
> Testing
> -------
> 
> Compiles.  Created simple script to call 'module reload' from an #exec and
> another independent script that simply connects on startup and issues a reload.
> 
> Phillippe Lindheimer of the FreePBX project (reporter of issue 15189) has also
> tested and confirms that this resolves his problem.
> 
> 
> Thanks,
> 
> Sean
> 
>