Skip to content

fix(server/state): lock syncedEntities in the client tick loop #4057

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
1fanya wants to merge 1 commit into
base: master
Choose a base branch
from
+42 −17
Open

fix(server/state): lock syncedEntities in the client tick loop #4057

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 42 additions & 17 deletions code/components/citizen-server-impl/src/state/ServerGameState.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,10 @@ ServerGameState::ServerGameState()

fx::sync::SyncEntityPtr ServerGameState::GetEntity(uint8_t playerId, uint16_t objectId)
{
// .size() needs to be read under the lock too - m_entitiesById can be resized by another thread between
// this check and the shared_lock below otherwise.
std::shared_lock _lock(m_entitiesByIdMutex);

if (objectId >= m_entitiesById.size() || objectId < 0)
{
return {};
Expand All @@ -670,7 +674,6 @@ fx::sync::SyncEntityPtr ServerGameState::GetEntity(uint8_t playerId, uint16_t ob
uint16_t objIdAlias = objectId;
debug::Alias(&objIdAlias);

std::shared_lock _lock(m_entitiesByIdMutex);
return m_entitiesById[objectId].lock();
}

Expand Down Expand Up @@ -1444,22 +1447,28 @@ void ServerGameState::Tick(fx::ServerInstanceBase* instance)
const auto& clientRegistry = m_instance->GetComponent<fx::ClientRegistry>();

// if any relevant entities are getting deleted, add them to our removal list and remove them from the relevancy list.
// ReassignEntityInner writes into this same syncedEntities map (under selfMutex, via GetClientData) whenever
// another client's tick task reassigns an entity we're relevant to, so our own walk/erase here needs the
// same lock or the two can hit this eastl::fixed_map from two different TBB threads at once.
auto& syncedEntities = clientDataUnlocked->syncedEntities;
auto& entitiesToDestroy = clientDataUnlocked->entitiesToDestroy;
for (auto entityIt = syncedEntities.begin(), entityEnd = syncedEntities.end(); entityIt != entityEnd;)
{
auto [identPair, syncData] = *entityIt;
auto oldIt = entityIt;
std::unique_lock _(clientDataUnlocked->selfMutex);
for (auto entityIt = syncedEntities.begin(), entityEnd = syncedEntities.end(); entityIt != entityEnd;)
{
auto [identPair, syncData] = *entityIt;
auto oldIt = entityIt;

++entityIt;
++entityIt;

auto& entity = syncData.entity;
auto& entity = syncData.entity;

if (entity->deleting)
{
GS_LOG("deleting [obj:%d:%d] because it's deleting\n", entity->handle, entity->uniqifier);
entitiesToDestroy[identPair] = { entity, { false, false } };
syncedEntities.erase(oldIt);
if (entity->deleting)
{
GS_LOG("deleting [obj:%d:%d] because it's deleting\n", entity->handle, entity->uniqifier);
entitiesToDestroy[identPair] = { entity, { false, false } };
syncedEntities.erase(oldIt);
}
}
}

Expand Down Expand Up @@ -1622,6 +1631,12 @@ void ServerGameState::Tick(fx::ServerInstanceBase* instance)
auto& entity = syncData.entity;
auto& forceUpdate = syncData.forceUpdate;

// same syncedEntities map as the deletion scan above - needs the same selfMutex ReassignEntityInner
// already takes via GetClientData when another client's tick task reaches in. dropped below around
// calls that can loop back into GetClientData(this, client) for this exact client, since selfMutex
// isn't recursive and self-relocking it would deadlock this tick thread against itself.
std::unique_lock selfLock(clientDataUnlocked->selfMutex);

// relevant entity owned by nobody, or wants a reassign? try to yoink it
// (abuse clientMutex for wantsReassign safety)
{
Expand All @@ -1630,13 +1645,23 @@ void ServerGameState::Tick(fx::ServerInstanceBase* instance)
if (!cl || (entity->wantsReassign && cl->GetNetId() != client->GetNetId()))
{
entity->wantsReassign = false;

// 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

@AvarianKnight AvarianKnight Jul 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

@1fanya 1fanya Jul 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

selfLock.lock();
}
}

if (!syncData.hasCreated)
{
bool canCreate = true;

// GetClientData(this, client) a few lines down (BigMode player-scope bookkeeping) targets this
// same client - same self-relock risk as the reassign above.
selfLock.unlock();

if (fx::IsBigMode())
{
if (entity->type == sync::NetObjEntityType::Player)
Expand Down Expand Up @@ -1725,6 +1750,8 @@ void ServerGameState::Tick(fx::ServerInstanceBase* instance)
}
}

selfLock.lock();

if (!canCreate)
{
// darn
Expand Down Expand Up @@ -4186,12 +4213,10 @@ void ServerGameState::SendArrayData(const fx::ClientSharedPtr& client)
{
auto data = GetClientDataUnlocked(this, client);

decltype(m_arrayHandlers)::iterator arrayRef;

{
std::shared_lock s(m_arrayHandlersMutex);
arrayRef = m_arrayHandlers.find(data->routingBucket);
}
// m_arrayHandlers holds unique_ptrs, so we can't copy a handler out and unlock early - keep the shared_lock
// held for as long as we're using arrayRef, otherwise a concurrent erase (2340) can free it out from under us.
std::shared_lock s(m_arrayHandlersMutex);
auto arrayRef = m_arrayHandlers.find(data->routingBucket);

if (arrayRef != m_arrayHandlers.end())
{
Expand Down
Loading