Do not incorrectly `impl Send for World` by DJMcNab · Pull Request #3519 · bevyengine/bevy

DJMcNab · 2022-01-01T23:32:42Z

For actual-pipelining, it might be required to change RenderWorld to contain a Turtle instead, but this version currently works.
I suspect that would also cause mutability problems, and Turtle would need to defer most of the non-non_send methods.

As it happens, those checks would panic at runtime anyway, but we could remove the checks now that this change has happened.

alice-i-cecile · 2022-01-01T23:35:26Z

crates/bevy_ecs/src/world/mod.rs

+/// A world which does not contain any [`!Send`] resources, and therefore
+/// can be safely sent between threads.
+///
+/// The name turtle is derived from this being something which is moving a


🤔 I... don't hate it? Very cute. Not sure it's terribly descriptive.

What about ThreadSafeWorld or something?

It is quite a niche type, to be fair.

I hoped I'd be able to sneak a bit of whimsy through.

(I will claim I didn't just create this PR for this joke; whether you believe that is your choice)

it is turtles all the way down

I have to leave a comment in support of Turtles. It's personal for me, given my avatar.

crates/bevy_ecs/src/world/mod.rs

alice-i-cecile · 2022-01-01T23:37:51Z

crates/bevy_ecs/src/world/mod.rs

+        self.world
+    }
+
+    /// A view on the


Sentence fragment.

alice-i-cecile · 2022-01-01T23:54:08Z

crates/bevy_ecs/src/component.rs

        ComponentId(*index)
    }
+
+    pub(crate) fn non_send_components(&'_ self) -> impl Iterator<Item = ComponentId> + '_ {


Interesting. This will be useful if we want to relax the bounds on Component later too.

alice-i-cecile · 2022-01-01T23:56:40Z

crates/bevy_ecs/src/world/mod.rs

 }

-unsafe impl Send for World {}
 unsafe impl Sync for World {}


Just so I understand: why do we still want the Sync impl for World?

The entire idea of our world abstraction is that it is accessed in a shared way across multiple threads.

alice-i-cecile · 2022-01-02T00:01:13Z

crates/bevy_ecs/src/world/mod.rs

+    ///
+    /// [`!Send`]: Send
+
+    pub fn turtle(self) -> Turtle {


Hmm. I wonder if this method or a From impl is better...

It has the potential to be a relatively expensive operation, so I don't want it to be so implicit.

alice-i-cecile

Correct, well-scoped and important as a prerequisite. None of my musings are blocking, so LGTM.

Cart can decide on the 🐢 name :p

BoxyUwU · 2022-01-08T23:29:48Z

crates/bevy_ecs/src/world/mod.rs

+    /// A view on this [`Turtle`]'s [`World`], which contains no [`!Send`] resources
+    ///
+    /// [`!Send`]: Send
+    // Safety: NonSend resources cannot be added using a raw reference


this is a _really_ sketchy safety comment imo, its relying on things not being possible in the current API which could definitely be added in the future. Do we actually need this function? It doesn't seem like it from the changes in this PR

How would having a raw reference to the world ever allow adding !Send resources.

Also how would you propose we access the world in a scene without this method?

Well resources have no need to fragment archetypes, which is the main problem with &World allowed add/removes on components.

After looking over the code again I realise that you have &Turtle not Turtle so yeah I dunno what to do here but I think this safety comment is not going to end well

Also, "raw" reference feels like poor wording, maybe immutable reference or just reference would work better?

Yeah, not sure why I used the term raw here.

Also, in my view, the main challenge of &World allowing adds and removes is actually synchronisation; currently the synchronisation is outside of the World (for good reason). You seem to be suggesting that there are plans to add the synchronisation back into the World?

alice-i-cecile · 2022-01-13T20:11:07Z

Bumping to Critical, as clippy is now causing CI to fail because this Send impl is unsound.

alice-i-cecile · 2022-01-13T20:12:51Z

bors try

bors · 2022-01-13T20:17:54Z

try

Build failed:

ci

alice-i-cecile · 2022-01-14T06:37:10Z

Back down to High: #3667 is a much lower risk immediate fix.

alice-i-cecile · 2022-02-04T13:45:04Z

Ping @bevyengine/ecs-team; unsoundness is not good.

alice-i-cecile · 2022-04-29T00:12:02Z

@DJMcNab can you rebase?

DJMcNab · 2022-05-23T21:20:34Z

I'm not convinced this is quite the correct changeset. The current impl Send for World is definitely wrong, but I don't think Turtle is unambiguously the correct API here.

Broadly I think we might need to have variants of World, each differently constructed for Send and not Send
I.e. a SendWorld has static requirements that all component types are Send, and crucially a SendWorld cannot be created from an existing World (the main use for this is Assets, just like Turtle in this PR, but it's about constraining the API more correctly).

As it currently stands, this is equivalent to just not supporting the NonSend apis, so the temptation may be to implement World in terms of SendWorld (i.e. literally as a DerefMut); however this falls down once we relax the bounds on Component, which we're likely to do eventually.

I guess we can still call it 🐢.

# Objective - Currently, the `Extract` `RenderStage` is executed on the main world, with the render world available as a resource. - However, when needing access to resources in the render world (e.g. to mutate them), the only way to do so was to get exclusive access to the whole `RenderWorld` resource. - This meant that effectively only one extract which wrote to resources could run at a time. - We didn't previously make `Extract`ing writing to the world a non-happy path, even though we want to discourage that. ## Solution - Move the extract stage to run on the render world. - Add the main world as a `MainWorld` resource. - Add an `Extract` `SystemParam` as a convenience to access a (read only) `SystemParam` in the main world during `Extract`. ## Future work It should be possible to avoid needing to use `get_or_spawn` for the render commands, since now the `Commands`' `Entities` matches up with the world being executed on. We need to determine how this interacts with #3519 It's theoretically possible to remove the need for the `value` method on `Extract`. However, that requires slightly changing the `SystemParam` interface, which would make it more complicated. That would probably mess up the `SystemState` api too. ## Todo I still need to add doc comments to `Extract`. --- ## Changelog ### Changed - The `Extract` `RenderStage` now runs on the render world (instead of the main world as before). You must use the `Extract` `SystemParam` to access the main world during the extract phase. Resources on the render world can now be accessed using `ResMut` during extract. ### Removed - `Commands::spawn_and_forget`. Use `Commands::get_or_spawn(e).insert_bundle(bundle)` instead ## Migration Guide The `Extract` `RenderStage` now runs on the render world (instead of the main world as before). You must use the `Extract` `SystemParam` to access the main world during the extract phase. `Extract` takes a single type parameter, which is any system parameter (such as `Res`, `Query` etc.). It will extract this from the main world, and returns the result of this extraction when `value` is called on it. For example, if previously your extract system looked like: ```rust fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) { for cloud in clouds.iter() { commands.get_or_spawn(cloud).insert(Cloud); } } ``` the new version would be: ```rust fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) { for cloud in clouds.value().iter() { commands.get_or_spawn(cloud).insert(Cloud); } } ``` The diff is: ```diff --- a/src/clouds.rs +++ b/src/clouds.rs @@ -1,5 +1,5 @@ -fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) { - for cloud in clouds.iter() { +fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) { + for cloud in clouds.value().iter() { commands.get_or_spawn(cloud).insert(Cloud); } } ``` You can now also access resources from the render world using the normal system parameters during `Extract`: ```rust fn extract_assets(mut render_assets: ResMut<MyAssets>, source_assets: Extract<Res<MyAssets>>) { *render_assets = source_assets.clone(); } ``` Please note that all existing extract systems need to be updated to match this new style; even if they currently compile they will not run as expected. A warning will be emitted on a best-effort basis if this is not met. Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# Objective - Currently, the `Extract` `RenderStage` is executed on the main world, with the render world available as a resource. - However, when needing access to resources in the render world (e.g. to mutate them), the only way to do so was to get exclusive access to the whole `RenderWorld` resource. - This meant that effectively only one extract which wrote to resources could run at a time. - We didn't previously make `Extract`ing writing to the world a non-happy path, even though we want to discourage that. ## Solution - Move the extract stage to run on the render world. - Add the main world as a `MainWorld` resource. - Add an `Extract` `SystemParam` as a convenience to access a (read only) `SystemParam` in the main world during `Extract`. ## Future work It should be possible to avoid needing to use `get_or_spawn` for the render commands, since now the `Commands`' `Entities` matches up with the world being executed on. We need to determine how this interacts with bevyengine#3519 It's theoretically possible to remove the need for the `value` method on `Extract`. However, that requires slightly changing the `SystemParam` interface, which would make it more complicated. That would probably mess up the `SystemState` api too. ## Todo I still need to add doc comments to `Extract`. --- ## Changelog ### Changed - The `Extract` `RenderStage` now runs on the render world (instead of the main world as before). You must use the `Extract` `SystemParam` to access the main world during the extract phase. Resources on the render world can now be accessed using `ResMut` during extract. ### Removed - `Commands::spawn_and_forget`. Use `Commands::get_or_spawn(e).insert_bundle(bundle)` instead ## Migration Guide The `Extract` `RenderStage` now runs on the render world (instead of the main world as before). You must use the `Extract` `SystemParam` to access the main world during the extract phase. `Extract` takes a single type parameter, which is any system parameter (such as `Res`, `Query` etc.). It will extract this from the main world, and returns the result of this extraction when `value` is called on it. For example, if previously your extract system looked like: ```rust fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) { for cloud in clouds.iter() { commands.get_or_spawn(cloud).insert(Cloud); } } ``` the new version would be: ```rust fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) { for cloud in clouds.value().iter() { commands.get_or_spawn(cloud).insert(Cloud); } } ``` The diff is: ```diff --- a/src/clouds.rs +++ b/src/clouds.rs @@ -1,5 +1,5 @@ -fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) { - for cloud in clouds.iter() { +fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) { + for cloud in clouds.value().iter() { commands.get_or_spawn(cloud).insert(Cloud); } } ``` You can now also access resources from the render world using the normal system parameters during `Extract`: ```rust fn extract_assets(mut render_assets: ResMut<MyAssets>, source_assets: Extract<Res<MyAssets>>) { *render_assets = source_assets.clone(); } ``` Please note that all existing extract systems need to be updated to match this new style; even if they currently compile they will not run as expected. A warning will be emitted on a best-effort basis if this is not met. Co-authored-by: Carter Anderson <mcanders1@gmail.com>

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 1, 2022

DJMcNab added A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention and removed S-Needs-Triage This issue needs to be labelled labels Jan 1, 2022

alice-i-cecile reviewed Jan 1, 2022

View reviewed changes

crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jan 1, 2022

View reviewed changes

crates/bevy_ecs/src/world/mod.rs Outdated

self.world

}

/// A view on the

Copy link

Member

alice-i-cecile Jan 1, 2022

Choose a reason for hiding this comment

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

Sentence fragment.

alice-i-cecile reviewed Jan 1, 2022

View reviewed changes

alice-i-cecile reviewed Jan 2, 2022

View reviewed changes

alice-i-cecile approved these changes Jan 2, 2022

View reviewed changes

BoxyUwU reviewed Jan 8, 2022

View reviewed changes

DJMcNab added 2 commits January 8, 2022 23:59

Prototype world being !Send

769bff9

Add docs and fix inverted condition

22b4033

DJMcNab force-pushed the all-the-way-down branch from bfe9cdb to 22b4033 Compare January 8, 2022 23:59

raw->shared

896dffe

alice-i-cecile added P-Critical This must be fixed immediately or contributors or users will be severely impacted and removed P-High This is particularly urgent, and deserves immediate attention labels Jan 13, 2022

alice-i-cecile requested a review from cart January 13, 2022 20:12

bors bot added a commit that referenced this pull request Jan 13, 2022

Try #3519:

224e429

Appease the 🗞

32e7743

alice-i-cecile mentioned this pull request Jan 13, 2022

Spurious warning in #[allow(clippy::non_send_fields_in_send_ty)] for complex thread-safe wrapper rust-lang/rust-clippy#8275

Open

mockersf mentioned this pull request Jan 13, 2022

new clippy lint: unsound Send implementations #3666

Closed

alice-i-cecile mentioned this pull request Jan 13, 2022

[Merged by Bors] - Ignore clippy 1.58 #3667

Closed

alice-i-cecile added P-High This is particularly urgent, and deserves immediate attention and removed P-Critical This must be fixed immediately or contributors or users will be severely impacted labels Jan 14, 2022

alice-i-cecile mentioned this pull request Feb 18, 2022

NonSend resources off the main thread #3973

Open

DJMcNab mentioned this pull request Apr 3, 2022

[Merged by Bors] - Make RenderStage::Extract run on the render world #4402

Closed

alice-i-cecile added this to the Bevy 0.8 milestone Apr 25, 2022

alice-i-cecile added P-Unsound A bug that results in undefined compiler behavior and removed P-High This is particularly urgent, and deserves immediate attention labels May 5, 2022

alice-i-cecile added the X-Needs-SME This type of work requires an SME to approve it. label May 13, 2022

alice-i-cecile removed this from the Bevy 0.8 milestone Jun 28, 2022

alice-i-cecile mentioned this pull request Jun 29, 2022

Move Non Send Resources into thread locals #5135

Closed

2 tasks

DJMcNab closed this Aug 1, 2022

alice-i-cecile mentioned this pull request Jan 23, 2025

Eliminate NonSend resources by making World optionally !Send using generics #17517

Open

Uh oh!

Conversation

DJMcNab commented Jan 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Jan 13, 2022

Uh oh!

alice-i-cecile commented Jan 13, 2022

Uh oh!

bors bot commented Jan 13, 2022

try

Uh oh!

alice-i-cecile commented Jan 14, 2022

Uh oh!

alice-i-cecile commented Feb 4, 2022

Uh oh!

alice-i-cecile commented Apr 29, 2022

Uh oh!

DJMcNab commented May 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants