[asterisk-dev] tilghman: branch 1.4 r187428 - in /branches/1.4: include/asterisk/ main/

[Russell Bryant]

On Apr 9, 2009, at 1:52 PM, Tilghman Lesher wrote:

>> I'm actually more in favor of the originally proposed patch by  
>> jamesson
>> on issue 14705.
>>
>> As it stands, we now have two things in the code dealing with this
>> potential deadlock.  We have the CLI command blacklist, which  
>> jamesson
>> simply proposed adding "module reload" to, and now we have this timed
>> lock stuff.
>>
>> This timed lock handling will result in less deterministic  
>> behavior.  In
>> some cases, module CLI commands will work from the AMI, and in other
>> cases they won't.  I would much rather just make it clear that it  
>> is not
>> supported at all by sticking with the blacklist approach.
>
> The only module command that will occasionally not work is the  
> 'module unload'
> command.  The 'module reload' command will always work (with this  
> patch), and
> if you ask users, I suspect that a great many of them will resent  
> not being
> able to perform reloads via AMI, whereas the likelihood of them  
> resenting not
> being able to unload a module is much lower.  That's why I pursued  
> this
> approach.


Thanks for the feedback.  I'm okay with this approach after looking  
closer at things.

After I looked closer, I now realize that this doesn't change the  
behavior of module load/unload from AMI.  Those are still  
blacklisted.  Also, I now see that with what you've done, "module  
reload" from the AMI will always work, where it might not before.

The only change in behavior is that module loading and unloading from  
the CLI may fail is if the manager action list is blocked for longer  
than 5 seconds (which I can not imagine happening except in the edge  
case that you are addressing.)

I have one final note.  I think all of this could be simplified and  
the blacklisting could be removed completely if we implemented module  
ref counting on registered manager actions.  That way, when we're  
executing a manager action, we wouldn't need the manager action list  
locked at all.  The need for it would disappear.  I think that this is  
something that we should do at some point in trunk.  It will require  
API changes, so I don't think it's worth it for existing releases.

--
Russell Bryant
Digium, Inc. | Senior Software Engineer, Open Source Team Lead
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org