Summary:	ASTERISK-14041: [patch] CoreShowChannels Response does not honor actionid
Reporter:	Sebastian Gutierrez (sum)	Labels:
Date Opened:	2009-04-29 20:07:12	Date Closed:	2009-05-26 17:46:46
Priority:	Trivial	Regression?	No
Status:	Closed/Complete	Components:	Core/ManagerInterface
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) honoractionidshowchannels.patch ( 1) patchactionid2.patch
Description:	Response: Success Eventlist: start Message: Channels will follow Channel: SIP/phone-02-00872080 UniqueID: 1241052557.223 Context: from-local Extension: 1299 Priority: 3 ChannelState: 6 ChannelStateDesc: Up Application: MusicOnHold ApplicationData: CallerIDnum: 1201 Duration: 00:00:12 AccountCode: BridgedChannel: BridgedUniqueID: Event: CoreShowChannelsComplete EventList: Complete ListItems: 1 ActionID missing.
Comments:	By: Sebastian Gutierrez (sum) 2009-04-29 20:08:11 Uploaded a patch to honor the action id. comments? Patch for trunk 191177 By: Stefan Reuter (srt) 2009-04-29 20:12:08 Looks good. Thanks. By: Sebastian Gutierrez (sum) 2009-04-29 20:17:06 the example shows that is missing the event header but that is solved in trunk the example was with a previuos version, but all of them misses the actionid. By: Sebastian Gutierrez (sum) 2009-05-12 16:15:51 is it possible to have this commited? is a very little fix. Thanks By: Sebastian Gutierrez (sum) 2009-05-13 18:48:13 I realized that on that function the name is different for the action id so the patch was wrong, I now tested the new one and uploaded the new patchactionid2.patch can you check if this is ok? By: Sebastian Gutierrez (sum) 2009-05-21 18:59:11 any comments? is it possible to have this checked in? it's important to any who is using ami to be able to parse the response the same way that is done with other responses. Thnks By: Matt Riddell (zx81) 2009-05-21 21:27:39 I don't see how that could work? You're including actionidtext but have not set it to anything.  What's wrong with the "actionid"? By: Sebastian Gutierrez (sum) 2009-05-21 21:37:32 actionidtext is setted at the begining of the function, actionid was never include on the astman_append. based on trunk check lines: 2984: actionidtext defined   2991 - 2995: actionidtext setted 3019: the appends never include the actionidtext, so this patch adds it. (the lines may not be exact but should be close, i have some modifications) By: Matt Riddell (zx81) 2009-05-21 22:28:40 Ok, cool makes sense - weird that this disappeared. By: Sean Bright (seanbright) 2009-05-26 15:41:26 I'm not entirely convinced this is necessary.  There is an acknowledgment message that is sent before the list of channels, and a CoreShowChannelsComplete event is sent at the end of the list of the channels.  Both of those messages contain the ActionID.  Why is it needed on each 'CoreShowChannel' message as well? By: Stefan Reuter (srt) 2009-05-26 15:49:00 One reason is consistency: All other manager actions that send their response as a series of events include the action id on all events. This is a feature that existing libraries use to match the events with the action they answer. Another one could be that other messages might appear in between the events, even multiple concurrent CoreStatusActions could be sent (not sure if this is relevant or if there is some locking in place). By: Sean Bright (seanbright) 2009-05-26 16:01:35 All other manager actions do not send ActionID on all events (SIPshowregistry for one). While other messages might appear between the events, they wouldn't be CoreShowChannel events, so there wouldn't be confusion.  And yes, you could call CoreShowChannels multiple times, but the results would be returned in serial, so there wouldn't be any stepping on toes. I'm pretty sure this can be closed, but I'll wait a day or two for others to comment. By: Stefan Reuter (srt) 2009-05-26 16:28:50 Then SIPshowregistry is one of those misbehaving actions. There are a couple of actions that contain the ActionID and we would really love to keep them. Examples include: - Agents - Status - QueueStatus - QueueSummary - ShowDialplan - SIPpeers - ... Why would the result be in series if I would call CoreShowChannels multiple times without waiting it to finish sending the response events before sending the next one? By: Sean Bright (seanbright) 2009-05-26 16:39:56 I have no intention of removing the functionality from other events, so no need to be concerned about those. If you sent: <pre> Action: CoreShowChannels ... Action: CoreShowChannels </pre> You would get back: <pre> Response: Channels will follow ... Event: CoreShowChannel Event: CoreShowChannel ... Event: CoreShowChannelsComplete Response: Channels will follow ... Event: CoreShowChannel Event: CoreShowChannel ... Event: CoreShowChannelsComplete </pre> Since your manager connection is single threaded and the responses from each command are buffered before being sent.  Give it a shot, you'll see what I mean. By: Stefan Reuter (srt) 2009-05-26 16:48:28 Ok then the second reason is not valid. I agree on that. :) Let me explain how Asterisk-Java is currently handling actions that send their response as a series of events: For each such action we define a completion event (in this case CoreShowChannelsComplete). After sending the action we collect all events that carry the action id of that action until we receive the completion event. This list of events is what we return to the caller. Quite simple and works fine for the "old" events. If this "rule" no longer exists we must change our implementation. Is this a change that has been discussed or just a feeling of you? By: Stefan Reuter (srt) 2009-05-26 16:52:49 You may also want to have a look at http://www.voip-info.org/wiki/view/Asterisk+manager+API which says "When a client sends an event generating action Asterisk sends a Response packed indicating success and containing a "Response: Follows" line. Then it sends zero or more events that contain the actual payload and finally an action complete event indicating that all data has been sent. The events sent in response to an event generating action and the action complete event contain the ActionID of the Action packet that triggered them, so you can easily match them the same way as Response packets. An example of an event generating action is the Status action that triggers Status events for each active channel. When all Status events have been sent a terminating a StatusComplete event is sent." By: Digium Subversion (svnbot) 2009-05-26 17:38:06 Repository: asterisk Revision: 196945 U   trunk/main/manager.c ------------------------------------------------------------------------ r196945 | seanbright | 2009-05-26 17:38:05 -0500 (Tue, 26 May 2009) | 13 lines Add ActionID to CoreShowChannel event. There is inconsistency in how we handle manager responses that are lists of items and, unfortunately, third parties have come to rely on ActionID being on every event within those lists instead of just keeping track of the ActionID for the current response.  This change makes CoreShowChannels include the ActionID with each CoreShowChannel event generated as a result of it being called. (closes issue ASTERISK-14041) Reported by: sum Patches:      patchactionid2.patch uploaded by sum (license 766) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=196945 By: Digium Subversion (svnbot) 2009-05-26 17:46:45 Repository: asterisk Revision: 196950 _U  branches/1.6.2/ U   branches/1.6.2/main/manager.c ------------------------------------------------------------------------ r196950 | seanbright | 2009-05-26 17:46:45 -0500 (Tue, 26 May 2009) | 20 lines Merged revisions 196945 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........  r196945 | seanbright | 2009-05-26 18:38:05 -0400 (Tue, 26 May 2009) | 13 lines    Add ActionID to CoreShowChannel event.    There is inconsistency in how we handle manager responses that are lists of  items and, unfortunately, third parties have come to rely on ActionID being on  every event within those lists instead of just keeping track of the ActionID for  the current response.  This change makes CoreShowChannels include the ActionID  with each CoreShowChannel event generated as a result of it being called.    (closes issue ASTERISK-14041)  Reported by: sum  Patches:        patchactionid2.patch uploaded by sum (license 766) ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=196950