Fix run_system for adapter systems wrapping exclusive systems
#18406
Conversation
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
…usive systems. It was calling `run_unsafe` on the adapter, which called `ExclusiveFunctionSystem::run_unsafe`.
alice-i-cecile
added
C-Bug
alice-i-cecile
approved these changes
Mar 19, 2025
alice-i-cecile
added
D-Straightforward
hymm
reviewed
Mar 19, 2025
| world: &mut World, | ||
| world: UnsafeWorldCell, | ||
| ) -> Self::Out { | ||
| // SAFETY: `is_exclusive` returned true, so the caller ensured we had `&mut World` access |
Contributor
There was a problem hiding this comment.
This safety comment doesn't make sense to me. We never check is_exclusive in this method. My understanding here is that you've passed the safety requirements for calling world_mut to the caller as it can't be verified here. https://docs.rs/bevy/latest/bevy/ecs/world/unsafe_world_cell/struct.UnsafeWorldCell.html#safety-1
And so need to update the safety comment on run_unsafe.
Contributor
Author
There was a problem hiding this comment.
Right, I did update the safety comment on run_unsafe. It now has the additional requirement that
/// - If [`System::is_exclusive`] returns `true`, then it must be valid to call
/// [`UnsafeWorldCell::world_mut`] on `world`.
What I wanted to communicate here is that because ExclusiveFunctionSystem::is_exclusive returns true, that clause triggered and the caller had to ensure that world_mut was valid.
If you have ideas on how to make the wording more clear, I'm happy to update it!
Contributor
Author
There was a problem hiding this comment.
I'll try changing it to
- // SAFETY: `is_exclusive` returned true, so the caller ensured we had `&mut World` access
+ // SAFETY: `Self::is_exclusive` returns true, so the caller is
+ // required to ensure that the it's valid to call `world_mut()`Is that any better?
Contributor
Author
|
Adding to 0.16 because I'm pretty sure this worked in 0.15 and will be a regression if not fixed. |
alice-i-cecile
added
the
P-Regression
hymm
approved these changes
Mar 26, 2025
Contributor
hymm
left a comment
hymm
left a comment
There was a problem hiding this comment.
Just some nits, but the safety comments in multi_threaded should probably be fixed.
alice-i-cecile
added
S-Waiting-on-Author
Member
|
Ping me when the safety comments are improved and I'll merge this in. |
Contributor
Author
|
It should be ready now! (Sorry, I had pushed up the fixes before your message, but I always forget that I'm allowed to click "resolve conversation" myself.) |
chescock
added
S-Ready-For-Final-Review
mockersf
pushed a commit
that referenced
this pull request
Mar 26, 2025
# Objective Fix panic in `run_system` when running an exclusive system wrapped in a `PipeSystem` or `AdapterSystem`. #18076 introduced a `System::run_without_applying_deferred` method. It normally calls `System::run_unsafe`, but `ExclusiveFunctionSystem::run_unsafe` panics, so it was overridden for that type. Unfortunately, `PipeSystem::run_without_applying_deferred` still calls `PipeSystem::run_unsafe`, which can then call `ExclusiveFunctionSystem::run_unsafe` and panic. ## Solution Make `ExclusiveFunctionSystem::run_unsafe` work instead of panicking. Clarify the safety requirements that make this sound. The alternative is to override `run_without_applying_deferred` in `PipeSystem`, `CombinatorSystem`, `AdapterSystem`, `InfallibleSystemWrapper`, and `InfallibleObserverWrapper`. That seems like a lot of extra code just to preserve a confusing special case! Remove some implementations of `System::run` that are no longer necessary with this change. This slightly changes the behavior of `PipeSystem` and `CombinatorSystem`: Currently `run` will call `apply_deferred` on the first system before running the second, but after this change it will only call it after *both* systems have run. The new behavior is consistent with `run_unsafe` and `run_without_applying_deferred`, and restores the behavior prior to #11823. The panic was originally necessary because [`run_unsafe` took `&World`](https://github.com/bevyengine/bevy/pull/6083/files#diff-708dfc60ec5eef432b20a6f471357a7ea9bfb254dc2f918d5ed4a66deb0e85baR90). Now that it takes `UnsafeWorldCell`, it is possible to make it work. See also Cart's concerns at #4166 (comment), although those also predate `UnsafeWorldCell`. And see #6698 for a previous bug caused by this panic.
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 6, 2025
…utor` (#18684) # Objective Simplify code in the `SingleThreadedExecutor` by removing a special case for exclusive systems. The `SingleThreadedExecutor` runs systems without immediately applying deferred buffers. That required calling `run_unsafe()` instead of `run()`, but that would `panic` for exclusive systems, so the code also needed a special case for those. Following #18076 and #18406, we have a `run_without_applying_deferred` method that has the exact behavior we want and works on exclusive systems. ## Solution Replace the code in `SingleThreadedExecutor` that runs systems with a single call to `run_without_applying_deferred()`. Also add this as a wrapper in the `__rust_begin_short_backtrace` module to preserve the special behavior for backtraces.
andrewzhurov
pushed a commit
to andrewzhurov/bevy
that referenced
this pull request
May 17, 2025
…utor` (bevyengine#18684) # Objective Simplify code in the `SingleThreadedExecutor` by removing a special case for exclusive systems. The `SingleThreadedExecutor` runs systems without immediately applying deferred buffers. That required calling `run_unsafe()` instead of `run()`, but that would `panic` for exclusive systems, so the code also needed a special case for those. Following bevyengine#18076 and bevyengine#18406, we have a `run_without_applying_deferred` method that has the exact behavior we want and works on exclusive systems. ## Solution Replace the code in `SingleThreadedExecutor` that runs systems with a single call to `run_without_applying_deferred()`. Also add this as a wrapper in the `__rust_begin_short_backtrace` module to preserve the special behavior for backtraces.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Objective
Fix panic in
run_systemwhen running an exclusive system wrapped in aPipeSystemorAdapterSystem.#18076 introduced a
System::run_without_applying_deferredmethod. It normally callsSystem::run_unsafe, butExclusiveFunctionSystem::run_unsafepanics, so it was overridden for that type. Unfortunately,PipeSystem::run_without_applying_deferredstill callsPipeSystem::run_unsafe, which can then callExclusiveFunctionSystem::run_unsafeand panic.Solution
Make
ExclusiveFunctionSystem::run_unsafework instead of panicking. Clarify the safety requirements that make this sound.The alternative is to override
run_without_applying_deferredinPipeSystem,CombinatorSystem,AdapterSystem,InfallibleSystemWrapper, andInfallibleObserverWrapper. That seems like a lot of extra code just to preserve a confusing special case!Remove some implementations of
System::runthat are no longer necessary with this change. This slightly changes the behavior ofPipeSystemandCombinatorSystem: Currentlyrunwill callapply_deferredon the first system before running the second, but after this change it will only call it after both systems have run. The new behavior is consistent withrun_unsafeandrun_without_applying_deferred, and restores the behavior prior to #11823.The panic was originally necessary because
run_unsafetook&World. Now that it takesUnsafeWorldCell, it is possible to make it work. See also Cart's concerns at #4166 (comment), although those also predateUnsafeWorldCell.And see #6698 for a previous bug caused by this panic.