[asterisk-dev] [Code Review] A chan_local with no busy loop deadlock avoidance (and no deadlocks, too)

[Tilghman Lesher]

> On 2009-04-16 07:53:26, Brad Latus wrote:
> > Just took a look over coding guidelines wise..
> 
> Mark Michelson wrote:
>     Thanks for taking a look at this. I'm not going to be posting any new diffs on this review until the idea of the frame list lock is pursued, though. Until then, I'm not really accomplishing anything here.

What is currently holding up this patch?


- Tilghman


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


On 2009-04-11 18:01:50, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/219/
> -----------------------------------------------------------
> 
> (Updated 2009-04-11 18:01:50)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> chan_local is notorious for its ability to deadlock. Some functions in chan_local consist mainly of deadlock avoidance using busy loop constructs.
> 
> Upon inspecting the code closely, I found that local_queue_frame and local_hangup were the two functions which were the worst as far as this construct is concerned. This is because these two functions were attempting to lock two channels as well as the local_pvt. What I noticed was that both of these functions really didn't need to synchronously take its actions. It would be fine to push the tasks to another thread and have them taken care of there instead. That's where this patch comes in. With it, I have made use of a task processor in order to avoid all the deadlock avoidance code which had plagued chan_local before. The only place where you'll see deadlock avoidance used is in local_call, because we really do need to have both channels locked there.
> 
> I made this patch against the ast_channel_ao2 branch because I found that it was not possible to implement all the fixes I wanted to without the use of ref-counted channels. The problem was that the channel thread could free the channel out from under a task processor execution. With ref-counted channels, you don't have to worry about that at all since we can prevent the channel from being released until all references to it are removed. I found while working on this that I could also get rid of the LOCAL_GLARE_DETECT and LOCAL_CANCEL_QUEUE flags.
> 
> 
> Diffs
> -----
> 
>   /team/russell/ast_channel_ao2/channels/chan_local.c 188030 
> 
> Diff: https://reviewboard.asterisk.org/r/219/diff
> 
> 
> Testing
> -------
> 
> I have attempted placing calls to Local channels with and without the /n modifier. I also have attempted operations which use local channels internally, such as attended transfers and call-forwards. I ran both plain and under valgrind and have no errors.
> 
> In the interest of full disclosure, I discovered a crash at one point stemming from my use of realtime music on hold. I tested with vanilla trunk and found that the same crash occurred there, too, so it has nothing to do with my changes or the ast_channel_ao2 branch's changes. When I re-ran my tests with statically-configured music on hold, all went well.
> 
> 
> Thanks,
> 
> Mark
> 
>