fix(server/state): lock syncedEntities in the client tick loop#4057
fix(server/state): lock syncedEntities in the client tick loop#40571fanya wants to merge 1 commit into
Conversation
Tick()'s per-client loop reads and erases GameStateClientData::syncedEntities without ever taking selfMutex, but ReassignEntityInner does take it (via GetClientData) when another client's tick task needs to force an update on an entity we're relevant to. Since parallel_for_each runs one TBB task per client, two threads can hit the same client's syncedEntities map at once, one of them unlocked. This split goes back to 638a776, which moved this notification off a per-entity newSendsMutex and onto a cross-client write straight into the target client's own map, but never added the matching lock on the home side. selfMutex already exists for exactly this and ReassignEntityInner already uses it, the loop just never caught up. The tricky part: GetClientData(this, client) and ReassignEntity can both loop back and try to lock this same client's selfMutex again, through ReassignEntityInner notifying every client relevant to the entity, which includes us. selfMutex isn't recursive, so it gets dropped right before those two calls and picked back up right after, instead of held across the whole loop body. Also tightened two smaller gaps found while going through this function: SendArrayData was releasing m_arrayHandlersMutex before dereferencing the iterator found under it (m_arrayHandlers holds unique_ptrs so there's nothing cheap to copy out first), and GetEntity(uint8_t, uint16_t) was reading m_entitiesById.size() before taking the lock that's supposed to guard the vector.
github-actions
Bot
added
triage
| // ReassignEntity -> ReassignEntityInner notifies every client relevant to this entity, | ||
| // which includes us if we're relevant - that path calls GetClientData(this, client) for us. | ||
| selfLock.unlock(); | ||
| ReassignEntity(entity->handle, client, std::move(_)); // transfer the lock inside |
There was a problem hiding this comment.
I'm a bit confused here, we already lock the entities clientMutex here before calling GetClientUnsafe.
This returns an atomic reference counted weak_lock which we then call lock on to upgrade it into a regular shared_ptr.
There isn't anything unsafe about accessing the syncData here if we're just reading it (but also I don't think there's anything unsafe about it at all).
I'm a bit confused as to why there's locking here at all since there shouldn't be a case where reading/writing for this happens outside of svSync, and tbb::parallel_for_each should completely block the svSync thread so none of the async calls via gscomms_execute_callback_on_sync_thread should be executed until after this call (and vice-versa).
There was a problem hiding this comment.
The svSync thread being blocked isn't the same as everything running on one
thread. parallel_for_each distributes the per-client lambda across the TBB
worker pool, so different clients' iterations run on different threads at
the same time. svSync just waits for all of them to finish, it's not doing
the work itself.
The race isn't svSync vs something async outside it. It's client B's TBB
worker calling ReassignEntity, which calls ReassignEntityInner, which calls
GetClientData(client A), while client A's own TBB worker is walking/erasing
that same syncedEntities map, unlocked, at the same wall clock time. Two
different threads, both live during the same parallel_for_each call.
If there was no real concurrency here, selfMutex around ReassignEntityInner
wouldn't need to exist either, but it's already there, added in 5fa2ae5427
specifically to guard cross-client access to this same map. This PR just
closes the one access point that commit missed.
2 participants
Goal of this PR
GameStateClientData::syncedEntities gets read and erased unlocked in Tick()'s
per-client loop, but ReassignEntityInner takes selfMutex on it when a
different client's tick task forces an update on an entity we're both
relevant to. parallel_for_each runs one TBB task per client, so this is two
threads touching the same eastl::fixed_map, one of them without a lock.
Traced the history on this: 638a776 (2020) replaced an always-locked,
per-entity notification mechanism with a direct write into the target
client's own map, locked through GetClientData. The home side of the loop
kept using the old unlocked convention and nobody went back to fix it up.
5fa2ae5 added selfMutex to two other mutation points in the same function
a month later but missed this one.
How is this PR achieving the goal
Wrapped the loop's direct touches of syncedEntities (the delete-scan pass
and the main sync pass) in selfMutex, same as ReassignEntityInner already
does.
The one thing that made this not a one-line fix: GetClientData(this, client)
and ReassignEntity/ReassignEntityInner can both call back into this exact
client's own selfMutex (ReassignEntityInner notifies every client relevant
to the reassigned entity, and that includes us). selfMutex is a plain
std::mutex, not recursive, so holding it across those calls would deadlock
the tick thread against itself. The lock gets dropped right before both
call sites and reacquired right after, everything else in the loop stays
covered.
While in there I also fixed two smaller, unrelated locking gaps in the same
file that were easy to close without touching anything else:
the lock, then dereferenced the iterator. m_arrayHandlers erases happen
elsewhere under a unique lock, so that's a use-after-free window. Now the
shared_lock just stays held for the lookup and the dereference.
m_entitiesByIdMutex. Moved the whole bounds check under the lock.
This PR applies to the following area(s)
Server (OneSync)
Successfully tested on
Not build/soak tested yet, flagging this explicitly rather than checking
the box below. This needs a real build and some load (ideally something
that exercises entity reassignment, trains/vehicles changing hands a lot)
before merging.
Checklist
Fixes issues
No linked issue, found during a concurrency audit of ServerGameState.cpp
(see docs/threading-map.md in this branch for the full trace).