Conversation
… 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 rwmt#991
|
Needs to be tested proprely with multiple players |
|
What needs to happen for this issue to trigger? Does it come from the shutdown in LocalServer.running = false;
localServerThread?.Join();
LocalServer.TryStop();
LocalServer = null;The interesting part here, is that the code only calls TryStop after the server thread is finished, avoiding this issue altogether. I think I'd rather have the dedicated server follow a similar pattern rather than using a workaround. I imagine the |
|
Took a closer look at this, here's the situation: Yes, the trigger is the bootstrap shutdown. When bootstrap completes, ServerBootstrapState does Server.running = false; Server.TryStop(); from the packet-handling thread. TryStop() nulls the static instance immediately, while the server thread may still be inside a loop iteration — so the next freezeManager.Tick() sees MultiplayerServer.instance == null through p.IsHost and throws. The exception lands in catch (Exception e) in Run(), the iteration is abandoned before gameTimer advances, and the loop keeps repeating that way — which matches the "simulation never advances" symptom. One nuance worth pointing out before touching the lifecycle: Run() also calls TryStop() itself when it exits the loop (MultiplayerServer.cs ~line 167), i.e. from the server thread. So a naive _serverThread.Join() inside TryStop() would deadlock on that path — the join needs a Thread.CurrentThread != _serverThread guard. The hosted path you quoted is exactly the model: running = false → Join() → TryStop(), and in the standalone entry point the thread is started without holding a handle, so nobody can join it today. I don't have a strong preference — if that's the direction you'd rather see, I'm happy to take this PR in that shape: have MultiplayerServer own the thread and join it from external TryStop() calls (with the self-join guard), then verify everything behaves as expected and demote the null-safe FreezeManager lookup to what's left of a belt-and-braces check. I'll report back once I've confirmed whether that cleanly closes the race — if it does, I'd probably still keep the guard as cheap insurance unless you'd rather drop it entirely. |
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.
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.
|
Pushed rework implementing the lifecycle approach: Two commits, each green on its own:
In-game validation on a standalone dedicated server with |
… 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 #991