Fix ConcurrentModificationException in netplay delta sync (Tracker delayed props) - #11536
Fix ConcurrentModificationException in netplay delta sync (Tracker delayed props)#11536dschartman wants to merge 1 commit into
Conversation
…layed props) The netplay delta sync reads Tracker's delayed-prop queue via getDelayedPropsFor from outside the game thread (EDT reveal/rollback dialogs, netty reconnect handshake) while the game thread mutates it, killing the host's EDT with a ConcurrentModificationException and leaving a headless host serving remote clients. Synchronize the delayed-prop queue accessors on the queue itself; unfreeze() now drains via a synchronized snapshot-and-clear, then replays outside the lock. Correct the stale single-threaded-access comments in Tracker and DeltaSyncManager, and add a two-thread regression test that reproduced the exception reliably before the fix. Fixes Card-Forge#11535 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByQVCNbGVAtKfZs4pVf4Gr
|
Ugh what? 😭 if EDT can still reach and get blocked by it then we have a far bigger problem imo, trying to synchronize one tracker part will not solve that... |
|
Thanks — helpful context; I'm new to this codebase and didn't know that history. The crash was on 2.0.13, which includes #10693 — so one entry point still slips past it: Agreed the tracker lock doesn't restore the single-threaded design — I'll rework this to fix the entry point instead. Happy to close this in favor of a fresh PR if that's cleaner. |
Summary
Fixes #11535 — while hosting a network game, the host's EDT can die with
java.util.ConcurrentModificationExceptionin the delta-sync path, leaving a headless host with netty still serving the remote clients. The delayed-prop queue inforge.trackable.Trackerwas read by the sync path while the game thread mutated it, with no synchronization.Root cause
Tracker.delayedPropChangesis a plainArrayListwith no synchronization. The game thread mutates it (addDelayedPropChange,clearDelayed, theunfreeze()drain) while the netplay sync reads it throughgetDelayedPropsFor.In the crash, the reader was the EDT: revealing a zone to a remote player dispatches
hideZonesonto the EDT (FThreads.invokeInEdtNowOrLaterinPlayerControllerHuman.reveal), which lands inRemoteClientGuiGame.syncAndSendand walks the tracker viaDeltaSyncManager— racing the game thread. The host log confirms it: DeltaSync INFO lines are interleaved between the stack frames of the exception printout.There was no existing guard to find — the code documented single-threaded assumptions instead of locking ("no locks, snapshots, or volatile barriers needed"), and the crash falsifies them.
Fix
Minimal, at the shared state, following the existing snapshot idiom (cf.
CombatView's synchronized defensive copies):addDelayedPropChange,clearDelayed,getDelayedPropsFor) on the queue itself, so every reader is covered — the checksum path (NetworkChecksumUtil.getEffectiveValue) reads the same queue.unfreeze()now snapshots-and-clears the queue inside the lock and replays outside it — same semantics (queued changes can only be added while frozen, and the drain runs at freeze count 0, so nothing can re-queue mid-replay), but the replay no longer iterates a list any other thread could touch.TrackerandDeltaSyncManagerto state the actual contract.Testing
TrackerConcurrencyTest(forge-game): two threads, one queuing/clearing delayed prop changes inside a freeze bracket, one callinggetDelayedPropsFor. Before the fix it failed with the identical CME every run; after the fix it passes (verified red-then-green, plus repeat runs).NetworkPlayIntegrationTest#testTrueNetworkTraffic(real host/client traffic over the delta-sync pipeline) passes.🤖 Generated with Claude Code — the commit carries a
Co-authored-bytrailer per CONTRIBUTING.md.https://claude.ai/code/session_01ByQVCNbGVAtKfZs4pVf4Gr