jsip
  1. jsip
  2. JSIP-439

Out of order Acks aren't handled properly. Patch attached.

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      We were seeing some calls failing to connect (using Jitsi (which uses JSIP))

      We tracked this down to JSIP's handling of a ACKs arriving out of order.

      The fix is to not update JAIN's stored sequence number on an ACK (because it's not a unique number). Should fix the specific case we were hitting but probably won't fix all windows.

      Patch file is below...

      Index: D:/Work/workspace/jain-sip.dc/src/gov/nist/javax/sip/stack/SIPDialog.java
      ===================================================================
      — D:/Work/workspace/jain-sip.dc/src/gov/nist/javax/sip/stack/SIPDialog.java (revision 548)
      +++ D:/Work/workspace/jain-sip.dc/src/gov/nist/javax/sip/stack/SIPDialog.java (revision 549)
      @@ -1877,7 +1877,14 @@
      // jeand needed for reinvite reliable processing
      firstTransaction = transaction;
      }

      • if (transaction instanceof SIPServerTransaction) {
        +
        + // Update the current sequence number from the server unless
        + // this was an ACK, in which case it doesn't have a new sequence
        + // number (and updating it here causes problems with crossing
        + // messages).
        + if (transaction instanceof SIPServerTransaction
        + && Request.ACK.equals(this.method))
        + { setRemoteSequenceNumber(sipRequest.getCSeq().getSeqNumber()); }

        Activity

        Hide
        jbemmel added a comment -

        That code seems to only set the sequence number for ACK requests, is that intended? Shouldn't it be "if (!Request.ACK.equals(...) )"

        Show
        jbemmel added a comment - That code seems to only set the sequence number for ACK requests, is that intended? Shouldn't it be "if (!Request.ACK.equals(...) )"
        Hide
        jbemmel added a comment -

        If ACKs are a problem, then CANCELs will be too. See RFC3261 section 9.1 - they have the same sequence number as the INVITE they target

        Show
        jbemmel added a comment - If ACKs are a problem, then CANCELs will be too. See RFC3261 section 9.1 - they have the same sequence number as the INVITE they target
        Hide
        nickbaker added a comment -

        I’m a colleague of Tom’s and have taken over this work. We agree with both your comments (thanks), and I have modified the fix as follows. RFC3261 section 12.2.1.1 says that this only affects ACKs and CANCELs so I believe this fix is complete now.

        Index: SIPDialog.java
        ===================================================================
        — SIPDialog.java (revision 2185)
        +++ SIPDialog.java (working copy)
        @@ -1881,7 +1881,15 @@
        // jeand needed for reinvite reliable processing
        firstTransaction = transaction;
        }

        • if (transaction instanceof SIPServerTransaction) {
          +
          + // RFC-3261 Section - 12.2.1.1 Update the current sequence
          + // number from the server unless this was an ACK or a
          + // CANCEL, in which case it doesn't have a new sequence
          + // number (and updating it here causes problems with
          + // crossing messages).
          + if (transaction instanceof SIPServerTransaction &&
          + !(Request.ACK.equals(this.method) ||
          + Request.CANCEL.equals(this.method))) { setRemoteSequenceNumber(sipRequest.getCSeq().getSeqNumber()); }
        Show
        nickbaker added a comment - I’m a colleague of Tom’s and have taken over this work. We agree with both your comments (thanks), and I have modified the fix as follows. RFC3261 section 12.2.1.1 says that this only affects ACKs and CANCELs so I believe this fix is complete now. Index: SIPDialog.java =================================================================== — SIPDialog.java (revision 2185) +++ SIPDialog.java (working copy) @@ -1881,7 +1881,15 @@ // jeand needed for reinvite reliable processing firstTransaction = transaction; } if (transaction instanceof SIPServerTransaction) { + + // RFC-3261 Section - 12.2.1.1 Update the current sequence + // number from the server unless this was an ACK or a + // CANCEL, in which case it doesn't have a new sequence + // number (and updating it here causes problems with + // crossing messages). + if (transaction instanceof SIPServerTransaction && + !(Request.ACK.equals(this.method) || + Request.CANCEL.equals(this.method))) { setRemoteSequenceNumber(sipRequest.getCSeq().getSeqNumber()); }

          People

          • Assignee:
            Unassigned
            Reporter:
            tomdenham
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated: