From 5d1fe16bfd7d62a62e390c714940c7b898c5b2e1 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Wed, 26 Mar 2025 09:40:42 -0400 Subject: [PATCH] Fix `run_system` for adapter systems wrapping exclusive systems (#18406) # 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 https://github.com/bevyengine/bevy/pull/4166#discussion_r979140356, although those also predate `UnsafeWorldCell`. And see #6698 for a previous bug caused by this panic. --- crates/bevy_ecs/src/observer/runner.rs | 1 + .../src/schedule/executor/multi_threaded.rs | 4 ++++ crates/bevy_ecs/src/system/adapter_system.rs | 6 ------ crates/bevy_ecs/src/system/combinator.rs | 19 +------------------ .../src/system/exclusive_function_system.rs | 12 +++--------- crates/bevy_ecs/src/system/observer_system.rs | 6 ------ crates/bevy_ecs/src/system/schedule_system.rs | 6 ------ crates/bevy_ecs/src/system/system.rs | 2 ++ crates/bevy_ecs/src/system/system_registry.rs | 9 +++++++++ 9 files changed, 20 insertions(+), 45 deletions(-) diff --git a/crates/bevy_ecs/src/observer/runner.rs b/crates/bevy_ecs/src/observer/runner.rs index c1ea4b2e19..d68c495dab 100644 --- a/crates/bevy_ecs/src/observer/runner.rs +++ b/crates/bevy_ecs/src/observer/runner.rs @@ -402,6 +402,7 @@ fn observer_system_runner>( // - `update_archetype_component_access` is called first // - there are no outstanding references to world except a private component // - system is an `ObserverSystem` so won't mutate world beyond the access of a `DeferredWorld` + // and is never exclusive // - system is the same type erased system from above unsafe { (*system).update_archetype_component_access(world); diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 779686e319..25b3c3a020 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -452,6 +452,8 @@ impl ExecutorState { // SAFETY: // - Caller ensured no other reference to this system exists. + // - `system_task_metadata[system_index].is_exclusive` is `false`, + // so `System::is_exclusive` returned `false` when we called it. // - `can_run` has been called, which calls `update_archetype_component_access` with this system. // - `can_run` returned true, so no systems with conflicting world access are running. unsafe { @@ -608,6 +610,7 @@ impl ExecutorState { /// # Safety /// - Caller must not alias systems that are running. + /// - `is_exclusive` must have returned `false` for the specified system. /// - `world` must have permission to access the world data /// used by the specified system. /// - `update_archetype_component_access` must have been called with `world` @@ -625,6 +628,7 @@ impl ExecutorState { // SAFETY: // - The caller ensures that we have permission to // access the world data used by the system. + // - `is_exclusive` returned false // - `update_archetype_component_access` has been called. unsafe { if let Err(err) = __rust_begin_short_backtrace::run_unsafe( diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs index 4c7261aafd..825389a307 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -162,12 +162,6 @@ where }) } - #[inline] - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut crate::prelude::World) -> Self::Out { - self.func - .adapt(input, |input| self.system.run(input, world)) - } - #[inline] fn apply_deferred(&mut self, world: &mut crate::prelude::World) { self.system.apply_deferred(world); diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 9f91d21eac..a429668b9c 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -176,6 +176,7 @@ where input, // SAFETY: The world accesses for both underlying systems have been registered, // so the caller will guarantee that no other systems will conflict with `a` or `b`. + // If either system has `is_exclusive()`, then the combined system also has `is_exclusive`. // Since these closures are `!Send + !Sync + !'static`, they can never be called // in parallel, so their world accesses will not conflict with each other. // Additionally, `update_archetype_component_access` has been called, @@ -186,19 +187,6 @@ where ) } - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out { - let world = world.as_unsafe_world_cell(); - Func::combine( - input, - // SAFETY: Since these closures are `!Send + !Sync + !'static`, they can never - // be called in parallel. Since mutable access to `world` only exists within - // the scope of either closure, we can be sure they will never alias one another. - |input| self.a.run(input, unsafe { world.world_mut() }), - // SAFETY: See the above safety comment. - |input| self.b.run(input, unsafe { world.world_mut() }), - ) - } - #[inline] fn apply_deferred(&mut self, world: &mut World) { self.a.apply_deferred(world); @@ -419,11 +407,6 @@ where self.b.run_unsafe(value, world) } - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out { - let value = self.a.run(input, world); - self.b.run(value, world) - } - fn apply_deferred(&mut self, world: &mut World) { self.a.apply_deferred(world); self.b.apply_deferred(world); diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 3b07798108..22ce54bb5c 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -112,18 +112,12 @@ where #[inline] unsafe fn run_unsafe( - &mut self, - _input: SystemIn<'_, Self>, - _world: UnsafeWorldCell, - ) -> Self::Out { - panic!("Cannot run exclusive systems with a shared World reference"); - } - - fn run_without_applying_deferred( &mut self, input: SystemIn<'_, Self>, - world: &mut World, + world: UnsafeWorldCell, ) -> Self::Out { + // SAFETY: The safety is upheld by the caller. + let world = unsafe { world.world_mut() }; world.last_change_tick_scope(self.system_meta.last_run, |world| { #[cfg(feature = "trace")] let _span_guard = self.system_meta.system_span.enter(); diff --git a/crates/bevy_ecs/src/system/observer_system.rs b/crates/bevy_ecs/src/system/observer_system.rs index 743b58872b..2cfd99b3f9 100644 --- a/crates/bevy_ecs/src/system/observer_system.rs +++ b/crates/bevy_ecs/src/system/observer_system.rs @@ -138,12 +138,6 @@ where Ok(()) } - #[inline] - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out { - self.observer.run(input, world); - Ok(()) - } - #[inline] fn apply_deferred(&mut self, world: &mut World) { self.observer.apply_deferred(world); diff --git a/crates/bevy_ecs/src/system/schedule_system.rs b/crates/bevy_ecs/src/system/schedule_system.rs index 34d4afae9d..2b82047c7d 100644 --- a/crates/bevy_ecs/src/system/schedule_system.rs +++ b/crates/bevy_ecs/src/system/schedule_system.rs @@ -65,12 +65,6 @@ impl> System for InfallibleSystemWrapper { Ok(()) } - #[inline] - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out { - self.0.run(input, world); - Ok(()) - } - #[inline] fn apply_deferred(&mut self, world: &mut World) { self.0.apply_deferred(world); diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index a12d1988d7..4eff0b85cd 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -69,6 +69,8 @@ pub trait System: Send + Sync + 'static { /// - The caller must ensure that [`world`](UnsafeWorldCell) has permission to access any world data /// registered in `archetype_component_access`. There must be no conflicting /// simultaneous accesses while the system is running. + /// - If [`System::is_exclusive`] returns `true`, then it must be valid to call + /// [`UnsafeWorldCell::world_mut`] on `world`. /// - The method [`System::update_archetype_component_access`] must be called at some /// point before this one, with the same exact [`World`]. If [`System::update_archetype_component_access`] /// panics (or otherwise does not return for any reason), this method must not be called. diff --git a/crates/bevy_ecs/src/system/system_registry.rs b/crates/bevy_ecs/src/system/system_registry.rs index 416bf251f8..56ade62958 100644 --- a/crates/bevy_ecs/src/system/system_registry.rs +++ b/crates/bevy_ecs/src/system/system_registry.rs @@ -896,4 +896,13 @@ mod tests { assert_eq!(INVOCATIONS_LEFT.get(), 0); } + + #[test] + fn run_system_exclusive_adapters() { + let mut world = World::new(); + fn system(_: &mut World) {} + world.run_system_cached(system).unwrap(); + world.run_system_cached(system.pipe(system)).unwrap(); + world.run_system_cached(system.map(|()| {})).unwrap(); + } }