Skip to content

Fix NullReferenceException in FreezeManager.Tick when server instance… - #993

Open
MhaWay wants to merge 3 commits into
rwmt:devfrom
MhaWay:fix/991-freeze-host-nullref
Open

MhaWay wants to merge 3 commits into
rwmt:devfrom
MhaWay:fix/991-freeze-host-nullref

Conversation

@MhaWay

@MhaWay MhaWay commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

… 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

… 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
@MhaWay

MhaWay commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Needs to be tested proprely with multiple players

@mibac138

Copy link
Copy Markdown
Member

What needs to happen for this issue to trigger? Does it come from the shutdown in ServerBootstrapState? I took a quick look around, and I noticed another use of Server.TryStop in Multiplayer:

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 MultiplayerServer could be made to spawn the thread and hold a handle to it, but you'll need to check if it's a good approach.

@MhaWay

MhaWay commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

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.

MhaWay added 2 commits October 2, 2026 00:19
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.
@MhaWay

MhaWay commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Pushed rework implementing the lifecycle approach: MultiplayerServer now owns its loop thread (StartServer/serverThread) and TryStop joins it (guarded against self-joins, 5s timeout) before tearing down state and nulling instance, so FreezeManager.Tick can't race a null instance anymore. FreezeManager.Tick is back to the upstream host lookup — the null-safe workaround is gone, superseded by the join. The hosted and standalone entry points go through StartServer, and the shutdown regression test is replaced by two lifecycle tests (join is observed; TryStop from the loop thread doesn't deadlock).

Two commits, each green on its own:

  1. Add server loop thread handle so TryStop can wait for it — thread plumbing, null-safe path still intact (159 tests).
  2. Fix #991 via TryStop loop join, superseding the null-safe workaround — the actual swap (160 tests).

In-game validation on a standalone dedicated server with multifaction=true, asyncTime=true, timeControl=EveryoneControls (the #991 repro settings): fresh bootstrap, load, 1+ players joining/leaving, repeated start/stop — no Exception ticking the server in the server log and no NRE in the client Player.log; time control responsive throughout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants