[asterisk-dev] [Code Review] 3431: Fix channel staging assertion failure.

[rmudgett]

> On April 9, 2014, 5:01 p.m., Matt Jordan wrote:
> > /branches/12/channels/chan_sip.c, lines 7271-7299
> > <https://reviewboard.asterisk.org/r/3431/diff/1/?file=57177#file57177line7271>
> >
> >     While this no longer worked - since we don't call sip_hangup until channels are well out of a bridge - I think it's worth pointing out what this was trying to do and why it's really not needed (even if it did work).
> >     
> >     The idea here was probably to set the statistics on the bridged channel such that whatever executed in the 'h' extension got the statistics from this RTP instance. Unfortunately, I'm not sure how that would have worked well in this case - ast_rtp_instance_set_stats_vars writes the channel variables for the RTCP statistics. If anything, this would have just overwritten the channel variables from one channel with another (most likely writing the bridged channel's statistics onto the original channel's). If both channels that were in the bridge still 'knew' of their bridged channel, they would have just swapped statistics.
> >     
> >     Since we have hangup handlers, the need for this logic is non-existent in the first place.
> 
> Matt Jordan wrote:
>     Actually, I'm totally wrong here. I had to go back and look at the stats function. It actually *does* write the variables onto both channels... oh well. Things you realize after you hit the publish button.

Yes, the stats function does set stats on the channel and its bridged peer.  In sip_hangup() there cannot be a bridge peer anymore.  However, the stats were already set anyway if the channel was in a bridge because the BYE processing sets the stats on the channel and its peer.


- rmudgett


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


On April 9, 2014, 2:19 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3431/
> -----------------------------------------------------------
> 
> (Updated April 9, 2014, 2:19 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The failing assertion ensures that the final snapshot gets generated so CDR records can get finalized.  The only place where a channel staging snapshot flag could be left set is in the handle_request_bye().  The function could return before clearing the flag because the channel could dissappear while the function had to have the channel unlocked.
> 
> * Fixed handle_request_bye() channel snapshot staging coverage area to not have a return in the middle of it and be unable to clear the staging flag.
> 
> * Pushed the channel snapshot staging coverage area into ast_rtp_instance_set_stats_vars() to ensure that the staging is not interrutped.
> 
> * Made callers of ast_rtp_instance_set_stats_vars() not call it with channels or channel driver private locks held to eliminate the deadlock potential.  The callers must hold references to the passed in channel and rtp objects.
> 
> * Eliminated sip_hangup() trying to get the bridge peer.  It is futile at this point because the channel could never be in a bridge.
> 
> * Moved sip_pvt unref in ast_hangup() and handle_request_do() to the end of the function.  The unref needs to happen after the last use of the pointer.
> 
> 
> Diffs
> -----
> 
>   /branches/12/main/rtp_engine.c 412047 
>   /branches/12/channels/chan_sip.c 412047 
> 
> Diff: https://reviewboard.asterisk.org/r/3431/diff/
> 
> 
> Testing
> -------
> 
> I was unsuccessful in reproducing the testsuite channel staging assertion failure.
> However, SIP calls can still setup and teardown with the patch installed.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140409/0648d486/attachment-0001.html>