Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gbirchmeier merged 2 commits into from
Oct 16, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25e75d9
reimplement @oclancy's SeqReset UTs from PR #784
gbirchmeier Mar 26, 2025
07f8a7a
issue #309: obey a GapFill even if it replaces...
gbirchmeier Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions AcceptanceTest/definitions/server/fix44/issue309_GapFillSkip.def
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.435=A34=149=TW52=<TIME>56=ISLD98=0108=2
E8=FIX.4.435=A34=149=ISLD52=00000000-00:00:00.00056=TW98=0108=210=0

# News msg to bump the seq up
I8=FIX.4.435=B34=249=TW52=<TIME>56=ISLD148=no_echo

# Heartbeat with seq 5 (when 3 is expected)
I8=FIX.4.435=034=549=TW52=<TIME>56=ISLD

# Expect ResendRequest for 3+
E8=FIX.4.435=234=249=ISLD52=00000000-00:00:00.00056=TW7=316=010=0

# Resend 3 and 4
I8=FIX.4.435=B34=343=Y49=TW52=<TIME>56=ISLD148=no_echo
I8=FIX.4.435=B34=443=Y49=TW52=<TIME>56=ISLD148=no_echo
sleep(0.2)

# Session now processes the Heartbeat (seq=5) from its queue
E8=FIX.4.435=034=349=ISLD52=00000000-00:00:00.00056=TW10=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.435=434=549=TW52=<TIME>56=ISLD43=Y122=<TIME-1>36=7123=Y

# Do an echo to ensure that client obeyed the SequenceReset and set NextExpected=7
I8=FIX.4.435=B34=749=TW52=<TIME>56=ISLD148=echo: NextExpected/7
E8=FIX.4.435=B34=449=ISLD52=00000000-00:00:00.00056=TW148=NextExpected/710=0
18 changes: 16 additions & 2 deletions QuickFIXn/Session.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,7 @@ public void Next()
public void Next(string msgStr)
{
NextMessage(msgStr);
_state.LastProcessedMessageWasQueued = false;
NextQueued();
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -1594,6 +1607,7 @@ protected bool NextQueued(SeqNumType num)
{
NextMessage(msg.ConstructString());
}
_state.LastProcessedMessageWasQueued = true;
return true;
}
return false;
Expand Down
6 changes: 6 additions & 0 deletions QuickFIXn/SessionState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion RELEASE_NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
99 changes: 99 additions & 0 deletions UnitTests/SessionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
8 changes: 5 additions & 3 deletions UnitTests/SessionTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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());
}
}