Uploaded image for project: 'ejabberd development'
  1. ejabberd development
  2. EJAB-1739

Improve return of c2s send_element for finer control

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: ejabberd 15.06
    • Component/s: None
    • Labels:
      None

      Description

      in order to fix 8e33f31f507, 2a94c68724a has been introduced.
      but possible return of send_element is not clear, making this patch not 100% safe (maybe).

      % the proper solution is to guarantee that send_element returns ok | {error, Error}  then apply this
      case send_element(StateData, Stanza) of
      ok -> send_element(StateData, AckReq);
      Error -> Error
      end.
      

      for the record, here are some suggestions while thinking with Holger about this
      https://github.com/processone/ejabberd/compare/master...weiss:drop-assumption
      https://github.com/processone/ejabberd/compare/master...weiss:drop-assumption-2
      https://github.com/processone/ejabberd/compare/master...weiss:drop-assumption-3

        Activity

        Hide
        holger Holger Weiß added a comment - - edited

        I checked the code paths send_element/2 can take. As far as I can see, it will always return ok or error or (in theory) error, lager_not_running.

        Anyway, I submitted PR #569, which lets send_element/2 return error, Reason instead of error and also lets send_stanza_and_ack_req/2 interpret any non-ok value as an error. That way, the code should be safe.

        Show
        holger Holger Weiß added a comment - - edited I checked the code paths send_element/2 can take. As far as I can see, it will always return ok or error or (in theory) error, lager_not_running. Anyway, I submitted PR #569 , which lets send_element/2 return error, Reason instead of error and also lets send_stanza_and_ack_req/2 interpret any non-ok value as an error. That way, the code should be safe.
        Hide
        alexey Alexey Shchepin added a comment -

        Thanks, merged.

        Show
        alexey Alexey Shchepin added a comment - Thanks, merged.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development