[asterisk-dev] [Code Review] Avoid deadlocks in chan_sip - 1.6.0

[Russell Bryant]

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


I would be very interested in seeing the "core show locks" output for the deadlocks that you encountered.  Then, I would be more than happy to help with determining the safest changes to make.

There is a "Locking in Asterisk" section of the CODING-GUIDELINES document that may help, as well.


/team/oej/moretrylock-1.6.0/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/433/#comment2918>

    I'm not okay with this change.  If this code is failing to acquire the channel lock, it is indicative of a problem elsewhere.



/team/oej/moretrylock-1.6.0/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/433/#comment2919>

    This change is not safe.  You can not unlock the channel like this inside of a channel tech callback.



/team/oej/moretrylock-1.6.0/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/433/#comment2920>

    This change is not safe either.  You can't unlock a channel like this inside of a channel tech callback.


- Russell


On 2009-11-19 11:02:38, Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/433/
> -----------------------------------------------------------
> 
> (Updated 2009-11-19 11:02:38)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> While running 1.6.0 in production, we have stumbled over a few deadlocks that we have proven to avoid with these changes.
> 
> - CLI "sip show channels" can cause serious damage before this change
> - Retrans_pkt had some issues with locking the Ast_channel
> - Both sip_write and sip_read was hanging waiting for a better time.
> 
> We debugged with GDB to see where each thread was hanging and found all these issues. Tested with and without patches and the issues disappeared on the patched systems.
> 
> Locking is a part of Asterisk coding that I'm far away from being a master in - so please give feedback. I'm sure there's a lot of room for improvement.
> 
> (And yes, I noticed the red box in the patch. Will remove.)
> 
> 
> Diffs
> -----
> 
>   /team/oej/moretrylock-1.6.0/channels/chan_sip.c 230467 
> 
> Diff: https://reviewboard.asterisk.org/r/433/diff
> 
> 
> Testing
> -------
> 
> The patches has been running in production on a couple of PBXs for a couple of weeks without causing any harm that we're aware of.
> 
> 
> Thanks,
> 
> Olle E
> 
>