Summary:	ASTERISK-00117: Manager: Implement Unique ID for Actions
Reporter:	jayson (jayson)	Labels:
Date Opened:	2003-08-18 00:48:49	Date Closed:	2004-09-25 02:40:13
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) actionid.patch
Description:	Allow user to specify ActionID for Actions. ****** ADDITIONAL INFORMATION ****** I think this could be a big win on the user side. It is incredibly useful to receive UniqueIDs on channels (thus being able to track their state).  It may also be useful to be able to track your own Actions similarly. I would propose a generic interface whereby one could specify an ActionID header that allowed the user side to specify an ID so it can track a response to its Action. Should * become threaded enough to intermix messages such as Newchannels with a Status response (a very real possibility with large numbers of action channels) this would be invaluable for sorting the resulting mess.  It would also prevent us from placing onerous requirements such as atomic responses (can't interrupt Status or any other response). This could later become useful if we get Actions that could take minutes to execute.  When a response comes in minutes from the Action, there is currently no good way to identify its response unless it immediately comes back via the interface. Requiring immediate responses hurts the "event-based" nature of the current protocol.  ActionIDs allow a trivial/optional extension to the Action syntax that is backwards compatible, simple to implement, and solves the above problems before we encounter them.
Comments:	By: Mark Spencer (markster) 2003-08-18 01:00:49 Actions are atomic because things which may take time are handled and receive an asynchronous message.  The only case this could help would presumably in highly latent connections.  The structure doesn't really support easily adding something like an ActionID (not that I can see anyhow). By: jayson (jayson) 2003-08-18 01:58:55 What about long-running actions?  Right now we kind of punt with Status, but there are other possibilities.  Won't forcing actions to be atomic prevent us from being able to have anything like that in the future? See bug ASTERISK-119 for an idea of what I mean. Right now we're a bit fragmentary in the protocol.  I think there are limitations (atomic actions) that are present in the protocol. In other words, I recommend not ACTUALLY making the actions occur asynchronously, only modifying the protocol so that doing so in the future doesn't require protocol changes.  Implementing stuff that relies on the existing behavior makes me nervous. I'll try to catch you on IRC and give some example of what I mean.  I'm not proposing drastically changing how the events are handled, only making the protocol more resilient in the face of a changing backend (and making my software depend less on assumptions/behavior and more on a clearly defined protocol). In the Action: Status example, it pretty much consists of making the attached patch (minor changes to send_ack, send_response, send_error; include file fix; touch up of calls for new parameter; less than 10 lines of changes to action_status).  If you'd like I can implement it on the other actions. Note that this is exactly what I'm recommending it for--being able to associate asynchronous events to a command (like associating Events to a channel using UniqueIDs).  Note also that this is done with minor changes in a sensibly backwards compatible way. To put it another way.  We've got a wonderful protocol except for a few exceptional cases--Status and Command, for instance.  If we can generalize the solutions to the special problems they create, we'll save ourselves trouble later. edited on: 08-18-03 02:37 By: Mark Spencer (markster) 2003-08-18 09:06:08 If we were to do this, it needs to be done in a consistant way, so that every time you send a message with an ActionID, that ActionID would be populated to every response that came back.  Further, omitting the ActionID from your request should cause the ActionID header to be completely omitted from the response.  I'd love some sort of clever symantics that makes it easy for the author of manager events to be sure that.  I'm still not sure it allows us to do anything that the protocol doesn't already support, but I can see some places where it could make things cleaner *if* you can come up with a good implementation. By: jayson (jayson) 2003-08-18 10:20:42 Perhaps I should file another bug on this, but the above part involving ActionID headers being omitted (which I fully support) has a bit of a setback. Specifically, the ast_get_header function returns a blank string for a header not being present.  This prevents me from accurately telling whether the header was missing or just blank (a possibility). Would you mind me changing that to return NULL instead of ""?  I can update the rest of the source. Also, is returning a pointer to that string literal safe?  Isn't that possibly allocated on the stack (and thus freed when the function returns?)? See manager.c, roughly line number 145. edited on: 08-18-03 10:04 By: Mark Spencer (markster) 2003-08-18 19:31:59 No, I prefer having it return an empty string.  An empty ActionID is equivalent to no action ID. By: Mark Spencer (markster) 2003-08-18 19:32:33 Returning a pointer to a static string is fine because they are all allocated at the beginning of program execution on the data area. By: jayson (jayson) 2003-08-18 19:42:18 You indicated above that having no ActionID should suppress an ActionID in the response.  There is currently no way to tell (via ast_get_header) whether the header is no there or just empty. Do you want me to: a)  Treat a blank ActionID like it wasn't specified (IOW, recieve a blank ActionID and leave it out the response!) b)  Handle the headers directly (multiple functions doing the same thing === bad) c)  Adjust return semantics Or anything else that works.  It's just that if I leave it the way it is, we'll have a situation like this: Event: Status ActionID: Response: Success Message: Status Will Follow NOTE:  ActionID specified (although probably not useful) but not emitted.  Bad behavior stemming from no distinction from missing header and blank header. If you say the above situation is okay (it is admittedly nonsensical, GIGO) I'll take care of it. By: Mark Spencer (markster) 2003-08-18 19:46:58 "Specifying an empty ActionID is equivalent to not specifying an ActionID at all, and none shall be presented in the response.  If you want an ActionID header, you must put a non-empty value in the request" In otherwords, "a" from your list. There is no logical reason for adding an ActionID header without a value in it, ergo I have no interest in changing things to support it. By: jayson (jayson) 2003-08-18 19:50:05 <puts on pedantic hat> So, in the general case, we'll never need to distinguish between a blank and missing header? <removes pedantic hat> ;P However, you're the boss.  Implementing as you request. By: Mark Spencer (markster) 2003-08-18 19:55:13 Precisely. By: Mark Spencer (markster) 2003-08-18 19:59:17 Let me explicitly point out that this simplifies implementation of the protocol greatly by: 1) You never have to check the return value to see if it is NULL 2) People creating new functions will never rely on such silly behavior. It's clearly a win-win.  Remember, these are *machines* talking to *machines*.  When you start complicating the syntax, you end up with big piles of syntactic garbage ala SIP.  Simplicity is paramount.  For exmaple, note that the format for a header is ALWAYS: variable: value with precisely one colon and one space (0x20) between the variable and value, thus meaning you don't have to parse it.  There is no need for the programmer to make even minimally complex code to parse it.  Why should it be exactly one space?  Because there's no reason the machine would ever need it to be a different value.  No headers are allowed to span multiple lines, etc etc. Think pragmatically. By: jayson (jayson) 2003-08-18 20:44:42 I think this is rather pragmatic.  When I look at such an implementation, I only see what we can't do (what our protocol can't represent). As for 1, the code appears to actually check for that in a number of places (action_originate: is this a bug?, action_mailboxstatus/mailboxcount, action_extensionstate)!  Secondly, I'm hardpressed to find a point we don't check the return value with strlen to see if it is missing! As for 2, it is easily concievable to want to be able to specify a value as a blank string.  We currently have no way to do this. From the implmentation side: header = astman_get_header(m,"ActionID") if (header)   ...blah... seems cleaner than: header = astman_get_header(m,"ActionID") if (!strlen(header))  ...blah... The loss here is that you can't blindly ignore the possibility of a missing input value and just run with the empty string (couldn't find anywhere that did this).  Looking at the code, not checking for missing inputs is the exception not the rule--and I don't know that encouraging sloppy input handling (even for ourselves) is a good idea anyways. As an example of a time when number 2 would be useful: Action: SetGlobalVar Name: Something Value: Basically, we have to hack in a special case (values like None or <none> or a Clear action) to handle setting some value to an empty string (if we are ever to do proper input checking).  Requiring a separate action to clear a setting doesn't seem very good.  I'm not exciting about the option of just not passing in a value and relying on the implementation's mapping of missing to "" either. Pragmatically, what I see is: Benefits: * Checkup Code Doesn't Waste Time on strlen * Explicit Ability to Handle Missing Headers * Able to Specify Blank Values (which do matter) * ActionID presence *ALWAYS* matches for both input and output Disadvantages: * Possible NULL-Point Deref if Return Is Used Sloppily Changes: -    return ""; +    return NULL; 100 x - if (strlen(header))      + if (header) versus: Benefits: * Ability To Ignore Presence of Optional Headers (is this good?) Disadvantages: * No Way To Handle Missing Header Versus Blank Value (creates protocol limitation) * No Error Thrown On Bad Input (Missing Value) Changes:  None In short, we don't gain simplicity of parsing from this decision--just simpler implementation (change would amount to at most three lines per action of handling that is already there in the general case). I agree wholeheartedly on the header syntax being 'Header:\x20Value\r\n'. I understand that in "machines talking to machines" simplicity means less badness, but I think we are sacrificing simplicity in the protocol for simplicity in the implementation that doesn't gain us anything significant. I may be missing something.  Is there any advantage to the current scheme other than the capability to just ignore the non-existant headers without a second thought? Keep in mind, we gain flexibility in the protocol (blank values), we gain power in the implementation (can detect blank headers), we gain speed at runtime (checking a NULL pointer is faster than strlen), we gain resilience at runtime (strlen and company can make badness worse on a bogus pointer), and we lose what?  After searching manager.c (and a few others) for astman_get_header, I can't find any code that actually relies on that behavior to do anything useful (but I do find tons of strlens and even some cases where you check the point to see if it is NULL!). edited on: 08-18-03 20:28 By: jayson (jayson) 2003-08-18 21:46:48 Look at the Message: Action: Login Username: Foo Here it is not obvious that there is a header Secret.  Someone looking at the protocol would have to dig into the source to find out that checking for the Secret header magically creates an empty on in the implementation. The IETF has dealt with this too.  HTTP also utilizes this behavior.  Specifially, Accept: */* Accept: and no Accept header all have specific meanings. The biggest place this comes up is protocol compatibility.  Look at how the Host header plays into HTTP (again).  A missing Host header indicates an old client, a blank Host header indicates an invalid request.  For extensibility sake, not being able to detect if the header was actually specified makes us have to rely on Asterisk Manager/1.0 (or whatever) carrying all the meaning. It just seems that the protocol should say what it means, not assume things in the implementation. However, I submit.  I'll drop this. By: Mark Spencer (markster) 2003-08-19 10:23:28 Thank you. By: Mark Spencer (markster) 2003-09-08 11:44:00 Fixed in CVS