diff --git a/quickfixj-core/src/main/java/quickfix/Session.java b/quickfixj-core/src/main/java/quickfix/Session.java index f9151daf8..a3938b58f 100644 --- a/quickfixj-core/src/main/java/quickfix/Session.java +++ b/quickfixj-core/src/main/java/quickfix/Session.java @@ -2377,10 +2377,9 @@ private void resendMessages(Message receivedMessage, int beginSeqNo, int endSeqN int msgSeqNum = 0; int begin = 0; int current = beginSeqNo; - boolean appMessageJustSent = false; + int newBegin = beginSeqNo; for (final String message : messages) { - appMessageJustSent = false; final Message msg; try { // QFJ-626 @@ -2418,7 +2417,7 @@ private void resendMessages(Message receivedMessage, int beginSeqNo, int endSeqN return; } begin = 0; - appMessageJustSent = true; + newBegin = msgSeqNum + 1; } else { if (begin == 0) { begin = msgSeqNum; @@ -2427,28 +2426,11 @@ private void resendMessages(Message receivedMessage, int beginSeqNo, int endSeqN } current = msgSeqNum + 1; } - - int newBegin = beginSeqNo; - if (appMessageJustSent) { + if (begin != 0) { + generateSequenceReset(receivedMessage, begin, msgSeqNum + 1); newBegin = msgSeqNum + 1; } - if (enableNextExpectedMsgSeqNum) { - if (begin != 0) { - generateSequenceReset(receivedMessage, begin, msgSeqNum + 1); - } else { - /* - * I've added an else here as I managed to fail this without it in a unit test, however the unit test data - * may not have been realistic to production on the other hand. - * Apart from the else - */ - generateSequenceResetIfNeeded(receivedMessage, newBegin, endSeqNo, msgSeqNum); - } - } else { - if (begin != 0) { - generateSequenceReset(receivedMessage, begin, msgSeqNum + 1); - } - generateSequenceResetIfNeeded(receivedMessage, newBegin, endSeqNo, msgSeqNum); - } + generateSequenceResetIfNeeded(receivedMessage, newBegin, endSeqNo, msgSeqNum); } private void generateSequenceResetIfNeeded(Message receivedMessage, int beginSeqNo, int endSeqNo, int msgSeqNum) diff --git a/quickfixj-core/src/test/java/quickfix/SessionTest.java b/quickfixj-core/src/test/java/quickfix/SessionTest.java index dc70f958c..db57e655e 100644 --- a/quickfixj-core/src/test/java/quickfix/SessionTest.java +++ b/quickfixj-core/src/test/java/quickfix/SessionTest.java @@ -2338,6 +2338,195 @@ public void correct_sequence_number_for_last_gap_fill_if_next_sender_sequence_nu assertEquals(lastGapFill.getHeader().getString(MsgSeqNum.FIELD), "6"); } + /** + * Regression test for GitHub issue #344 ({@code enableNextExpectedMsgSeqNum=false}). + * + *

Bug: When {@code resendMessages} processes a range where the last retrieved + * persisted message is an admin message (e.g. a Heartbeat at seqno 5), + * {@code appMessageJustSent} is {@code false} at the end of the loop. + * Therefore {@code newBegin} is never updated from its initial value of + * {@code beginSeqNo} (= 1 in this scenario) and the subsequent call to + * {@code generateSequenceResetIfNeeded} emits a SequenceReset-GapFill with + * {@code MsgSeqNum=1} instead of the correct {@code MsgSeqNum=6}. + * + *

The test asserts the correct value (6) and therefore + * fails on the current buggy code, serving as a + * regression test once the bug is fixed. + * + *

Scenario: + *

+ * + *

Expected outgoing messages when the counterparty's ResendRequest arrives: + *

    + *
  1. SequenceReset(MsgSeqNum=1, NewSeqNo=3) – gap-fill for admin 1-2
    2. + *
  2. ExecutionReport(3) resent (PossDupFlag)
    3. + *
  3. ExecutionReport(4) resent (PossDupFlag)
    4. + *
  4. SequenceReset(MsgSeqNum=5, NewSeqNo=6) – gap-fill for admin Heartbeat at 5
    5. + *
  5. SequenceReset(MsgSeqNum=6, NewSeqNo=11) – gap-fill for admin 6-10 (BUG gives 1)
    6. + *
+ */ + @Test + public void resendMessages_whenLastRetrievedMessageIsAdmin_finalSequenceResetShouldNotOverlapResentAppMessages() + throws IOException, InvalidMessage, FieldNotFound, RejectLogon, UnsupportedMessageType, + IncorrectTagValue, IncorrectDataFormat { + doResendMessages_whenLastRetrievedMessageIsAdmin_scenario(false); + } + + /** + * Regression test for GitHub issue #344 ({@code enableNextExpectedMsgSeqNum=true}). + * + *

Same bug as the non-{@code enableNextExpectedMsgSeqNum} variant: when + * {@code resendMessages} ends on an admin message, {@code newBegin} is + * not advanced. With {@code enableNextExpectedMsgSeqNum=true} the post-loop + * code takes a different branch, so the trailing gap-fill SequenceReset is + * omitted entirely rather than being emitted with the wrong sequence number. + * + *

The test asserts the correct output (7 messages total, last one + * being SequenceReset(MsgSeqNum=6, NewSeqNo=11)) and therefore + * fails on the current buggy code. + * + *

Scenario (with {@code enableNextExpectedMsgSeqNum=true}, no explicit + * ResendRequest is sent by the session at logon time, so app messages start + * one seqno earlier than in the non-{@code enableNextExpectedMsgSeqNum} variant): + *

+ * + *

Expected outgoing messages when the counterparty's ResendRequest arrives: + *

    + *
  1. SequenceReset(MsgSeqNum=1, NewSeqNo=2) – gap-fill for admin 1
    2. + *
  2. ExecutionReport(2) resent (PossDupFlag)
    3. + *
  3. ExecutionReport(3) resent (PossDupFlag)
    4. + *
  4. ExecutionReport(4) resent (PossDupFlag)
    5. + *
  5. SequenceReset(MsgSeqNum=5, NewSeqNo=6) – gap-fill for admin Heartbeat at 5
    6. + *
  6. SequenceReset(MsgSeqNum=6, NewSeqNo=11) – gap-fill for admin 6-10 (BUG: omitted)
    7. + *
+ */ + @Test + public void resendMessages_whenLastRetrievedMessageIsAdmin_finalSequenceResetShouldNotOverlapResentAppMessages_withEnableNextExpectedMsgSeqNum() + throws IOException, InvalidMessage, FieldNotFound, RejectLogon, UnsupportedMessageType, + IncorrectTagValue, IncorrectDataFormat { + doResendMessages_whenLastRetrievedMessageIsAdmin_scenario(true); + } + + /** + * Shared scenario driver for the two issue #344 regression tests. + * + *

With {@code enableNextExpectedMsgSeqNum=false} the session sends an explicit + * ResendRequest (seqno 2) after receiving the counterparty's too-high Logon, so + * the application messages are stored at seqnos 3 and 4. + * With {@code enableNextExpectedMsgSeqNum=true} the resend is implicit via tag 789 + * in the outgoing Logon and no ResendRequest is transmitted, so the application + * messages start one seqno earlier (seqnos 2, 3 and 4). + * In both cases the Heartbeat ends up at seqno 5 and + * {@code NextSenderMsgSeqNum} ends at 11, giving 7 total outgoing messages. + */ + private void doResendMessages_whenLastRetrievedMessageIsAdmin_scenario(boolean enableNextExpectedMsgSeqNum) + throws IOException, InvalidMessage, FieldNotFound, RejectLogon, UnsupportedMessageType, + IncorrectTagValue, IncorrectDataFormat { + final SessionID sessionID = new SessionID(FixVersions.BEGINSTRING_FIX44, "SENDER", "TARGET"); + final boolean resetOnLogon = false; + final boolean validateSequenceNumbers = true; + + Session session = new Session(new UnitTestApplication(), new MemoryStoreFactory(), new InMemoryMessageQueueFactory(), + sessionID, null, null, null, null, + new DefaultMessageFactory(), 30, false, 30, UtcTimestampPrecision.MILLIS, resetOnLogon, + false, false, false, false, false, true, false, 1.5, null, validateSequenceNumbers, + new int[]{5}, false, false, false, false, true, false, true, false, null, true, 0, + enableNextExpectedMsgSeqNum, false, true, new ArrayList<>(), Session.DEFAULT_HEARTBEAT_TIMEOUT_MULTIPLIER, false); + + Responder mockResponder = mock(Responder.class); + when(mockResponder.send(anyString())).thenReturn(true); + session.setResponder(mockResponder); + + // Send Logon (seqno 1); NextSenderMsgSeqNum becomes 2. + session.logon(); + session.next(); + + ArgumentCaptor messageCaptor = ArgumentCaptor.forClass(String.class); + verify(mockResponder).send(messageCaptor.capture()); + + // Receive Logon from counterparty with seqno 101 (too high). + // With enableNextExpectedMsgSeqNum=false: session sends an explicit ResendRequest + // (seqno 2); NextSenderMsgSeqNum becomes 3. + // With enableNextExpectedMsgSeqNum=true: resend is implicit via tag 789; no + // ResendRequest is transmitted; NextSenderMsgSeqNum stays at 2. + session.next(createLogonResponse(sessionID, new Message(messageCaptor.getValue()), 101)); + MessageStore messageStore = session.getStore(); + + // Store application messages (ExecutionReports). + // The start seqno is read from the store so it adapts to whether a ResendRequest + // was sent (false → starts at 3; true → starts at 2). The loop always stops at 4. + for (int i = messageStore.getNextSenderMsgSeqNum(); i <= 4; i++) { + String executionReportString = "8=FIX.4.2\0019=0246\00135=8\001115=THEM\00134=" + i + "\00143=Y\001122=20100908-17:52:37.920\00149=THEM\00156=US\001369=178\00152=20100908-17:59:30.642\00137=10118506\00111=a00000052.1\00117=17537743\00120=0\001150=4\00139=4\00155=ETFC\00154=1\00138=500000\00144=0.998\00132=0\00131=0\001151=0\00114=0\0016=0\00160=20100908-17:52:37.920\00110=80\001"; + messageStore.set(i, executionReportString); + messageStore.incrNextSenderMsgSeqNum(); + } + + // Store an admin (Heartbeat) message at seqno 5 – the last retrieved message. + // This is the key ingredient for bug #344: when the resendMessages loop ends on an + // admin message, appMessageJustSent is false, so newBegin is never updated from + // its initial value of beginSeqNo, causing the final SequenceReset to be wrong. + Heartbeat heartbeat = new Heartbeat(); + heartbeat.getHeader().setString(SenderCompID.FIELD, "THEM"); + heartbeat.getHeader().setString(TargetCompID.FIELD, "US"); + heartbeat.getHeader().setInt(MsgSeqNum.FIELD, 5); + heartbeat.getHeader().setUtcTimeStamp(SendingTime.FIELD, LocalDateTime.now(ZoneOffset.UTC)); + messageStore.set(5, heartbeat.toString()); + messageStore.incrNextSenderMsgSeqNum(); // NextSenderMsgSeqNum = 6 + + // Simulate admin messages 6-10 that were sent but not persisted. + for (int i = 0; i < 5; i++) { + messageStore.incrNextSenderMsgSeqNum(); // ends at 11 + } + + // Counterparty sends a ResendRequest for all our messages. + // The session will call resendMessages(1, 10). + final Message resendRequest = createResendRequest(1, 1); + session.next(resendRequest); + + // Seven messages should have been sent in total. + // With enableNextExpectedMsgSeqNum=false: + // 1. Logon (seqno 1) + // 2. ResendRequest (seqno 2, our request to counterparty) + // 3. SequenceReset(MsgSeqNum=1, NewSeqNo=3) – gap-fill for admin 1-2 + // 4. ExecutionReport(3) resent + // 5. ExecutionReport(4) resent + // 6. SequenceReset(MsgSeqNum=5, NewSeqNo=6) – gap-fill for Heartbeat at 5 + // 7. SequenceReset(MsgSeqNum=?, NewSeqNo=11) – trailing gap 6-10 (BUG: MsgSeqNum=1) + // With enableNextExpectedMsgSeqNum=true: + // 1. Logon (seqno 1) + // 2. SequenceReset(MsgSeqNum=1, NewSeqNo=2) – gap-fill for admin 1 + // 3. ExecutionReport(2) resent + // 4. ExecutionReport(3) resent + // 5. ExecutionReport(4) resent + // 6. SequenceReset(MsgSeqNum=5, NewSeqNo=6) – gap-fill for Heartbeat at 5 + // 7. SequenceReset(MsgSeqNum=6, NewSeqNo=11) – trailing gap 6-10 (BUG: omitted) + verify(mockResponder, times(7)).send(messageCaptor.capture()); + + Message lastSent = new Message(messageCaptor.getAllValues().get(messageCaptor.getAllValues().size() - 1)); + assertEquals("Last message should be a SequenceReset-GapFill", + MsgType.SEQUENCE_RESET, lastSent.getHeader().getString(MsgType.FIELD)); + // Bug #344: current code gives MsgSeqNum=1 (beginSeqNo) instead of 6 (last admin seqno + 1) + // for the enableNextExpectedMsgSeqNum=false case, and omits the trailing reset entirely + // for the enableNextExpectedMsgSeqNum=true case (causing times(7) above to fail first). + assertEquals("SequenceReset covering trailing gap should begin at seqno 6 (just after the admin " + + "Heartbeat at 5), not at beginSeqNo=1", + "6", lastSent.getHeader().getString(MsgSeqNum.FIELD)); + assertEquals("SequenceReset NewSeqNo should advance to NextSenderMsgSeqNum=11", + "11", lastSent.getString(NewSeqNo.FIELD)); + } + @Test // QFJ-795 public void testMsgSeqNumTooHighWithDisconnectOnError() throws Exception {