[Asterisk-code-review] ARI: Allow for early bridging on dialed ARI channels. (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: ARI: Allow for early bridging on dialed ARI channels.
......................................................................


Patch Set 1:

(3 comments)

https://gerrit.asterisk.org/#/c/2620/1/main/dial.c
File main/dial.c:

Line 517: 		ast_bridge_unsuspend(bridge, chan);
> {quote}
{quote}
The crux of all of this is based on the hypothetical of the channel leaving the bridging system during the call to ast_bridge_suspend().

The question I have is, how would that actually happen in this scenario?
{quote}

The ARI app cannot move the channel while it is dialing?

The ARI app could create the channel, dial, and throw it in a bridge.  Nothing then prevents it from pulling it out of the bridge before dialing is complete.

The ARI app could create the channel, throw it in a bridge, then dial.  Still nothing prevents it from pulling it out of the bridge before dialing is complete.

If the ARI app is quick to pull the channel out of the bridge while you are in the middle of suspending the channel in the bridge, then you could hit the problem.

{quote}
Is the idea that this would allow for me to suspend the channel in the bridge from within the bridge channel's thread? Or did you have something else in mind with this suggestion?
{quote}

Yes.  The idea was to have the bridge channel thread run the dial.  This wouldn't be that easy in the current departable channel model stasis uses.  The threading model changes depending upon if the channel is in a bridge or not.  You may have to put some restrictions on the new ARI dialing method.  Such as restricting the dialing to be allowed only after the channel is put into the target bridge.  You may have to also restrict the actively dialing channel to not being moved if moving a channel to another bridge is accomplished by pulling it out of the bridging system to put it back into another bridge.

Speaking of ARI dialing, I don't think you can have the dial API call multiple locations like the dialplan Dial application.  As far as the ARI dialing in Asterisk is concerned, all dials would be to a single location.  It is the ARI application that would know about other dial instances.

{quote}
That's fine, but I'd prefer to keep that discussion on this review if at all possible.
{quote}

Well we could put the results of the discussion on the review.  The discussion would likely be faster that way and result in fewer walls of text.


Line 535: 			unsuspend_bridge(channel, original);
> Can you explain why? The way things work right now, when the dial state cha
Does channel->bridge_suspended get set regardless if the ARI channel is in a bridge or not?  The code here hangs up the channel if channel->bridge_suspended is not set.

Has ARI been required to hangup dead channels before?  Do dead channels automatically go away when the user hangs up?  

You have given the ARI application a "final" state event.  What could it do with the channel other than hanging it up?  The only thing I can think of is querying some channel value that is not part of the event.


PS1, Line 994: 			int hangup_channel;
             : 
             : 			if (channel->bridge_suspended) {
             : 				unsuspend_bridge(channel, channel->owner ?: who);
             : 				ast_channel_cleanup(channel->owner);
             : 				hangup_channel = 0;
             : 			} else{
             : 				hangup_channel = 1;
             : 			}
> What is "this"? Unsuspending the channel in the bridge?
Yes. Unsuspending the bridge channel.  I don't think the call to ast_poll_channel_del() below is appropriate if you don't control the channel.

Also the loop here is hanging up the losers.  The winner is handled after the loop.  The winner should be unsuspended there.


-- 
To view, visit https://gerrit.asterisk.org/2620
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3b05862644ca5080c7743a1b9b22465486e56ba
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes