[QFJ-557] Session.generateReject methods may incorrectly increment nextTargetMsgSeqNum in some scenarios Created: 10/Sep/10  Updated: 02/Apr/15  Resolved: 07/Nov/13

Status: Closed
Project: QuickFIX/J
Component/s: Engine
Affects Version/s: 1.4.0
Fix Version/s: 1.6.0

Type: Bug Priority: Default
Reporter: Rhys Yarranton Assignee: Christoph John
Resolution: Fixed Votes: 0
Labels: None

Attachments: Text File diff.txt     Text File QFTest.java    

 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):

            if (!msgType.equals(MsgType.LOGON) && !msgType.equals(MsgType.SEQUENCE_RESET)
                    && (msgSeqNum == getExpectedTargetNum() || !isPossibleDuplicate(message))) {
                state.incrNextTargetMsgSeqNum();

(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.



 Comments   
Comment by Rhys Yarranton [ 10/Sep/10 ]

Test case attached.

Comment by Rhys Yarranton [ 16/Sep/10 ]

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.

Comment by Christoph John [ 07/Nov/13 ]

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

Generated at Fri Mar 29 02:10:57 UTC 2024 using JIRA 7.5.2#75007-sha1:9f5725bb824792b3230a5d8716f0c13e296a3cae.