Asterisk
  1. Asterisk
  2. ASTERISK-119

[patch] Manager Protocol Ambiguous For Long Responses [Post 1.0]

    Details

    • Type: Bug Bug
    • Status: Closed
    • Severity: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Target Release Version/s: None
    • Component/s: Core/General
    • Labels:
      None
    • Mantis ID:
      123
    • Regression:
      No

      Description

      Certain Actions return arbitrarily long responses that do not fit the format for the protocol (requiring extensive special casing to implement). This should be handled explicitly by the protocol.

                • ADDITIONAL INFORMATION ******

      My beef lies with the Command action. It's response comes as:

      Response: Follows
      Many
      Lines
      Of
      Ill
      Formatted
      Text
      -END COMMAND-

      This currently is only an issue with the Command action, but we will likely encounter it later.

      Specifically, we have a very elegant protocol for communicating events/requests (it's even symmetrical!), but we kludge in long responses.

      I propose one of two ways of making a standard response format. Either break it into lines and return multiple Events (like Status). This would also benefit from a concrete closure event (ala bug ASTERISK-118).

      It may also be useful to define a standard way to encode the data as either plain or Base64 (to allow binary data).

      While individual events and Base64 for this could waste space, I see no other good way to:

      a) prevent jamming the manager pipeline with a large, atomic response
      b) potentially return binary data
      c) allow for long-running responses (perhaps some sort of AGI over manager-protocol become possible)
      d) allow other responses to be interspersed with output (ties in with a/b/c and scalability)

      Right now we seem to handle this case-by-case. I would recommend defining it in the protocol before someone develops important software that depends on this (thus making a compatability/stable-interface concern).

        Activity

        Hide
        jayson added a comment -

        Noticed a few slight inconsistencies in my patch. I've made some changes that actually make it more useful in the process.

        Specifically, the first patch would prepend a colon to payload lines. While this made it parseable, my purpose for choosing a colon was to allow payload lines to function like lines with an "empty key". That way, you could write your implementation to split on a colon, and lines without keys are your payload.

        The problem was, the manager protocol actually specifies headers be delimited with a colon FOLLOWED BY a space. I adjusted the prefix system to allow prefixing with arbitrary strings instead of single characters. This may have increased the memory footprint somewhat, however, I can now print each line appropriately prefixed so as to keep the protocol clean.

        I also removed the -END COMMAND- string. The prefixes conveyed this information nicely. Since we were already breaking compatibility with the old Command syntax (not apparently widely used), I figured it would be good to remove the other cruft now.

        I also updated the manager protocol document to reflect this change. My wording may not be best, so I welcome any comments.

        The new patch will be attached as mgr123-2.patch

        Show
        jayson added a comment - Noticed a few slight inconsistencies in my patch. I've made some changes that actually make it more useful in the process. Specifically, the first patch would prepend a colon to payload lines. While this made it parseable, my purpose for choosing a colon was to allow payload lines to function like lines with an "empty key". That way, you could write your implementation to split on a colon, and lines without keys are your payload. The problem was, the manager protocol actually specifies headers be delimited with a colon FOLLOWED BY a space. I adjusted the prefix system to allow prefixing with arbitrary strings instead of single characters. This may have increased the memory footprint somewhat, however, I can now print each line appropriately prefixed so as to keep the protocol clean. I also removed the - END COMMAND - string. The prefixes conveyed this information nicely. Since we were already breaking compatibility with the old Command syntax (not apparently widely used), I figured it would be good to remove the other cruft now. I also updated the manager protocol document to reflect this change. My wording may not be best, so I welcome any comments. The new patch will be attached as mgr123-2.patch
        Hide
        jayson added a comment -

        ast_man doesn't use Command anywhere. I don't really see that it's parser cares about this. Exactly what would you want me to update to support this?

        The input_check function actually will parse this already. The existing get_header function which should neatly ignore this change. I can implement a get_payload (or get_body) function to grab a response like this. Nothing would use it initially. A get_body_first_line and get_body_next_line function pair would be childs play. If you're happy with the API, I could do the latter in about 30 minutes. Let me know.

        Jayson

        Show
        jayson added a comment - ast_man doesn't use Command anywhere. I don't really see that it's parser cares about this. Exactly what would you want me to update to support this? The input_check function actually will parse this already. The existing get_header function which should neatly ignore this change. I can implement a get_payload (or get_body) function to grab a response like this. Nothing would use it initially. A get_body_first_line and get_body_next_line function pair would be childs play. If you're happy with the API, I could do the latter in about 30 minutes. Let me know. Jayson
        Hide
        jayson added a comment -

        I've been using this in solid production for a month now.

        It's proven to be rock solid for me.

        Any problems with applying the patch?

        Show
        jayson added a comment - I've been using this in solid production for a month now. It's proven to be rock solid for me. Any problems with applying the patch?
        Hide
        Olle Johansson added a comment -

        The general standing seems to be that this big a change has to wait to be considered after 1.0 release. We're closing it pending 1.0 release with [Post1.0] marker to catch it afterwards. Please bug a bug marshal when you want it re-considered.

        Show
        Olle Johansson added a comment - The general standing seems to be that this big a change has to wait to be considered after 1.0 release. We're closing it pending 1.0 release with [Post1.0] marker to catch it afterwards. Please bug a bug marshal when you want it re-considered.
        Hide
        Olle Johansson added a comment -

        ---To be re-evaluated after 1.0 release, closing temporarily –

        Show
        Olle Johansson added a comment - ---To be re-evaluated after 1.0 release, closing temporarily –

          People

          • Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development