[asterisk-dev] [Code Review] 3900: Bridging: Fix an issue where bridge features can be interrupted by actions that set the UNBRIDGE soft hangup flag on a channel.

[Jonathan Rose]

> On Aug. 8, 2014, 6:19 p.m., rmudgett wrote:
> > /branches/12/include/asterisk/channel.h, lines 1547-1554
> > <https://reviewboard.asterisk.org/r/3900/diff/1/?file=66462#file66462line1547>
> >
> >     This goes against the established defacto pattern.  _locked vs _nolock

It's slightly weird with the ast_softhangup_nolock being the method for setting soft hangup.  Then there's ast_check_hangup_locked which is somewhat more like this in that it functions as an accessor.

Anyway, I've switched them.  I hope I got it right this time.


> On Aug. 8, 2014, 6:19 p.m., rmudgett wrote:
> > /branches/12/main/channel_internal_api.c, lines 1121-1126
> > <https://reviewboard.asterisk.org/r/3900/diff/1/?file=66466#file66466line1121>
> >
> >     These functions names are opposite from defacto convention: _nolock vs _locked

See above. I switched this to nolock and changed the behavior so that ast_channel_unbridged locks the channel and calls the nolock version to get the results, but this seems a little similar to ast_check_hangup to me in purpose (accesses the value) and that used ast_check_hangup_locked, so that's why I went with _locked here instead of _nolock.


- Jonathan


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


On Aug. 8, 2014, 5:42 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3900/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2014, 5:42 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-24027
>     https://issues.asterisk.org/jira/browse/ASTERISK-24027
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> An odd little issue that can interrupt features and even hangup calls entirely in an unwanted fashion.
> 
> Reproduction is fairly trivial, simply have a dynamic bridge feature that plays audio or executes an AGI or something that just generally takes a little while and is sensitive to hangups.
> Call into an extension that puts that feature on the channel and call another device.
> Execute the feature.
> While the feature is running, start mixmonitor on the channel executing the feature and watch as the feature is prematurely terminated and the call itself is hung up entirely.
> 
> Having the flag to re-evaluate the status of the bridge be a hangup flag seems to have been a little off point and trying to have everything that pays attention to hangups specifically ignore it seems a little wacky, so instead I've pulled AST_SOFTHANGUP_UNBRIDGE out of the soft hangup flags and turned it into its own thing.
> 
> 
> Diffs
> -----
> 
>   /branches/12/main/pbx.c 420558 
>   /branches/12/main/framehook.c 420558 
>   /branches/12/main/channel_internal_api.c 420558 
>   /branches/12/main/channel.c 420558 
>   /branches/12/main/bridge_channel.c 420558 
>   /branches/12/main/bridge_after.c 420558 
>   /branches/12/include/asterisk/channel.h 420558 
>   /branches/12/apps/app_stack.c 420558 
>   /branches/12/apps/app_mixmonitor.c 420558 
>   /branches/12/apps/app_chanspy.c 420558 
> 
> Diff: https://reviewboard.asterisk.org/r/3900/diff/
> 
> 
> Testing
> -------
> 
> Performed the above reproduction steps with the patch and the call no longer hangs up and the feature completes normally.  Mixmonitor captures all the audio as well.
> Made sure native RTP bridges would still be re-evaluated and become simple bridges when a hook such as mixmonitor is placed on one of the bridged channels.
> Ran through chan_sip and chan_pjsip testsuite tests to make sure the patch didn't introduce any failures.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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