Skip to content
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 by Bors] - Fix PipeSystem panicking with exclusive systems #6698

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Fix PipeSystem panicking with exclusive systems #6698

wants to merge 1 commit into from

Conversation

inodentry
Copy link
Contributor

Without this fix, piped systems containing exclusive systems fail to run, giving a runtime panic.
With this PR, running piped systems that contain exclusive systems now works.

Explanation of the bug

This is because, unless overridden, the default implementation of run from the System trait simply calls run_unsafe. That is not valid for exclusive systems. They must always be called via run, as run_unsafe takes &World instead of &mut World.

Trivial reproduction example:

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_system(exclusive.pipe(another))
        .run();
}

fn exclusive(_world: &mut World) {}
fn another() {}

If you run this, you will get a panic 'Cannot run exclusive systems with a shared World reference' and the backtrace shows how bevy (correctly) tries to call the run method (because the system is exclusive), but it is the implementation from the System trait (because PipeSystem does not have its own), which calls run_unsafe (incorrect):

  • 3: <bevy_ecs::system::system_piping::PipeSystem<SystemA,SystemB> as bevy_ecs::system::system::System>::run_unsafe
  • 4: bevy_ecs::system::system::System::run

@inodentry inodentry added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events P-Crash A sudden unexpected crash labels Nov 20, 2022
@NiklasEi
Copy link
Member

Sorry if this is a dumb question, but does run taking &mut World mean, that now all piped systems are essentially exclusive and will not be parallelized?

@inodentry
Copy link
Contributor Author

inodentry commented Nov 20, 2022

No.

There is the is_exclusive method, whose responsibility is to report to bevy whether the system is exclusive or not. For PipedSystem, it returns true if any of the member systems are exclusive. If none are, it will return false.

That tells bevy how to set up the system descriptors and where to place the system in the schedule.

This PR has no effect on that. It simply makes sure that PipeSystem will behave correctly if it happens to be exclusive.

Exclusive systems and parallel systems are ran in different ways.

Exclusive systems (and parallel systems that the user wants to treat as exclusive, via .at_end() or whatever) will run one-by-one at the designated part of the stage, and they will be called via the run method (giving them &mut World access).

Regular parallel systems will typically be run by the parallel system executor algorithm in the middle of the stage. It will verify the invariants (ensure safe non-overlapping data access) for the system and call it via run_unsafe when it is safe to do so. This method is intended for shared World access, when other systems may be running in parallel, but the safety is the responsibility of the caller.

Regular parallel systems can be called via either run or run_unsafe. The default impl for run in the trait System will simply call run_unsafe (this is always safe, since run has &mut World exclusive access). But run_unsafe will panic if the system is exclusive (hence this PR), because one cannot turn the & back into &mut.

@mockersf mockersf added this to the 0.9.1 milestone Nov 20, 2022
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't block on this, but can you add a test to prevent regressions in the future?

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 21, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Nov 21, 2022
Without this fix, piped systems containing exclusive systems fail to run, giving a runtime panic.
With this PR, running piped systems that contain exclusive systems now works.

## Explanation of the bug

This is because, unless overridden, the default implementation of `run` from the `System` trait simply calls `run_unsafe`. That is not valid for exclusive systems. They must always be called via `run`, as `run_unsafe` takes `&World` instead of `&mut World`.

Trivial reproduction example:
```rust
fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_system(exclusive.pipe(another))
        .run();
}

fn exclusive(_world: &mut World) {}
fn another() {}
```
If you run this, you will get a panic 'Cannot run exclusive systems with a shared World reference' and the backtrace shows how bevy (correctly) tries to call the `run` method (because the system is exclusive), but it is the implementation from the `System` trait (because `PipeSystem` does not have its own), which calls `run_unsafe` (incorrect):
 - 3: <bevy_ecs::system::system_piping::PipeSystem<SystemA,SystemB> as bevy_ecs::system::system::System>::run_unsafe
 - 4: bevy_ecs::system::system::System::run
@bors bors bot changed the title Fix PipeSystem panicking with exclusive systems [Merged by Bors] - Fix PipeSystem panicking with exclusive systems Nov 21, 2022
@bors bors bot closed this Nov 21, 2022
cart pushed a commit that referenced this pull request Nov 30, 2022
Without this fix, piped systems containing exclusive systems fail to run, giving a runtime panic.
With this PR, running piped systems that contain exclusive systems now works.

## Explanation of the bug

This is because, unless overridden, the default implementation of `run` from the `System` trait simply calls `run_unsafe`. That is not valid for exclusive systems. They must always be called via `run`, as `run_unsafe` takes `&World` instead of `&mut World`.

Trivial reproduction example:
```rust
fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_system(exclusive.pipe(another))
        .run();
}

fn exclusive(_world: &mut World) {}
fn another() {}
```
If you run this, you will get a panic 'Cannot run exclusive systems with a shared World reference' and the backtrace shows how bevy (correctly) tries to call the `run` method (because the system is exclusive), but it is the implementation from the `System` trait (because `PipeSystem` does not have its own), which calls `run_unsafe` (incorrect):
 - 3: <bevy_ecs::system::system_piping::PipeSystem<SystemA,SystemB> as bevy_ecs::system::system::System>::run_unsafe
 - 4: bevy_ecs::system::system::System::run
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
Without this fix, piped systems containing exclusive systems fail to run, giving a runtime panic.
With this PR, running piped systems that contain exclusive systems now works.

## Explanation of the bug

This is because, unless overridden, the default implementation of `run` from the `System` trait simply calls `run_unsafe`. That is not valid for exclusive systems. They must always be called via `run`, as `run_unsafe` takes `&World` instead of `&mut World`.

Trivial reproduction example:
```rust
fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_system(exclusive.pipe(another))
        .run();
}

fn exclusive(_world: &mut World) {}
fn another() {}
```
If you run this, you will get a panic 'Cannot run exclusive systems with a shared World reference' and the backtrace shows how bevy (correctly) tries to call the `run` method (because the system is exclusive), but it is the implementation from the `System` trait (because `PipeSystem` does not have its own), which calls `run_unsafe` (incorrect):
 - 3: <bevy_ecs::system::system_piping::PipeSystem<SystemA,SystemB> as bevy_ecs::system::system::System>::run_unsafe
 - 4: bevy_ecs::system::system::System::run
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants