Skip to content

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Sep 6, 2025

Objective

Across the engine, we're regularly calling mutex.lock().unwrap(), or mutex.lock().unwarp_or_else(PoisonError::into_inner). It's clear that Bevy, as an project cares little about handling the poisoning. It'd be great if that state could be cleaned up.

Partner PR to #20630. Partially addresses #12660.

Solution

Wrap std::sync locks to expose an infallible interface for both Mutex and RwLock. Unlike parking_lot, these wrappers do not panic upon trying to lock a poisoned lock, and it instead just swallows the PoisonError and silence the poison. This mirrors the current heavy use of uwnrap_or_else(PoisonError::into_error). This is a strong assertion that either all code interacting with the Mutex/RwLock either never panics while a guard is held, or that any panic that may occur leaves the data held by the Mutex/RwLock to be never be left in an inconsistent state where logical (not safety) invariants remain unbroken. This doesn't seem to be an issue with any of our current uses.

As a separate cleanup...

  • Both TypeRegistryArc and FunctionRegistryArc have been removed, as users of TypeRegistry and FunctionRegistry can implement their own without too much work
  • Removed the explicit functions wrapping DrawFunctions as, opting for a Deref derive instead.
  • The internal Arc of AppTypeRegistry and AppFunctionRegistry are no longer pub and no longer implements DerefMut as it just wraps an Arc and it makes no sense to implement.
  • SceneLoader just wraps a clone of AppTypeRegistry instead.
  • Use SyncCell instead of Mutex where reasonable.

This does result in bevy_platform::sync deviating from std::sync, so alternatively, we can jump the gun and provide std::sys::nopoison before that becomes stabilized and expose these wrapper separately from the poisoning locks.

Testing

Local CI run.

@james7132 james7132 added C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide A-Cross-Cutting Impacts the entire engine D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Sep 6, 2025
@james7132 james7132 added this to the 0.18 milestone Sep 6, 2025
@BenjaminBrienen BenjaminBrienen added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 6, 2025
@james7132 james7132 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 7, 2025
@james7132 james7132 force-pushed the no-poison branch 2 times, most recently from 458da24 to 5e61c4f Compare September 8, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants