[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.

[rmudgett]

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



/branches/12/apps/app_stack.c
<https://reviewboard.asterisk.org/r/3900/#comment23489>

    Should just remove the UNBRIDGE references as this code is only for softhangup flags.



/branches/12/apps/app_stack.c
<https://reviewboard.asterisk.org/r/3900/#comment23490>

    Restore this code as ASYNCGOTO is still a thing.



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3900/#comment23491>

    Leave a hole in the bit definitions for ABI compatibility.



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3900/#comment23492>

    No need to state why the function was created.  Only what it does.



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3900/#comment23493>

    This goes against the established defacto pattern.  _locked vs _nolock



/branches/12/main/channel.c
<https://reviewboard.asterisk.org/r/3900/#comment23496>

    This function may only care about ASYNCGOTO now.  Unbridge may no longer have anything to do with this function.



/branches/12/main/channel.c
<https://reviewboard.asterisk.org/r/3900/#comment23494>

    There is only one non-hangup flag left so the extra parens aren't necessary.



/branches/12/main/channel.c
<https://reviewboard.asterisk.org/r/3900/#comment23495>

    Unbridge no longer has a part in this.



/branches/12/main/channel_internal_api.c
<https://reviewboard.asterisk.org/r/3900/#comment23497>

    These functions names are opposite from defacto convention: _nolock vs _locked



/branches/12/main/pbx.c
<https://reviewboard.asterisk.org/r/3900/#comment23498>

    This can probably just be removed.  It is no longer a softhangup flag.  If it does leak out of the bridge framework, it won't cause any harm.  The next time the channel goes into the bridging framework, it would just get reevaluated unnecessarily.
    
    You could instead always clear the unbridge flag when a channel joins the bridging system.


- rmudgett


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/20140808/05336c7e/attachment-0001.html>