Skip to content

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

Merged
mibac138 merged 4 commits into
rwmt:devfrom
MhaWay:fix/991-freeze-host-nullref
Oct 28, 2026
Merged

mibac138 merged 4 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.

@MhaWay

MhaWay commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

@mibac138 PTAL when you get a chance — the rework per your review suggestion is pushed (ab02727 thread handle + TryStop join, 0ae8304 the actual swap). I can't request a review formally from here, so pinging instead. In-game repro of #991 checks out; test bundles for @stefan307 are attached on the issue.

@mibac138 mibac138 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just added a small nit

Comment thread Source/Common/FreezeManager.cs Outdated
Comment on lines +32 to +42
// 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;
// }
// }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed


public Thread StartServer(string threadName = "Server thread")
{
serverThread = new Thread(Run) { Name = threadName };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be possible to start the same server twice, perhaps check that the serverThread is null before starting a new one?

@MhaWay
MhaWay restored the fix/991-freeze-host-nullref branch October 28, 2026 11:47
@MhaWay MhaWay reopened this Oct 28, 2026
… against double-start

- FreezeManager.Tick: remove the disabled rwmt#991 host-lookup workaround left as a comment, now fully superseded by the TryStop loop join.

- MultiplayerServer.StartServer: throw if serverThread is already set, so the same instance can't spawn two racing loop threads.
@MhaWay
MhaWay requested a review from mibac138 October 28, 2026 13:22
@mibac138
mibac138 merged commit 9df94bd into rwmt:dev Oct 28, 2026
1 check passed
@MhaWay
MhaWay deleted the fix/991-freeze-host-nullref branch October 28, 2026 13:40
SecDefPachiko added a commit to SecDefPachiko/rimworld-multiplayer-fixes that referenced this pull request Oct 30, 2026
- rwmt#980: seed quest name generation so mixed-language players stay in sync
- rwmt#993: TryStop waits for the server loop thread before tearing down
- rwmt#972: join data window colors mod diff status and shows file versions

Protocol bumped to 58: upstream and this fork each moved to 57 for
different packet changes (mod file metadata vs. placement previews).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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