Fix `run_system` for adapter systems wrapping exclusive systems by chescock · Pull Request #18406 · bevyengine/bevy

chescock · 2025-03-19T00:29:46Z

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

…usive systems. It was calling `run_unsafe` on the adapter, which called `ExclusiveFunctionSystem::run_unsafe`.

hymm · 2025-03-19T01:14:47Z

crates/bevy_ecs/src/system/exclusive_function_system.rs

-        world: &mut World,
+        world: UnsafeWorldCell,
    ) -> Self::Out {
+        // SAFETY: `is_exclusive` returned true, so the caller ensured we had `&mut World` access


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.

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!

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?

chescock · 2025-03-24T15:09:06Z

Adding to 0.16 because I'm pretty sure this worked in 0.15 and will be a regression if not fixed.

hymm

Just some nits, but the safety comments in multi_threaded should probably be fixed.

crates/bevy_ecs/src/schedule/executor/multi_threaded.rs

crates/bevy_ecs/src/system/exclusive_function_system.rs

alice-i-cecile · 2025-03-26T04:01:57Z

Ping me when the safety comments are improved and I'll merge this in.

chescock · 2025-03-26T13:24:02Z

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

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

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

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

Fix run_without_applying_deferred for adapter systems wrapping excl…

e9dc5f5

…usive systems. It was calling `run_unsafe` on the adapter, which called `ExclusiveFunctionSystem::run_unsafe`.

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Mar 19, 2025

alice-i-cecile approved these changes Mar 19, 2025

View reviewed changes

alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 19, 2025

hymm reviewed Mar 19, 2025

View reviewed changes

Clarify safety comment.

f524342

chescock added this to the 0.16 milestone Mar 24, 2025

alice-i-cecile added the P-Regression Functionality that used to work but no longer does. Add a test for this! label Mar 24, 2025

hymm approved these changes Mar 26, 2025

View reviewed changes

crates/bevy_ecs/src/schedule/executor/multi_threaded.rs Show resolved Hide resolved

crates/bevy_ecs/src/system/exclusive_function_system.rs Outdated Show resolved Hide resolved

Fix safety comments.

7d70c6a

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 26, 2025

chescock added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 26, 2025

alice-i-cecile added this pull request to the merge queue Mar 26, 2025

Merged via the queue into bevyengine:main with commit 5d1fe16 Mar 26, 2025
34 checks passed

chescock mentioned this pull request Apr 2, 2025

Use new run_without_applying_deferred method in SingleThreadedExecutor #18684

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix `run_system` for adapter systems wrapping exclusive systems#18406

Fix `run_system` for adapter systems wrapping exclusive systems#18406
alice-i-cecile merged 3 commits intobevyengine:mainfrom
chescock:run_without_applying_deferred

chescock commented Mar 19, 2025

Uh oh!

hymm Mar 19, 2025

Uh oh!

chescock Mar 19, 2025

Uh oh!

chescock Mar 19, 2025

Uh oh!

chescock commented Mar 24, 2025

Uh oh!

hymm left a comment

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented Mar 26, 2025

Uh oh!

chescock commented Mar 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

chescock commented Mar 19, 2025

Objective

Solution

Uh oh!

hymm Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

chescock Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

chescock Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

chescock commented Mar 24, 2025

Uh oh!

hymm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented Mar 26, 2025

Uh oh!

chescock commented Mar 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants