[asterisk-dev] [Code Review] 2965: stasis: Ensure channels are departed from bridges

[rmudgett]

> On Oct. 28, 2013, 6:42 p.m., rmudgett wrote:
> > /branches/12/res/stasis/control.c, lines 492-493
> > <https://reviewboard.asterisk.org/r/2965/diff/1/?file=47645#file47645line492>
> >
> >     I expect you are going to have conflicts with app_control_continue() and app_control_remove_channel_from_bridge() because they depart the channel as well.
> >     
> >     I think using departable channels for stasis is just wrong.  I suggest looking at app_agent_pool.c for how to reclaim control of a channel when it leaves a bridge.
> 
> Matt Jordan wrote:
>     Richard: how would using a non-departable channel work for Stasis? In Stasis, the thread that puts the channel into the bridge cannot block while the channel is in the bridge, as it is most likely (and almost certainly not) the PBX thread that the channel used when it entered into the application.
> 
> rmudgett wrote:
>     Ok.  Forget the last two sentences in my original comment.
>     
>     However, calling app_control_continue() to remove the channel from a bridge is going to have problems with ast_bridge_depart() being called twice.  This is not allowed.  When app_control_continue() calls ast_bridge_depert() which kicks the channel out of a bridge, the after bridge callback calls the failure callback.  With this patch, the normal and failure callbacks do the same thing.  At this time, they call ast_bridge_depart() on a channel already being departed.
> 
> Joshua Colp wrote:
>     It actually won't. Those occur within the command queue thread and the ast_bridge_depart from the callback is added to the end of that queue. Once it executes and sees that the channel isn't in the bridge it basically becomes a noop.
> 
> rmudgett wrote:
>     Oh.  That works for app_control_continue() and app_control_remove_channel_from_bridge() since they take the channel out of a bridge an keep it out.  However, app_control_add_channel_to_bridge() takes the channel out of a bride if it is in one and puts it into another bridge immediately.  When the command queue gets around to processing the depart, it is in the new bridge and gets departed again.
> 
> Joshua Colp wrote:
>     I suppose it would be possible if the pointer was immediately reused (as the bridge_channel is checked in the command queue item).

After looking again, app_control_add_channel_to_bridge() will also work because the bridge_channel pointer is never going to be the same.  The command queue item has a reference to the bridge_channel.

This issue can be dropped.  There doesn't seem to be anything left with this issue. :)


- rmudgett


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


On Oct. 27, 2013, 5:19 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2965/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2013, 5:19 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22703
>     https://issues.asterisk.org/jira/browse/ASTERISK-22703
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Currently within Stasis there is no guarantee that a channel that leaves a bridge will have ast_bridge_depart called on it. The attached change changes this by having a command queued on the channel that explicitly departs it when it leaves a bridge. This works for outside manipulation, bridge deletion, hangup, etc.
> 
> This fixes an issue that the referenced bug exposed. When hanging up while in a bridge no synchronization occurred between the bridge and stasis - causing both to act on a channel at the same time. Depending on how fast the stasis side executed it was possible for the channel itself to be freed and the bridge reference this freed memory.
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/stasis/control.c 401922 
> 
> Diff: https://reviewboard.asterisk.org/r/2965/diff/
> 
> 
> Testing
> -------
> 
> Deleted a bridge with a channel in it, confirmed with the patch that departure occurs and the bridge is destroyed.
> 
> Hungup while in a bridge, confirmed with the patch that departure occurs.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131029/111f98bb/attachment.html>