<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Patch Set 1:</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Patch Set 1:</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Patch Set 1: Code-Review-1</p><p style="white-space: pre-wrap; word-wrap: break-word;">I feel that this patch will introduce a lot of subtle unintended changes and other bugs.</p><ul><li>The channel snapshots before this change were taken AFTER the channel was called.  So they would indicate that the target was called.</li></ul><ul><li>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.</li></ul><ul><li>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.</li></ul></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">OK, good points. Any other direction to fix this?</p><p style="white-space: pre-wrap; word-wrap: break-word;">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...</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think there is a good way to impose the ordering because more than one thread and stasis message queue is involved.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/11058">View Change</a></p><ul style="list-style: none; padding: 0;"></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/11058">change 11058</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/11058"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Id0865fb306136f30a1aa8be82f3753c47059ff2d </div>
<div style="display:none"> Gerrit-Change-Number: 11058 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Nathan Bruning <nathan@iperity.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Nathan Bruning <nathan@iperity.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Sun, 03 Mar 2019 23:39:03 +0000 </div>
<div style="display:none"> Gerrit-HasComments: No </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>