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/FreezeManager.cs b/Source/Common/FreezeManager.cs index 8091344c3..044317293 100644 --- a/Source/Common/FreezeManager.cs +++ b/Source/Common/FreezeManager.cs @@ -29,6 +29,17 @@ public FreezeManager(MultiplayerServer server) public void Tick() { + // 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/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/FreezeManagerTest.cs b/Source/Tests/FreezeManagerTest.cs index af79c35e8..3697fa81d 100644 --- a/Source/Tests/FreezeManagerTest.cs +++ b/Source/Tests/FreezeManagerTest.cs @@ -1,3 +1,4 @@ +using System.Threading; using Multiplayer.Common; namespace Tests; @@ -115,6 +116,46 @@ public void HostAbsent_NoPlayers_NotFrozenStaysNotFrozen() Assert.That(server.freezeManager.Frozen, Is.False); } + [Test] + public void TryStop_JoinsServerLoopThread() + { + // 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(() => + { + 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(() => + { + 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] public void HostReconnects_ResumesNormalBehavior() { 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(() =>