From bb01618d35a3e85f8698226a6b0974c23f8e6833 Mon Sep 17 00:00:00 2001 From: MhaWay Date: Tue, 29 Sep 2026 19:35:59 +0200 Subject: [PATCH 1/3] Fix NullReferenceException in FreezeManager.Tick when server instance is null ServerPlayer.IsHost dereferences the static MultiplayerServer.instance via the Server property. During shutdown, TryStop() clears instance on the main thread while the server loop's current iteration may still be inside FreezeManager.Tick evaluating the FirstOrDefault(p => p.IsHost) lambda. The resulting NRE escapes every iteration before TickNet() runs, so gameTimer never advances: clients see an unresponsive time control and the log fills with 'Simulation paused because some players are too far behind'. Resolve the host locally against the captured instance's hostUsername and each player's Username (same identity check, no static lookup), so the tick is crash-safe during the shutdown race. Includes a regression test that nulls the static instance around Tick() and previously failed with the exact stack trace reported in the issue. Fixes https://github.com/rwmt/Multiplayer/issues/991 --- Source/Common/FreezeManager.cs | 21 ++++++++++++++++++++- Source/Tests/FreezeManagerTest.cs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/Source/Common/FreezeManager.cs b/Source/Common/FreezeManager.cs index 8091344c3..7339f22cc 100644 --- a/Source/Common/FreezeManager.cs +++ b/Source/Common/FreezeManager.cs @@ -29,7 +29,26 @@ public FreezeManager(MultiplayerServer server) public void Tick() { - var hostPlayer = Server.PlayingPlayers.FirstOrDefault(p => p.IsHost); + // Note: ServerPlayer.IsHost dereferences MultiplayerServer.instance (static). + // During a server shutdown (TryStop sets instance = null) the server loop's + // current iteration can still execute FreezeManager.Tick and blow up with NRE + // on that accessor before the while(running) guard exits. Resolve "is this the + // host?" locally against our captured instance so the tick stays crash-safe. + // Note: ServerPlayer.IsHost dereferences MultiplayerServer.instance (static). + // During a server shutdown (TryStop sets instance = null) the server loop's + // current iteration can still execute FreezeManager.Tick and blow up with NRE + // on that accessor before the while(running) guard exits. Resolve "is this the + // host?" locally against our captured instance so the tick stays crash-safe. + var hostUsername = Server.hostUsername; + ServerPlayer hostPlayer = null; + foreach (var p in Server.PlayingPlayers) + { + if (hostUsername != null && p.Username == hostUsername) + { + hostPlayer = p; + break; + } + } if (hostPlayer != null) { diff --git a/Source/Tests/FreezeManagerTest.cs b/Source/Tests/FreezeManagerTest.cs index af79c35e8..96a0c3dd5 100644 --- a/Source/Tests/FreezeManagerTest.cs +++ b/Source/Tests/FreezeManagerTest.cs @@ -115,6 +115,37 @@ public void HostAbsent_NoPlayers_NotFrozenStaysNotFrozen() Assert.That(server.freezeManager.Frozen, Is.False); } + [Test] + public void StaticInstanceNull_PlayerPresent_DoesNotThrow() + { + // Reproduces issue #991: during server shutdown TryStop() sets the static + // MultiplayerServer.instance to null while the server thread's current loop + // iteration is still inside FreezeManager.Tick. The old p => p.IsHost lambda + // dereferenced the static instance (Server property), throwing NRE that froze + // the loop. The fix resolves "is this the host?" locally against our captured + // Server field so the tick is crash-safe. + var host = AddPlayer("host", isHost: true); + host.frozen = true; + server.freezeManager.Tick(); + Assert.That(server.freezeManager.Frozen, Is.True); + + var savedInstance = MultiplayerServer.instance; + try + { + MultiplayerServer.instance = null; + + // Before the fix the FirstOrDefault lambda threw NRE inside get_IsHost + // (Server => instance!), aborting FreezeManager.Tick and starving the + // server loop. Now it must run cleanly. + Assert.DoesNotThrow(() => server.freezeManager.Tick(), + "Tick must not throw when the static instance is null (shutdown race)"); + } + finally + { + MultiplayerServer.instance = savedInstance; + } + } + [Test] public void HostReconnects_ResumesNormalBehavior() { From ab02727e2f8d32fa98c133c714e3864c68efd786 Mon Sep 17 00:00:00 2001 From: MhaWay Date: Fri, 2 Oct 2026 00:19:56 +0200 Subject: [PATCH 2/3] Add server loop thread handle so TryStop can wait for it MultiplayerServer.StartServer registers the loop thread, TryStop joins it (with a self-call guard and 5s timeout) before tearing down. Hosted and standalone entry points use StartServer. --- Source/Client/Networking/HostUtil.cs | 6 +----- Source/Common/MultiplayerServer.cs | 23 +++++++++++++++++++++++ Source/Server/Server.cs | 2 +- Source/Tests/ServerTest.cs | 2 ++ 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/Source/Client/Networking/HostUtil.cs b/Source/Client/Networking/HostUtil.cs index 0b736217e..9359d67ac 100644 --- a/Source/Client/Networking/HostUtil.cs +++ b/Source/Client/Networking/HostUtil.cs @@ -201,11 +201,7 @@ private static void StartLocalServer() { Multiplayer.LocalServer.running = true; - Multiplayer.localServerThread = new Thread(Multiplayer.LocalServer.Run) - { - Name = "Local server thread" - }; - Multiplayer.localServerThread.Start(); + Multiplayer.localServerThread = Multiplayer.LocalServer.StartServer("Local server thread"); const string text = "Server started."; Messages.Message(text, MessageTypeDefOf.SilentInput, false); diff --git a/Source/Common/MultiplayerServer.cs b/Source/Common/MultiplayerServer.cs index 7d3940f8b..7edba604c 100644 --- a/Source/Common/MultiplayerServer.cs +++ b/Source/Common/MultiplayerServer.cs @@ -61,6 +61,10 @@ static MultiplayerServer() InitDataState.Requested; public volatile bool running; + public Thread? serverThread; + // Atomic latch: TryStop may be called both from a handler and from Run's + // exit; teardown must run only once. + private int stopFlag; public bool ArbiterPlaying => PlayingPlayers.Any(p => p.IsArbiter && p.status == PlayerStatus.Playing); public ServerPlayer HostPlayer => PlayingPlayers.First(p => p.IsHost); @@ -200,8 +204,27 @@ private void TickNet() serverTimePerTick = StandardTimePerTick * 4f; } + public Thread StartServer(string threadName = "Server thread") + { + serverThread = new Thread(Run) { Name = threadName }; + serverThread.Start(); + return serverThread; + } + + // Waits for the server loop to end before tearing down, so nulling + // instance can't race with Tick (#991). Callers must clear running first. + // Never joins the loop thread itself, which would deadlock Run -> TryStop. public void TryStop() { + if (serverThread is { } thread && thread != Thread.CurrentThread) + { + if (!thread.Join(TimeSpan.FromSeconds(5))) + ServerLog.Error("Server loop thread did not stop within 5 seconds, proceeding with shutdown"); + } + + if (Interlocked.CompareExchange(ref stopFlag, 1, 0) != 0) + return; + ServerLog.Detail("Server shutting down..."); playerManager.OnServerStop(); diff --git a/Source/Server/Server.cs b/Source/Server/Server.cs index 30890e715..dfb0f799d 100644 --- a/Source/Server/Server.cs +++ b/Source/Server/Server.cs @@ -115,7 +115,7 @@ server.netManagers.Add(lan); } -new Thread(server.Run) { Name = "Server thread" }.Start(); +server.StartServer(); while (server.running) { diff --git a/Source/Tests/ServerTest.cs b/Source/Tests/ServerTest.cs index 4cfff8bd5..53974c27a 100644 --- a/Source/Tests/ServerTest.cs +++ b/Source/Tests/ServerTest.cs @@ -171,6 +171,8 @@ private MultiplayerServer MakeServer(out int port) port = liteNet.netManagers[0].manager.LocalPort; var serverThread = new Thread(server.Run) { IsBackground = true }; + // Register the loop thread so TryStop's join contract is exercised here too. + server.serverThread = serverThread; serverThread.Start(); teardownActions.Add(() => From 0ae830401c8e92184ec90f09940c0db80bb5f70e Mon Sep 17 00:00:00 2001 From: MhaWay Date: Fri, 2 Oct 2026 00:20:50 +0200 Subject: [PATCH 3/3] Fix #991 via TryStop loop join, superseding the null-safe workaround FreezeManager.Tick is back to the upstream host lookup: the shutdown race is closed by TryStop waiting for the loop to end before nulling the instance. Replace the static-instance regression test with lifecycle tests for the join and its self-call guard. --- Source/Common/FreezeManager.cs | 32 +++++++---------- Source/Tests/FreezeManagerTest.cs | 60 ++++++++++++++++++------------- 2 files changed, 47 insertions(+), 45 deletions(-) diff --git a/Source/Common/FreezeManager.cs b/Source/Common/FreezeManager.cs index 7339f22cc..044317293 100644 --- a/Source/Common/FreezeManager.cs +++ b/Source/Common/FreezeManager.cs @@ -29,26 +29,18 @@ public FreezeManager(MultiplayerServer server) public void Tick() { - // Note: ServerPlayer.IsHost dereferences MultiplayerServer.instance (static). - // During a server shutdown (TryStop sets instance = null) the server loop's - // current iteration can still execute FreezeManager.Tick and blow up with NRE - // on that accessor before the while(running) guard exits. Resolve "is this the - // host?" locally against our captured instance so the tick stays crash-safe. - // Note: ServerPlayer.IsHost dereferences MultiplayerServer.instance (static). - // During a server shutdown (TryStop sets instance = null) the server loop's - // current iteration can still execute FreezeManager.Tick and blow up with NRE - // on that accessor before the while(running) guard exits. Resolve "is this the - // host?" locally against our captured instance so the tick stays crash-safe. - var hostUsername = Server.hostUsername; - ServerPlayer hostPlayer = null; - foreach (var p in Server.PlayingPlayers) - { - if (hostUsername != null && p.Username == hostUsername) - { - hostPlayer = p; - break; - } - } + // Old fix for #991, disabled — superseded by the loop join in TryStop. + // var hostUsername = Server.hostUsername; + // ServerPlayer hostPlayer = null; + // foreach (var p in Server.PlayingPlayers) + // { + // if (hostUsername != null && p.Username == hostUsername) + // { + // hostPlayer = p; + // break; + // } + // } + var hostPlayer = Server.PlayingPlayers.FirstOrDefault(p => p.IsHost); if (hostPlayer != null) { diff --git a/Source/Tests/FreezeManagerTest.cs b/Source/Tests/FreezeManagerTest.cs index 96a0c3dd5..3697fa81d 100644 --- a/Source/Tests/FreezeManagerTest.cs +++ b/Source/Tests/FreezeManagerTest.cs @@ -1,3 +1,4 @@ +using System.Threading; using Multiplayer.Common; namespace Tests; @@ -116,34 +117,43 @@ public void HostAbsent_NoPlayers_NotFrozenStaysNotFrozen() } [Test] - public void StaticInstanceNull_PlayerPresent_DoesNotThrow() + public void TryStop_JoinsServerLoopThread() { - // Reproduces issue #991: during server shutdown TryStop() sets the static - // MultiplayerServer.instance to null while the server thread's current loop - // iteration is still inside FreezeManager.Tick. The old p => p.IsHost lambda - // dereferenced the static instance (Server property), throwing NRE that froze - // the loop. The fix resolves "is this the host?" locally against our captured - // Server field so the tick is crash-safe. - var host = AddPlayer("host", isHost: true); - host.frozen = true; - server.freezeManager.Tick(); - Assert.That(server.freezeManager.Frozen, Is.True); - - var savedInstance = MultiplayerServer.instance; - try + // The #991 shutdown race is closed by having TryStop wait for the server + // loop to finish before nulling the static instance. A fake loop verifies + // the wait really happens: TryStop must not return before the loop did. + var loopFinished = false; + var fakeLoop = new Thread(() => { - MultiplayerServer.instance = null; - - // Before the fix the FirstOrDefault lambda threw NRE inside get_IsHost - // (Server => instance!), aborting FreezeManager.Tick and starving the - // server loop. Now it must run cleanly. - Assert.DoesNotThrow(() => server.freezeManager.Tick(), - "Tick must not throw when the static instance is null (shutdown race)"); - } - finally + Thread.Sleep(100); + loopFinished = true; + }) { IsBackground = true }; + server.serverThread = fakeLoop; + fakeLoop.Start(); + + server.TryStop(); + + Assert.That(loopFinished, Is.True); + Assert.That(fakeLoop.IsAlive, Is.False); + } + + [Test] + public void TryStop_FromLoopThreadItself_DoesNotDeadlock() + { + // Run() calls TryStop at its own exit; self-joining would hang forever. + var completed = false; + var loopThread = new Thread(() => { - MultiplayerServer.instance = savedInstance; - } + server.serverThread = Thread.CurrentThread; + server.TryStop(); // same thread as the loop we must not join + completed = true; + }) { IsBackground = true }; + loopThread.Start(); + + // If self-join deadlocked, the loop thread would never reach completed=true + // and the join with timeout below would observe an alive thread. + Assert.That(loopThread.Join(TimeSpan.FromSeconds(5)), Is.True); + Assert.That(completed, Is.True); } [Test]