[asterisk-dev] [Code Review] BLF Subscriptions Causes SIP Deadlock

[David Vossel]

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


This is really close to being ready to go.  Just a couple of things to comment about.


trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1563/#comment8874>

    The ref counts here should be removed. If we don't already own a ref before entering this function, grabbing one now doesn't help anything.
    
    I've looked at this callback in quite a bit of detail. Because of the way the event is removed during the "clear_peer_mailboxes(peer)" function in the peer's destructor, we are guaranteed this peer will always be valid regardless of any ref counting.  The event can not be removed in clear_peer_mailboxes until the event lock is grabbed, if the mwi_event_cb is taking place at that exact moment then the event lock is held in that thread.  This syncs the threads up in a way that prevents the peer pointer from existing in the mwi_event_cb after destruction.
    
    This is why the clear_peer_mailboxes(peer) call was moved to the top of the destructor.  We don't want to start tearing down peer information if the peer is somehow in the mwi_event_cb function before the event is removed.  Note that richard's patch moves the clear_peer_mailboxes(peer) call up as well. So depending on who commits first, this may already be done.
    
    Hope this makes sense.



trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1563/#comment8875>

    I believe the peer already has a ref in this function, so the ref bumping should not be necessary.
    
    Unlocking the pvt however is a good idea, nice catch.



trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1563/#comment8876>

    In case it wasn't obvious why the peer should be unlocked here, I'll explain.
    
    Getting the voicemail count, depending on what voicemail file system is in use, could require disk access... Blocking during disk access is stupid expensive.


- David


On Nov. 4, 2011, 11:16 a.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1563/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2011, 11:16 a.m.)
> 
> 
> Review request for Asterisk Developers and David Vossel.
> 
> 
> Summary
> -------
> 
> Fix a locking inversion that causes deadlocks during BLF subscriptions.
> 
> 
> This addresses bug ASTERISK-18663.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18663
> 
> 
> Diffs
> -----
> 
>   trunk/channels/chan_sip.c 342600 
> 
> Diff: https://reviewboard.asterisk.org/r/1563/diff
> 
> 
> Testing
> -------
> 
> Compile
> 
> 
> Thanks,
> 
> opticron
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111104/a7e20df0/attachment.htm>