[asterisk-dev] [Code Review] Asterisk 1.8 Deadlock in app_queue

[irroot]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1402/#review4278
-----------------------------------------------------------


Here is a self review explaining why its been altered.


/branches/1.8/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1402/#comment8365>

    ao2_t_find / ao2_t_link
    
    is used to return the q no need to lock here.



/branches/1.8/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1402/#comment8366>

    ao2_iterator_next handles the queues lock



/branches/1.8/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1402/#comment8367>

    ao2_t_find is used to get the queue so no need to lock the container here



/branches/1.8/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1402/#comment8368>

    ao2_t_iterator_next handles locking 



/branches/1.8/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1402/#comment8369>

    This is only needed for compare_weight latter on see first review above and it will lock a channel while held this long leading to a deadlock path on masquerade this is the route of the locks reported in this bug.



/branches/1.8/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1402/#comment8370>

    we do not need to lock the queues container when working on the members of a queue



/branches/1.8/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1402/#comment8371>

    working with members does not need queues container held



/branches/1.8/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1402/#comment8372>

    here we read members from the DB there is no need to lock the container here as its handeled by load_realtime_queue 



/branches/1.8/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1402/#comment8373>

    ao2_callback will take care of this.



/branches/1.8/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1402/#comment8374>

    ao2_iterator does this not needed here


- irroot


On Sept. 10, 2011, 2:43 p.m., irroot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1402/
> -----------------------------------------------------------
> 
> (Updated Sept. 10, 2011, 2:43 p.m.)
> 
> 
> Review request for Asterisk Developers and Matthew Nicholson.
> 
> 
> Summary
> -------
> 
> Locking order with queues container.
> 
> 1)the queues container is locked when calling channels this causes a deadlock and is unneeded.
> 2)lock the queues container when running queue_update to maintain correct locking order.
> 
> 
> This addresses bug ASTERISK-18101.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18101
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/apps/app_queue.c 335124 
> 
> Diff: https://reviewboard.asterisk.org/r/1402/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> irroot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110910/79fcfc7d/attachment-0001.htm>