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

[Mark Michelson]

> On 2009-04-16 07:53:26, Brad Latus wrote:
> > Just took a look over coding guidelines wise..

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.


> On 2009-04-16 07:53:26, Brad Latus wrote:
> > /team/russell/ast_channel_ao2/channels/chan_local.c, line 666
> > <http://reviewboard.digium.com/r/219/diff/2/?file=4143#file4143line666>
> >
> >     I would think for readability that this 'struct' declaration be moved to the top?

That's arguable. This struct is only used for the task processor hangup callbacks, so I declared it near those callbacks so that the structure could be easily referenced. This is consistent with other structs that are used only as the argument to a callback. For some examples, see the find_call_cb_arg struct in chan_sip.c and the thr_arg struct in utils.c.


> On 2009-04-16 07:53:26, Brad Latus wrote:
> > /team/russell/ast_channel_ao2/channels/chan_local.c, line 677
> > <http://reviewboard.digium.com/r/219/diff/2/?file=4143#file4143line677>
> >
> >     Isn't this wasteful in terms of 'alignment' maybe move to bottom of declaration? (i may be wrong)

Yes it is. I just knew I needed a bit and didn't really have much thoughts with regards to alignment.


> On 2009-04-16 07:53:26, Brad Latus wrote:
> > /team/russell/ast_channel_ao2/channels/chan_local.c, line 772
> > <http://reviewboard.digium.com/r/219/diff/2/?file=4143#file4143line772>
> >
> >     whitespace..

Noted. Once again, I'll fix it once it is worth it to work on this again.


> On 2009-04-16 07:53:26, Brad Latus wrote:
> > /team/russell/ast_channel_ao2/channels/chan_local.c, line 942
> > <http://reviewboard.digium.com/r/219/diff/2/?file=4143#file4143line942>
> >
> >     This now has no braces.. coding guidelines state even for single line if's should be 'brace-ified' ?

Yeah, the left side of the diff is correct here. The reason for this change is that before I realized I needed refcounted channels, I was making a modification against 1.4. This was something that I messed up when applying my original patch to chan_local.c in the ast_channel_ao2 branch.


> On 2009-04-16 07:53:26, Brad Latus wrote:
> > /team/russell/ast_channel_ao2/channels/chan_local.c, line 285
> > <http://reviewboard.digium.com/r/219/diff/2/?file=4143#file4143line285>
> >
> >     whitespace creep

Noted. I'll fix this once this becomes worth working on again.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/219/#review700
-----------------------------------------------------------


On 2009-04-11 18:01:50, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/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: http://reviewboard.digium.com/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
> 
>