Merge pull request #957 from gbirchmeier/i309-fix

solve issue #309: obey a GapFill even if it replaces a message that was processed off the queue

Author: gbirchmeier

AcceptanceTest/definitions/server/fix44/issue309_GapFillSkip.def

@@ -0,0 +1,33 @@
+# issue 309
+# When a GapFill skips over seqnos past the ExpectedNextTargetSeqNum,
+#   the engine needs to skip with it.
+
+iCONNECT
+I8=FIX.4.4
35=A
34=1
49=TW
52=<TIME>
56=ISLD
98=0
108=2

+E8=FIX.4.4
35=A
34=1
49=ISLD
52=00000000-00:00:00.000
56=TW
98=0
108=2
10=0

+
+# News msg to bump the seq up
+I8=FIX.4.4
35=B
34=2
49=TW
52=<TIME>
56=ISLD
148=no_echo

+
+# Heartbeat with seq 5 (when 3 is expected)
+I8=FIX.4.4
35=0
34=5
49=TW
52=<TIME>
56=ISLD

+
+# Expect ResendRequest for 3+
+E8=FIX.4.4
35=2
34=2
49=ISLD
52=00000000-00:00:00.000
56=TW
7=3
16=0
10=0

+
+# Resend 3 and 4
+I8=FIX.4.4
35=B
34=3
43=Y
49=TW
52=<TIME>
56=ISLD
148=no_echo

+I8=FIX.4.4
35=B
34=4
43=Y
49=TW
52=<TIME>
56=ISLD
148=no_echo

+sleep(0.2)
+
+# Session now processes the Heartbeat (seq=5) from its queue
+E8=FIX.4.4
35=0
34=3
49=ISLD
52=00000000-00:00:00.000
56=TW
10=0

+
+# Counterparty is still resending - it is on 5 now.
+# 5 is that heartbeat, an admin msg, so send a GapFill instead.
+# The EndSeqNo is 7, completely skipping over 6.
+I8=FIX.4.4
35=4
34=5
49=TW
52=<TIME>
56=ISLD
43=Y
122=<TIME-1>
36=7
123=Y

+
+# Do an echo to ensure that client obeyed the SequenceReset and set NextExpected=7
+I8=FIX.4.4
35=B
34=7
49=TW
52=<TIME>
56=ISLD
148=echo: NextExpected/7

+E8=FIX.4.4
35=B
34=4
49=ISLD
52=00000000-00:00:00.000
56=TW
148=NextExpected/7
10=0

QuickFIXn/Session.cs

@@ -523,6 +523,7 @@ public void Next()
         public void Next(string msgStr)
         {
             NextMessage(msgStr);
+            _state.LastProcessedMessageWasQueued = false;
             NextQueued();
         }
 
@@ -962,8 +963,20 @@ public bool Verify(Message msg, bool checkTooHigh = true, bool checkTooLow = tru
                     }
                     if (checkTooLow && IsTargetTooLow(msgSeqNum))
                     {
-                        DoTargetTooLow(msg, msgSeqNum);
-                        return false;
+                        if (_state.LastProcessedMessageWasQueued
+                            && msg.Header.GetString(35) == MsgType.SEQUENCE_RESET
+                            && msg.GetBoolean(Tags.GapFillFlag))
+                        {
+                            Log.Log(LogLevel.Warning,
+                                "SequenceReset-GapFill 34={MsgSeqNum} is too low (expected {NextTargetMsgSeqNum}), but in this case I'm going to obey it anyway",
+                                msgSeqNum, _state.NextTargetMsgSeqNum);
+                            // This is an uncommon situation, see #309
+                        }
+                        else
+                        {
+                            DoTargetTooLow(msg, msgSeqNum);
+                            return false;
+                        }
                     }
 
                     if (IsResendRequested)
@@ -1594,6 +1607,7 @@ protected bool NextQueued(SeqNumType num)
                 {
                     NextMessage(msg.ConstructString());
                 }
+                _state.LastProcessedMessageWasQueued = true;
                 return true;
             }
             return false;

QuickFIXn/SessionState.cs

@@ -48,6 +48,12 @@ public bool IsInitiator
 
         public ILogger Log { get; }
 
+        /// <summary>
+        /// True if the last message processed was an admin message from the queue.
+        /// Needed for an obscure SequenceReset scenario (see issue #390).
+        /// </summary>
+        internal bool LastProcessedMessageWasQueued { get; set; }
+
         #endregion
 
         #region Synchronized Properties

RELEASE_NOTES.md

@@ -45,7 +45,7 @@ What's New
 * #939 - minor checkTooHigh/checkTooLow refactor in Session.cs (gbirchmeier)
 * #941 - clarify ResendRequest-related log message, add UT coverage for Session (gbirchmeier)
 * #895 - fix: When SSLCACertificate is empty an error is logged and it fails to start (dckorben)
-* #942 - fix #942: field 369 (LastMsgSeqNumProcessed) wrong in ResendRequest message (gbirchmeier)
+* #942 - fix: field 369 (LastMsgSeqNumProcessed) wrong in ResendRequest message (gbirchmeier)
 * #940 - Create an alternate CharEncoding.GetBytes impl which uses ArrayPool to improve memory performance (VAllens)
 * #951 - fix: restore Session disconnect during SocketInitiatorThread.Read exception (gbirchmeier/trevor-bush)
 * #963 - fix: concurrency bug with NonSessionLog on Windows (gbirchmeier)
@@ -57,6 +57,8 @@ What's New
 * #969 - correct LinesOfText in DDs to not be required; make ATs not auto-echo News (gbirchmeier)
 * #965 - Reusing StringBuilder with Object Pooling (VAllens)
 * #980 - fix: ToJSON returns invalid json when content contains newlines/tabs/etc (Rob-Hague)
+* #309 - fix: obey a SeqReset-GapFill even if it 'replaces' a message that was processed off
+         queue in a ResendRequest series (gbirchmeier/oclancy)
 
 ### v1.13.1
 * backport #951 to 1.13

UnitTests/SessionTest.cs

@@ -762,4 +762,103 @@ public void TestBasicResendRequest_WithMax() {
                 Assert.That(_session.IsResendRequested, Is.False, $"seq num {i}");
         }
     }
+
+    [Test]
+    public void TestSequenceResetNoGapFillIsProcessed()
+    {
+        Assert.That(_session!.IgnorePossDupResendRequests, Is.EqualTo(false));
+        Logon();
+        SendNOSMessage();
+        SendNOSMessage();
+        Assert.That(_session.NextTargetMsgSeqNum, Is.EqualTo(4));
+
+        QuickFix.FIX42.SequenceReset sr = new(new NewSeqNo(150));
+        SendTheMessage(sr);
+
+        Assert.That(_session.NextTargetMsgSeqNum, Is.EqualTo(150));
+    }
+
+    [Test]
+    public void TestSequenceResetWithGapFillIsProcessed()
+    {
+        Assert.That(_session!.IgnorePossDupResendRequests, Is.EqualTo(false));
+        Logon();
+        SendNOSMessage();
+        SendNOSMessage();
+        Assert.That(_session.NextTargetMsgSeqNum, Is.EqualTo(4));
+
+        QuickFix.FIX42.SequenceReset sr = new(new NewSeqNo(150));
+        sr.GapFillFlag = new GapFillFlag(true);
+        SendTheMessage(sr);
+
+        Assert.That(_session.NextTargetMsgSeqNum, Is.EqualTo(150));
+    }
+
+    [Test]
+    public void TestSequenceResetDuringResendRequestIsProcessed()
+    {
+        Assert.That(_session!.IgnorePossDupResendRequests, Is.EqualTo(false));
+        _session.RequiresOrigSendingTime = false; // default is true
+        Logon();
+        SendNOSMessage(); // seq 2
+        SendNOSMessage(10); // causes ResendRequest to be sent
+        Assert.That(_session.NextTargetMsgSeqNum, Is.EqualTo(3));
+
+        var resendRequest =
+            (QuickFix.FIX42.ResendRequest)_responder.MsgLookup[MsgType.RESEND_REQUEST].Dequeue();
+        Assert.That(resendRequest.BeginSeqNo.Value, Is.EqualTo(3));
+        Assert.That(resendRequest.EndSeqNo.Value, Is.EqualTo(0));
+
+        // Resend & GapFill through seq=9
+        SendNOSMessage(3);
+        SendNOSMessage(4);
+        QuickFix.FIX42.SequenceReset sr = new(new NewSeqNo(9));
+        SendTheMessage(sr, 5);
+        SendNOSMessage(9);
+
+        // We already processed 10, so the next one is 11
+        Assert.That(_session.NextTargetMsgSeqNum, Is.EqualTo(11));
+    }
+
+    [Test]
+    public void TestObeyGapFillIfItReplacesAMessageOffTheQueue()
+    {
+        // issue #309: If GapFill has the same seqno of a message that was
+        //   processed off the queue, obey it anyway
+
+        Assert.That(_session!.IgnorePossDupResendRequests, Is.EqualTo(false));
+        _session.RequiresOrigSendingTime = false; // default is true
+        Logon();
+        SendNOSMessage(); // seq 2
+
+        QuickFix.FIX42.Heartbeat hb = new();
+        SendTheMessage(hb, 5); // seq=5, causes ResendRequest to be sent
+
+        // Verify the ResendRequest that was just sent
+        var resendRequest =
+            (QuickFix.FIX42.ResendRequest)_responder.MsgLookup[MsgType.RESEND_REQUEST].Dequeue();
+        Assert.That(resendRequest.BeginSeqNo.Value, Is.EqualTo(3));
+        Assert.That(resendRequest.EndSeqNo.Value, Is.EqualTo(0));
+
+        Assert.That(_session.NextTargetMsgSeqNum, Is.EqualTo(3));
+        Assert.That(_session.IsResendRequested);
+
+        // Resends
+        SendNOSMessage(3, possDupFlag: true);
+        SendNOSMessage(4, possDupFlag: true);
+        // Now the session processes the Heartbeat/seq=5 from its queue.
+        Assert.That(_session.NextTargetMsgSeqNum, Is.EqualTo(6));
+
+        // But the counterparty still needs to resend the seq=5!
+        // The 5 is a Heartbeat, an admin message, so it's a gapfill.
+        // The gapfill goes to 7, omitting ever sending a 6.
+        QuickFix.FIX42.SequenceReset sr = new(new NewSeqNo(7));
+        sr.Header.SetField(new PossDupFlag(true));
+        sr.SetField(new GapFillFlag(true));
+        SendTheMessage(sr, 5);
+
+        // Even though client already processed the original Heartbeat/seq=5,
+        //   it must not discard the GapFill 5.
+        Assert.That(_session.NextTargetMsgSeqNum, Is.EqualTo(7));
+    }
 }

UnitTests/SessionTestBase.cs

@@ -183,9 +183,12 @@ public void SendNOSMessage()
         _session!.Next(CreateNOSMessage(_seqNum++).ConstructString());
     }
 
-    public void SendNOSMessage(SeqNumType n)
+    public void SendNOSMessage(SeqNumType n, bool possDupFlag=false)
     {
-        _session!.Next(CreateNOSMessage(n).ConstructString());
+        var msg = CreateNOSMessage(n);
+        if (possDupFlag)
+            msg.Header.SetField(new QuickFix.Fields.PossDupFlag(possDupFlag));
+        _session!.Next(msg.ConstructString());
     }
 
     public void SendResendRequest(SeqNumType begin, SeqNumType end)
@@ -207,7 +210,6 @@ protected void SendTheMessage(QuickFix.Message msg, SeqNumType n = 0)
         msg.Header.SetField(new QuickFix.Fields.TargetCompID(_sessionId.SenderCompID));
         msg.Header.SetField(new QuickFix.Fields.SenderCompID(_sessionId.TargetCompID));
         msg.Header.SetField(new QuickFix.Fields.MsgSeqNum(n == 0 ? _seqNum++ : n));
-
         _session!.Next(msg.ConstructString());
     }
 }