[asterisk-dev] [Code Review] 3338: ARI: Prevent corner cases where channel in bridge can wait forever for playback to finish

[rmudgett]

> On March 13, 2014, 12:43 p.m., rmudgett wrote:
> > You need to add cases to the switches that handle AST_FRAME_BRIDGE_ACTION in frame.c and bridge_softmix.c.  It is probably best if the AST_FRAME_BRIDGE_ACTION_SYNC case is next to the AST_BRIDGE_ACTION case.
> > 
> > The main difficulty with this patch is that you are adding an allocated resource to ast_frame's which inherently doesn't support this.
> 
> Mark Michelson wrote:
>     I don't understand the comment about adding an allocated resource to ast_frame. The frame payload is allocated on the stack, but that's the same strategy used for all bridge_playfile payloads right now. The bridge_playfile is memcpy'd into the sync_payload, and the sync_payload's data length is adjusted to account for this. When ast_frdup() copies the frame data to another frame, the entire sync_payload and encased bridge_playfile is copied to the other frame, so I'm not sure what the problem here is.

The allocated resource is referring to the fact that you have to explicitly keep track of the frame to manage the semaphore.  If that frame is simply destroyed, you will wait forever; which is a resource leak.


- rmudgett


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


On March 13, 2014, 2 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3338/
> -----------------------------------------------------------
> 
> (Updated March 13, 2014, 2 p.m.)
> 
> 
> Review request for Asterisk Developers and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> ARI tests were occasionally showing a channel being stuck forever in Stasis. After looking at a live system where the problem was occurring, it became clear that the channel was stuck trying to wait for a playback to finish. However, it was also clear that the playback had been abandoned long ago.
> 
> When playing back a file to a channel that is in a bridge, the idea was to queue the playfile action onto the corresponding bridge channel, and then wait until the ARI custom playback function completed to signal that the playback had finished. There were several ways that this could fail:
> * If the bridge channel were not found in the bridge, then we would never attempt to queue the playfile action, but we would still try to wait for it to finish happening.
> * If queuing the playfile action did not succeed, then we would still attempt to wait for the action to occur.
> * If the playback action was successfully queued, but the bridge channel were removed from the bridge before the playfile action could start, then the playback would never happen, and we'd wait forever.
> 
> The responsibility of knowing whether a playfile action has conpleted or ever will occur is the bridge's, since ARI can never fully know. With this in mind, I have created a new bridge channel API call to queue a playfile action synchronously. The way it is done, synchronous queuing operations could be created for other bridge actions quite easily. A new frametype, AST_FRAME_BRIDGE_ACTION_SYNC, contains a payload consisting of a synchronization ID and the actual payload of the bridge action that is to be queued. When the frame is queued, a synchronization structure is created and added to a linked list. The procedure then blocks until it is told that the frame has been disposed of.
> 
> When the frame gets freed (which will occur whether the playfile action succeeds or does not happen), the freer of the frame signals the waiting procedure that the playfile action has terminated, and control returns to whoever queued the playfile action in the first place.
> 
> From ARI's perspective, this greatly simplifies its code. Most of the res_stasis_playback changes are code removal. The bridge_channel changes, though, are pretty large, which is why I've included Richard on this issue.
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/res_stasis_playback.c 410467 
>   /branches/12/main/frame.c 410467 
>   /branches/12/main/channel.c 410467 
>   /branches/12/main/bridge_channel.c 410467 
>   /branches/12/include/asterisk/frame.h 410467 
>   /branches/12/include/asterisk/bridge_channel.h 410467 
>   /branches/12/funcs/func_frame_trace.c 410467 
>   /branches/12/bridges/bridge_softmix.c 410467 
> 
> Diff: https://reviewboard.asterisk.org/r/3338/diff/
> 
> 
> Testing
> -------
> 
> Initially, I did some manual playbacks using Swagger-UI to a channel in a bridge to ensure that the specified file was actually being played as promised. I also queued up several files in quick succession to ensure that they played in series and not on top of each other.
> 
> In addition, I have created an automated test that is up for review at /r/3339
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

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