Uploaded image for project: 'QuickFIX/J'
  1. QuickFIX/J
  2. QFJ-493

When a gap fill satisfies a resend request, QF does not realize it

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Default
    • Resolution: Cannot Reproduce
    • Affects Version/s: 1.2.1
    • Fix Version/s: None
    • Component/s: Engine
    • Labels:
      None

      Description

      In our production environment, we saw the following scenario:
      They log on w/ 687
      We send resent request from 684 to 0
      They send a resend request (their #688)
      They send gap fill from MsgSeqNum=684 to NewSeqNo=688.
      They send us 689.
      QF did not recognize that the resend request had been fulfilled, so it did not send another resend request when it saw 689.
      The counterparty was never going to send us 688 otherwise - they had already sent it (and indeed we acted on it).
      So we ended up not processing any of their messages (sequence number too high), and eventually disconnected.

      We tracked the problem down to the
      boolean verify(Message msg, boolean checkTooHigh, boolean checkTooLow)
      method around line 1168. If the MsgSeqNum is at or beyond the end of the resend range waiting to be satisfied, then the resend request will be deemed satisfied. But for gap fills, it should really consider the entire range of the gap, i.e., MsgSeqNum to NewSeqNo - 1, which it does not. NB that in some scenarios, the next inbound message will clear the problem. But not in the one above.

      We are running 1.2.1. Based on inspection of the code, I believe this also affects 1.3.x and 1.4.x.

      Will attach a patch, diff.txt.

        Attachments

          Activity

          Hide
          ryarran1 Rhys Yarranton added a comment -

          Hmm, I can't find the link to add an attachment, so I will paste the patch here instead.

          — core/src/main/java/quickfix/Session.java 2009-12-08 16:28:15.657576498 -0700
          +++ core/src/main/java/quickfix/Session.java.original 2009-12-08 16:15:26.719542742 -0700
          @@ -1166,13 +1166,9 @@
          }

          if ((checkTooHigh || checkTooLow) && state.isResendRequested()) {

          • int endPoint = msgSeqNum;
          • if(msg.getHeader().getString(MsgType.FIELD).equals(MsgType.SEQUENCE_RESET) &&
          • msg.isSetField(NewSeqNo.FIELD))
          • endPoint = Math.max(endPoint, msg.getInt(NewSeqNo.FIELD) - 1);
            synchronized (this) {
            int[] range = state.getResendRange();
          • if (endPoint >= range[1]) {
            + if (msgSeqNum >= range[1]) {
            getLog().onEvent(
            "ResendRequest for messages FROM: " + range[0] + " TO: " + range[1]
            + " has been satisfied.");
          Show
          ryarran1 Rhys Yarranton added a comment - Hmm, I can't find the link to add an attachment, so I will paste the patch here instead. — core/src/main/java/quickfix/Session.java 2009-12-08 16:28:15.657576498 -0700 +++ core/src/main/java/quickfix/Session.java.original 2009-12-08 16:15:26.719542742 -0700 @@ -1166,13 +1166,9 @@ } if ((checkTooHigh || checkTooLow) && state.isResendRequested()) { int endPoint = msgSeqNum; if(msg.getHeader().getString(MsgType.FIELD).equals(MsgType.SEQUENCE_RESET) && msg.isSetField(NewSeqNo.FIELD)) endPoint = Math.max(endPoint, msg.getInt(NewSeqNo.FIELD) - 1); synchronized (this) { int[] range = state.getResendRange(); if (endPoint >= range [1] ) { + if (msgSeqNum >= range [1] ) { getLog().onEvent( "ResendRequest for messages FROM: " + range [0] + " TO: " + range [1] + " has been satisfied.");
          Hide
          ryarran Rhys Yarranton added a comment -

          Attachments seem to be working again. This one is a patch against 1.4.0.

          Show
          ryarran Rhys Yarranton added a comment - Attachments seem to be working again. This one is a patch against 1.4.0.
          Hide
          chrjohn Christoph John added a comment -

          Was not able to reproduce this. Added unit test: http://sourceforge.net/p/quickfixj/code/1170/

          Show
          chrjohn Christoph John added a comment - Was not able to reproduce this. Added unit test: http://sourceforge.net/p/quickfixj/code/1170/

            People

            • Assignee:
              chrjohn Christoph John
              Reporter:
              ryarran1 Rhys Yarranton
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: