[Asterisk-code-review] app_queue: fixed ordering of events for queue agent dial (...asterisk[16])

[Nathan Bruning]

Nathan Bruning has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11058 )

Change subject: app_queue: fixed ordering of events for queue agent dial
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> > Patch Set 1:
> > 
> > > Patch Set 1: Code-Review-1
> > > 
> > > I feel that this patch will introduce a lot of subtle unintended changes and other bugs.
> > > 
> > > * The channel snapshots before this change were taken AFTER the channel was called.  So they would indicate that the target was called.
> > > 
> > > * If the ast_call() fails now you have to report DialEnd when you didn't before because it is defined that you get a DialEnd paired with a DialBegin.
> > > 
> > > * The ordering of the events for any other calling method does not enforce this ordering change as you are only doing it for app_queue.  Thus the other methods will still send the UserEvent you mention in the ASTERISK issue before the DialBegin events.
> > 
> > OK, good points. Any other direction to fix this?
> > 
> > Otherwise, it would be a good idea to clarify this in the documentation, so that AMI consumers know they have to account for various orderings. As "unusual" event ordering happens only very sparingly, we've only come across this when our app was already in production...
> 
> Another thing is that these events that you are trying to impose an ordering on are for different channels.  The DialBegin and AgentCalled events in your ASTERISK issue are for the outgoing queue channel (i.e., the ;1 channel of a local channel pair) and the UserEvent event is on the ;2 channel of the local channel pair.  You are trying to order events that do not have a defined ordering.  Because these events are for different channels, it is conceivable that the events can still be out of your desired order even with your current patch.  All your patch is doing is shrinking the window of opportunity and only in the case of app_queue.
> 
> I don't think there is a good way to impose the ordering because more than one thread and stasis message queue is involved.

Actually, the AgentCalled is sent out for the "main" channel, with the ;1 local channel as the DestChannel. This establishes the causal relationship between the queue caller channel and the local channel. From the point-of-view of an event consumer, it would make sense to first know about the origins of the local channel (a queue dial attempt) before receiving dialplan events on it.

I agree my patch does not solve all cases, and doing so would be quite involved, so I would suggest adding a warning to the AMI/Stasis documentation and closing this PR.


-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11058
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Id0865fb306136f30a1aa8be82f3753c47059ff2d
Gerrit-Change-Number: 11058
Gerrit-PatchSet: 1
Gerrit-Owner: Nathan Bruning <nathan at iperity.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Nathan Bruning <nathan at iperity.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Sun, 03 Mar 2019 23:39:03 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190303/fc52cc42/attachment.html>