QuickFIX/J
  1. QuickFIX/J
  2. QFJ-557

Session.generateReject methods may incorrectly increment nextTargetMsgSeqNum in some scenarios

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Default Default
    • Resolution: Fixed
    • Affects Version/s: 1.4.0
    • Fix Version/s: 1.6.0
    • Component/s: Engine
    • Labels:
      None

      Description

      Scenario is as follows:
      - expecting 223
      - received 227 Logon, which we accepted --> "MsgSeqNum too high, expecting 223 but received 227"
      - sent resend request from 223 to 0
      - received 228, which failed data dictionary validation. --> "Message 228 Rejected: Invalid MsgType"
      - sent a Reject
      - received 223 SequenceReset, w/ PossDupFlag=Y, GapFillFlag=Y and NextSeqNo=225. *This gap fill was not applied.*
      - received 225 ExecutionReport w/ PossDupFlag=Y. --> "Rejected MsgSeqNum too high, expecting 224 but received 225", and "Already sent ResendRequest FROM: 223 TO: 226. Not sending another."

      We were able to write a test to reproduce this. Tracing it through the debugger led to the following snippet in generateReject(Message, int, int):
      {code:java}
                  if (!msgType.equals(MsgType.LOGON) && !msgType.equals(MsgType.SEQUENCE_RESET)
                          && (msgSeqNum == getExpectedTargetNum() || !isPossibleDuplicate(message))) {
                      state.incrNextTargetMsgSeqNum();
      {code}
      (There is a similar snippet in the other generateReject method.) The "bad" message 228 is not a Logon, not a SequenceReset and not a possible duplicate. Therefore the nextTargetMsgSeqNum is incremented, even though it should not be.

      Consequently the Session thinks message 223 has too low a sequence number. Normally this would cause a log message and disconnect, but since it is flagged as a possible duplicate and is a SequenceReset, the message is silently dropped instead. See doTargetTooLow(Message) and validatePossDup(Message).

      At this point we do not have a proposed fix. However, it seems clear that message 228 should not have caused an increment to nextTargetMsgSeqNum. The section "|| !isPossibleDuplicate(message)" in the snippet quoted above is a tad mysterious. Or it could be something deeper about how dictionary validation and sequence number validation should be ordered.
      1. diff.txt
        1 kB
        Rhys Yarranton
      2. QFTest.java
        3 kB
        Rhys Yarranton

        Activity

        Hide
        Rhys Yarranton added a comment -

        Test case attached.

        Show
        Rhys Yarranton added a comment - Test case attached.
        Hide
        Rhys Yarranton added a comment -

        After looking at the spec and the history of the source code in question, I suspect that the logic to increment the next expected target sequence number was taken rather literally from the spec, and then a couple of special cases were removed.

        Attached is a patch that changes it to only increment if the current message has the expected sequence number. You can think of this as continuing the tradition of the existing logic.

        The spec makes an implicit assumption that the message in question is in order. You could take that as an argument in favour of reordering of the logic, but it is also possible that the authors of the spec simply didn't envisage this case. Personally I think re-ordering the logic has merit.

        Show
        Rhys Yarranton added a comment - After looking at the spec and the history of the source code in question, I suspect that the logic to increment the next expected target sequence number was taken rather literally from the spec, and then a couple of special cases were removed. Attached is a patch that changes it to only increment if the current message has the expected sequence number. You can think of this as continuing the tradition of the existing logic. The spec makes an implicit assumption that the message in question is in order. You could take that as an argument in favour of reordering of the logic, but it is also possible that the authors of the spec simply didn't envisage this case. Personally I think re-ordering the logic has merit.
        Hide
        Christoph John added a comment -

        Committed as rev 1116: http://sourceforge.net/p/quickfixj/code/1116/
        Thanks to Rhys Yarranton for the patch!

        Show
        Christoph John added a comment - Committed as rev 1116: http://sourceforge.net/p/quickfixj/code/1116/ Thanks to Rhys Yarranton for the patch!

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development