Fix/thread safety bullet particle - #660
Open
noisethanks wants to merge 2 commits into
Open
Conversation
CBulletManager::Clear() (Level_Bullet_Manager.cpp) took no lock, while UpdateWorkload/Render/AddBullet in the same class all correctly guard m_Bullets with m_Lock. UpdateWorkload runs on GameThread, Render on the main thread - genuine concurrent access, gated on mtBullets (on by default). Added the same m_Lock guard to Clear() for consistency. Also removed a stale DEBUG-only VERIFY(m_thread_id == GetCurrentThreadId()) in AddBullet - it asserts main-thread-only, but AddBullet legitimately runs off-main today (ShootingObject, WeaponKnife, Explosive, Lua's level.add_bullet). The matching assertion in UpdateWorkload was already removed; this restores consistency. CParticleGroup: three sites touch Device.seqParallelBeforRender with no synchronization - SItem::OnFrame (worker thread) and SItem::~SItem (either thread) both push/erase entries, while CRenderDevice::on_idle (main thread) drains the vector every frame. The existing onframe_lock doesn't cover this - it's per-CParticleGroup, so it can't exclude different groups from each other on this shared Device-level vector. Added a new CRenderDevice member, seqParallelBeforRenderCS, guarding all three sites - same pattern already used elsewhere for this exact producer/drainer shape (CEventAPI::CS, CEffect_Rain::rainCS). Tested across multiple sessions including firefights with active particle effects - no crashes, hangs, or behavioral regressions.
The main thread drains Device.seqParallelBeforRender in CRenderDevice::on_idle and invokes every registered callback. Two callbacks are registered across the codebase. PS::CParticleGroup::SItem::DelayDeleteChilds comes from the particle system, and CObjectList::ProcessDestroyQueue is registered whenever mt_Scheduler is set, which is the default. ProcessDestroyQueue destroys queued objects synchronously. That reaches IGame_ObjectPool::destroy and xr_delete, then ~IRenderable and model_Delete. g_bRendering is false during the drain, so CModelPool takes the immediate branch and deletes the visual outright. When that visual is a particle group its destructor clears items, every SItem destructor runs, and each one erased its own entry from Device.seqParallelBeforRender while the drain loop was still iterating over that vector. SItem::Clear does not empty _children_destroy, so the early return at the top of ~SItem does not prevent this. ~SItem now clears its delegate in place rather than erasing it, which leaves the vector size untouched for the duration of the drain. The drain skips cleared entries and clear()s the whole vector when it finishes, so a callback belonging to a destroyed SItem is still never invoked and cancellation keeps working. The drain loop now indexes by position and copies each delegate before invoking it, so an entry appended by a callback is handled safely as well. Also guarded the two CObjectList accesses to this vector with seqParallelBeforRenderCS. Both run on the main thread and neither currently overlaps the drain, so this is consistency with the invariant the previous commit established rather than a fix for a live race.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix unsynchronized concurrent access: bullet manager and particle system
Found while investigating thread safety in the engine's multithreaded
render/game-logic split. Both fixes are self-contained. Neither depends
on or relates to any TBB work.
Also removed a stale DEBUG-only thread assertion in AddBullet. It
claimed AddBullet is main-thread-only. It isn't: several weapon/explosive
paths and a Lua binding call it off-main today. The matching assertion
in UpdateWorkload was already removed. This restores consistency.
Found but not fixed: SItem's destructor has a separate, pre-existing,
same-thread hazard if a deferred callback ever destroys an SItem
mid-drain. Not currently reachable and unrelated to the race above.
Flagging it rather than expanding scope here.
Tested across multiple sessions, including firefights with active
particle effects. No crashes, hangs, or behavior changes observed. This
confirms the new locking doesn't break anything. It doesn't prove the
original race ever caused a specific crash. Both were unsynchronized
shared-state access regardless of whether either was ever observed to
fail.
Edit: Upon closer inspection, it seems that the SItem iterator hazard is reachable. Second commit addresses it.