----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3908/#review13141 -----------------------------------------------------------
mostly minor nits /branches/12/include/asterisk/stasis_app_impl.h <https://reviewboard.asterisk.org/r/3908/#comment23596> stasis_app_send_command /branches/12/res/res_stasis.c <https://reviewboard.asterisk.org/r/3908/#comment23597> blank line not needed since the needs_depart is set to test it here. /branches/12/res/res_stasis_playback.c <https://reviewboard.asterisk.org/r/3908/#comment23599> playback is not checked for NULL from create failure. /branches/12/res/res_stasis_playback.c <https://reviewboard.asterisk.org/r/3908/#comment23598> To be safe, swap the ao2_ref and the send_comand statements. If the send_command fails to send, playback is unreffed thus this function may no longer have a valid ref to playback. The playback RAII_VAR here looks to be another silly useage. The game played with the playback ref here is rather fragile. A playback ref should be passed to the play_uri command and cleaned up by the remove_from_playback destructor as well as unlinking playback from the playbacks container. What happens if playback fails to get linked into the playbacks container a few lines earlier? /branches/12/res/res_stasis_recording.c <https://reviewboard.asterisk.org/r/3908/#comment23600> The same fragile game with recording is being played here as with playback pointed out elsewhere though the recording RAII_VAR is more useful here. What happens if recording fails to get linked into the recordings container a few lines above? /branches/12/res/stasis/command.h <https://reviewboard.asterisk.org/r/3908/#comment23602> stray blank line change from removing the wrapper functions. /branches/12/res/stasis/command.c <https://reviewboard.asterisk.org/r/3908/#comment23603> Silly RAII_VAR usage. /branches/12/res/stasis/control.c <https://reviewboard.asterisk.org/r/3908/#comment23606> The line wasn't long enough to require a wrap. /branches/12/res/stasis/control.c <https://reviewboard.asterisk.org/r/3908/#comment23607> The line wasn't long enough to require a wrap. /branches/12/res/stasis/stasis_bridge.c <https://reviewboard.asterisk.org/r/3908/#comment23608> Line should be wrapped before ao2_bump. - rmudgett On Aug. 21, 2014, 11:30 a.m., Jonathan Rose wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3908/ > ----------------------------------------------------------- > > (Updated Aug. 21, 2014, 11:30 a.m.) > > > Review request for Asterisk Developers, kmoore, Matt Jordan, and rmudgett. > > > Bugs: ASTERISK-24147 > https://issues.asterisk.org/jira/browse/ASTERISK-24147 > > > Repository: Asterisk > > > Description > ------- > > Here's a basic rundown of what was happening: > > 1. Stasis application with a channel in it, channel is in a bridge. Playback > actions for the channel are queued, the stasis control is running them. > > 2. The channel hangs up. Because the stasis control detects a hangup and the > depart is expected to be handled when the channel leaves the bridge, the > stasis application execution function exits taking the control along with it. > When the PBX thread exits, the channel is considered fully hung up and leaves > the bridging core. At this point we are running after bridge callbacks with, > one of which holds a pointer to a dead control object that we are trying to > do all queue actions to along with other things. This causes the crash. > > In order to fix this, I made sure that if the control execution loop is > exited without pulling the channel out of the bridge that the channel depart > would occur manually and the control would be marked so that the after bridge > cb wouldn't attempt to exit the bridge in this case. This wasn't actually > causing problems, but Richard and I both have concerns about an after bridge > callback attempting to depart a channel from a bridge... it seems a little > out there. > > > Diffs > ----- > > /branches/12/res/stasis/stasis_bridge.c 421424 > /branches/12/res/stasis/control.c 421424 > /branches/12/res/stasis/command.c 421424 > /branches/12/res/stasis/command.h 421424 > /branches/12/res/res_stasis_recording.c 421424 > /branches/12/res/res_stasis_playback.c 421424 > /branches/12/res/res_stasis_answer.c 421424 > /branches/12/res/res_stasis.c 421424 > /branches/12/include/asterisk/stasis_app_impl.h 421424 > > Diff: https://reviewboard.asterisk.org/r/3908/diff/ > > > Testing > ------- > > Made sure the crash that was happening no longer occurs > > Tested long queues of playback to check what would happen with the additional > playbacks. Each subsequent playback from the one currently running fails (in > an expected manner) and reports its failure over stasis. Nothing too odd > there. > > > Thanks, > > Jonathan Rose > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
