Skip to content

Fix run_system for adapter systems wrapping exclusive systems #18406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/observer/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,7 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
// - `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);
Expand Down
4 changes: 4 additions & 0 deletions crates/bevy_ecs/src/schedule/executor/multi_threaded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -592,6 +594,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`
Expand All @@ -609,6 +612,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(
Expand Down
6 changes: 0 additions & 6 deletions crates/bevy_ecs/src/system/adapter_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
19 changes: 1 addition & 18 deletions crates/bevy_ecs/src/system/combinator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
Expand Down Expand Up @@ -416,11 +404,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);
Expand Down
12 changes: 3 additions & 9 deletions crates/bevy_ecs/src/system/exclusive_function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,18 +110,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();
Expand Down
6 changes: 0 additions & 6 deletions crates/bevy_ecs/src/system/observer_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 0 additions & 6 deletions crates/bevy_ecs/src/system/schedule_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,6 @@ impl<S: System<In = (), Out = ()>> System for InfallibleSystemWrapper<S> {
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);
Expand Down
2 changes: 2 additions & 0 deletions crates/bevy_ecs/src/system/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
9 changes: 9 additions & 0 deletions crates/bevy_ecs/src/system/system_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -894,4 +894,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();
}
}