From 5ded41500631f8a531e3136a63622eb3c3ae4dbf Mon Sep 17 00:00:00 2001 From: Alexey Nikitin Date: Fri, 7 Aug 2026 04:52:29 -0500 Subject: [PATCH] Stop reporting an ordinary disconnect as a send error LiteNetConnection.SendRaw refused to send unless the LiteNetLib peer was Connected, and logged an error otherwise. The peer's transport state changes on LiteNetLib's own thread, and ConnectionBase.Send has already approved the send against the protocol state by the time SendRaw runs, so the two can disagree and no check here can be race-free. LiteNetLib discards sends to a peer that has gone away, silently and without throwing, so the guard suppressed nothing the library would not have suppressed anyway. ServerTest routes ServerLog.Error into Assert.Fail, so that log line failed the suite whenever a client disconnected while the server was still ticking: 7 runs in 1000 idle and 7 in 100 under CPU load, against zero in 1100 runs afterwards. Two tests in the same class also asserted nothing. One waited for Players.Count == 0 before its client had connected, the other for Players.Count == 1 when it had already seeded that player itself; both returned on their first poll and passed whether or not the join happened. They now watch the client arrive and then leave. Making those waits real exposed a further problem: SetImplementation is additive and rejects duplicate handlers, which suits registering each state once at start-up, but the test listener registers per connection with a per-test type, so two tests using different state classes threw and took the test host down. ClearImplementation lets a caller replace a registration; production still registers once and never calls it. The shared wait helper's budget goes from 2000 ms, which was never derived and is thin on slower CI runners, to 5000 ms. The longest wait measured was 757 ms. --- Source/Common/Networking/LiteNetConnection.cs | 12 +- Source/Common/Networking/MpConnectionState.cs | 13 +++ Source/Tests/Helper/TestNetListener.cs | 3 + Source/Tests/ServerTest.cs | 104 +++++++++++++----- 4 files changed, 99 insertions(+), 33 deletions(-) diff --git a/Source/Common/Networking/LiteNetConnection.cs b/Source/Common/Networking/LiteNetConnection.cs index b3a8c5d05..aa9fdc201 100644 --- a/Source/Common/Networking/LiteNetConnection.cs +++ b/Source/Common/Networking/LiteNetConnection.cs @@ -7,12 +7,16 @@ public class LiteNetConnection(NetPeer peer) : ConnectionBase { public readonly NetPeer peer = peer; + /// + /// Sends without first checking the peer's transport state. That state changes on LiteNetLib's + /// own thread, so it can go stale between approving the send + /// against the protocol state and the packet arriving here; a check on this side can never be + /// race-free. LiteNetLib discards sends to a peer that has gone away, silently and without + /// throwing, so there is nothing left for one to do. + /// protected override void SendRaw(byte[] raw, bool reliable) { - if (peer.ConnectionState == ConnectionState.Connected) - peer.Send(raw, reliable ? DeliveryMethod.ReliableOrdered : DeliveryMethod.Unreliable); - else - ServerLog.Error($"SendRaw() called with invalid connection state ({peer}): {peer.ConnectionState}"); + peer.Send(raw, reliable ? DeliveryMethod.ReliableOrdered : DeliveryMethod.Unreliable); } protected override void OnClose(ServerDisconnectPacket? goodbye) diff --git a/Source/Common/Networking/MpConnectionState.cs b/Source/Common/Networking/MpConnectionState.cs index 8551f48eb..ba8a62be7 100644 --- a/Source/Common/Networking/MpConnectionState.cs +++ b/Source/Common/Networking/MpConnectionState.cs @@ -43,6 +43,19 @@ public static ConnectionStateEnum GetStateEnumOf(MpConnectionState state) ? null : (MpConnectionState)Activator.CreateInstance(StateImpls[(int)state], conn); + /// + /// Removes the handlers registered for one state, so a different implementation can take its + /// place. is additive and rejects a second handler for the + /// same packet, which suits registering each state once at start-up but leaves no way to + /// replace one afterwards. + /// + public static void ClearImplementation(ConnectionStateEnum state) + { + StateImpls[(int)state] = null!; + for (var packetId = 0; packetId < packetHandlers.GetLength(1); packetId++) + packetHandlers[(int)state, packetId] = null; + } + public static void SetImplementation(ConnectionStateEnum state, Type type) { if (!type.IsSubclassOf(typeof(MpConnectionState))) return; diff --git a/Source/Tests/Helper/TestNetListener.cs b/Source/Tests/Helper/TestNetListener.cs index 1eebdf695..30c8d790d 100644 --- a/Source/Tests/Helper/TestNetListener.cs +++ b/Source/Tests/Helper/TestNetListener.cs @@ -15,6 +15,9 @@ public void OnPeerConnected(NetPeer peer) conn = new LiteNetConnection(peer); conn.username = "test1"; + // The registry is process-wide and additive, so a registration from an earlier test has to + // be removed before this one can take its place. + MpConnectionState.ClearImplementation(ConnectionStateEnum.ClientJoining); MpConnectionState.SetImplementation(ConnectionStateEnum.ClientJoining, joiningStateType); conn.ChangeState(ConnectionStateEnum.ClientJoining); diff --git a/Source/Tests/ServerTest.cs b/Source/Tests/ServerTest.cs index 4cfff8bd5..a746bf4a4 100644 --- a/Source/Tests/ServerTest.cs +++ b/Source/Tests/ServerTest.cs @@ -31,17 +31,10 @@ public void Test() var server = MakeServer(out var port); ConnectClient(port, typeof(TestJoiningState)); - var timeoutWatch = Stopwatch.StartNew(); - while (true) - { - if (server.InitDataState == InitDataState.Complete && server.playerManager.Players.Count == 0) - break; // Success - - if (timeoutWatch.ElapsedMilliseconds > 2000) - Assert.Fail("Timeout"); - - Thread.Sleep(50); - } + WaitUntil( + nameof(Test), + () => server.InitDataState == InitDataState.Complete && server.playerManager.Players.Count == 0, + () => $"InitDataState={server.InitDataState} Players={server.playerManager.Players.Count}"); } [Test] @@ -83,17 +76,22 @@ public void LoadingStateHandlesKeepAliveWhileWaitingForJoinPoint() ConnectClient(port, typeof(TestLoadingKeepAliveState)); - var timeoutWatch = Stopwatch.StartNew(); - while (true) - { - if (server.playerManager.Players.Count == 0) - break; - - if (timeoutWatch.ElapsedMilliseconds > 2000) - Assert.Fail("Timeout"); - - Thread.Sleep(50); - } + // Players.Count == 0 already holds before the client connects, so waiting on it alone proves + // nothing. Observe the client arrive and then leave: both conditions start out false. + WaitUntil( + $"{nameof(LoadingStateHandlesKeepAliveWhileWaitingForJoinPoint)}.joined", + () => server.playerManager.Players.Count == 1, + () => $"Players={server.playerManager.Players.Count} " + + $"CreatingJoinPoint={server.worldData.CreatingJoinPoint}"); + + WaitUntil( + $"{nameof(LoadingStateHandlesKeepAliveWhileWaitingForJoinPoint)}.left", + () => server.playerManager.Players.Count == 0, + () => $"Players={server.playerManager.Players.Count} " + + $"CreatingJoinPoint={server.worldData.CreatingJoinPoint}"); + + // The loading state was blocked on this join point, so reaching here means it completed. + Assert.That(server.worldData.CreatingJoinPoint, Is.False); } [Test] @@ -110,21 +108,69 @@ public void StandaloneJoinWithExistingPlayer_DoesNotStartJoinPoint() ConnectClient(port, typeof(TestJoiningState)); - var timeoutWatch = Stopwatch.StartNew(); + // The seeded player above already satisfies Players.Count == 1, so waiting on that alone + // would pass without the client ever joining. Wait for it to arrive, then to leave. + WaitUntil( + $"{nameof(StandaloneJoinWithExistingPlayer_DoesNotStartJoinPoint)}.joined", + () => server.playerManager.Players.Count == 2, + () => $"Players={server.playerManager.Players.Count} " + + $"CreatingJoinPoint={server.worldData.CreatingJoinPoint}"); + + WaitUntil( + $"{nameof(StandaloneJoinWithExistingPlayer_DoesNotStartJoinPoint)}.left", + () => server.playerManager.Players.Count == 1, + () => $"Players={server.playerManager.Players.Count} " + + $"CreatingJoinPoint={server.worldData.CreatingJoinPoint}"); + + Assert.That(server.worldData.CreatingJoinPoint, Is.False); + } + + /// + /// Polls until holds, or fails with what was actually observed. + /// + /// These tests drive a real server on a real socket, so how long the condition takes depends on the + /// machine. "Timeout" on its own says only that something did not happen; it does not say which half + /// of a compound condition was still false, nor how close to the budget the run came. Both are needed + /// to choose a defensible budget rather than a superstitious one. + /// + /// Every wait emits one measurement line, on success as well as failure, so a repeated run yields the + /// distribution instead of a single anecdote. Written through TestContext.Progress because NUnit only + /// surfaces captured Console output for tests that fail, and the successful runs are the interesting + /// ones here. + /// + private static void WaitUntil(string label, Func condition, Func describeState, + int timeoutMs = 5000) + { + var watch = Stopwatch.StartNew(); + var polls = 0; + while (true) { - if (server.playerManager.Players.Count == 1) - break; + polls++; + + if (condition()) + { + Report(label, "ok", watch.ElapsedMilliseconds, polls, describeState()); + return; + } - if (timeoutWatch.ElapsedMilliseconds > 2000) - Assert.Fail("Timeout"); + if (watch.ElapsedMilliseconds > timeoutMs) + { + var state = describeState(); + Report(label, "timeout", watch.ElapsedMilliseconds, polls, state); + Assert.Fail($"{label} timed out after {watch.ElapsedMilliseconds} ms " + + $"({polls} polls, budget {timeoutMs} ms); observed {state}"); + } Thread.Sleep(50); } - - Assert.That(server.worldData.CreatingJoinPoint, Is.False); } + /// One greppable line per wait. Prefixed so a harness can pick it out of ordinary test output. + private static void Report(string label, string outcome, long elapsedMs, int polls, string state) => + TestContext.Progress.WriteLine( + $"##WAIT## label={label} outcome={outcome} elapsed_ms={elapsedMs} polls={polls} state=[{state}]"); + private void ConnectClient(int port, Type joiningStateType) { var clientListener = new TestNetListener(joiningStateType);