[Merged by Bors] - Allow access to non-send resource through `World::resource_scope` #6113

Pietrek14 · 2022-09-27T17:34:23Z

Objective

Relaxes the trait bound for World::resource_scope to allow non-send resources. Fixes #6037.

Solution

No big changes in code had to be made. Added a check so that the non-send resources won't be accessed from a different thread.

Changelog

World::resource_scope accepts non-send resources now
World::resource_scope verifies non-send access if the resource is non-send
Two new tests are added, one for valid use of World::resource_scope with a non-send resource, and one for invalid use (calling it from a different thread, resulting in panic)

alice-i-cecile · 2022-09-27T18:08:46Z

crates/bevy_ecs/src/lib.rs

+    #[test]
+    fn non_send_resource_scope() {
+        let mut world = World::default();
+        world.insert_non_send_resource(A(0));


I think we should probably use a different resource type that is deliberately not Send.

alice-i-cecile · 2022-09-27T18:10:53Z

A small nit about the tests, but the logic itself looks sensible to me. Very nice first contribution!

hymm · 2022-09-27T18:18:27Z

crates/bevy_ecs/src/lib.rs

+    }
+
+    #[test]
+    #[should_panic]


This can probably use #[should_panic(expected = "fill this in with some error text")] here.

hymm · 2022-09-27T18:19:02Z

crates/bevy_ecs/src/lib.rs

+                // Should result in a panic
+                world.resource_scope(|world: &mut World, mut value: Mut<A>| {
+                    value.0 += 1;
+                    assert!(!world.contains_resource::<A>());


Do you need this assert here? The test should panic before hitting this line.

hymm · 2022-09-27T18:19:10Z

crates/bevy_ecs/src/lib.rs

+            });
+        });
+
+        assert_eq!(world.get_non_send_resource::<A>().unwrap().0, 1);


Same as above here.

Pietrek14 · 2022-09-27T19:37:22Z

Thanks for the feedback, I think it's fixed now.

hymm · 2022-09-27T19:48:12Z

crates/bevy_ecs/src/lib.rs

+        });
+
+        if let Err(err) = thread.join() {
+            panic!("{}", err.downcast::<String>().unwrap());
+        }


Suggested change

});

if let Err(err) = thread.join() {

panic!("{}", err.downcast::<String>().unwrap());

}

}).join().unwrap();

I tried to do it that way, but then the panic message doesn't match the expected one ("called Result::unwrap() on an Err value: Any { .. }" instead of "attempted to access NonSend resource bevy_ecs::tests::NonSend off of the main thread").

It's sad that that doesn't contain the actual error message. This approach looks fine then. Just made a small suggestion above.

crates/bevy_ecs/src/lib.rs

Co-authored-by: Mike <mike.hsu@gmail.com>

DJMcNab

Seems reasonable to me.

The pr should have a Changelog section I think, but this isn't a breaking change so shouldn't need a migration guide.

DJMcNab · 2022-09-28T09:10:51Z

crates/bevy_ecs/src/lib.rs

@@ -78,6 +79,9 @@ mod tests {
    #[derive(Component, Debug, PartialEq, Eq, Clone, Copy)]
    struct C;

+    #[derive(Default)]
+    struct NonSend(usize, PhantomData<*mut ()>);


It's not ideal that this shadows the name of the 'real' NonSend.

Changed it to NonSendA

DJMcNab · 2022-09-28T09:12:41Z

crates/bevy_ecs/src/world/mod.rs

@@ -1149,14 +1149,27 @@ impl World {
    /// });
    /// assert_eq!(world.get_resource::<A>().unwrap().0, 2);
    /// ```
-    pub fn resource_scope<R: Resource, U>(&mut self, f: impl FnOnce(&mut World, Mut<R>) -> U) -> U {
+    pub fn resource_scope<
+        R: 'static, /* The resource doesn't need to be Send nor Sync. */


Really we should remove the Send/Sync supertraits on resources, because the fact that you can add anything as a non send resource is a bit messy.

But this is fine for now.

I think you should open another issue about it, since it's out of the scope of this PR.

alice-i-cecile

bors r+

) # Objective Relaxes the trait bound for `World::resource_scope` to allow non-send resources. Fixes #6037. ## Solution No big changes in code had to be made. Added a check so that the non-send resources won't be accessed from a different thread. --- ## Changelog - `World::resource_scope` accepts non-send resources now - `World::resource_scope` verifies non-send access if the resource is non-send - Two new tests are added, one for valid use of `World::resource_scope` with a non-send resource, and one for invalid use (calling it from a different thread, resulting in panic) Co-authored-by: Dawid Piotrowski <41804418+Pietrek14@users.noreply.github.com>

bors · 2022-09-28T18:11:13Z

Pull request successfully merged into main.

Build succeeded:

…vyengine#6113) # Objective Relaxes the trait bound for `World::resource_scope` to allow non-send resources. Fixes bevyengine#6037. ## Solution No big changes in code had to be made. Added a check so that the non-send resources won't be accessed from a different thread. --- ## Changelog - `World::resource_scope` accepts non-send resources now - `World::resource_scope` verifies non-send access if the resource is non-send - Two new tests are added, one for valid use of `World::resource_scope` with a non-send resource, and one for invalid use (calling it from a different thread, resulting in panic) Co-authored-by: Dawid Piotrowski <41804418+Pietrek14@users.noreply.github.com>

Pietrek14 and others added 2 commits September 27, 2022 19:12

made resource_scope work for non-send resources

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

71056a3

Merge branch 'bevyengine:main' into main

d607e6b

hymm self-requested a review September 27, 2022 17:38

alice-i-cecile added A-ECS C-Usability labels Sep 27, 2022

alice-i-cecile reviewed Sep 27, 2022

View reviewed changes

alice-i-cecile requested a review from DJMcNab September 27, 2022 18:10

hymm reviewed Sep 27, 2022

View reviewed changes

fixed the non-send resource scope tests

83f2417

hymm approved these changes Sep 27, 2022

View reviewed changes

formatting

6da0e41

hymm reviewed Sep 27, 2022

View reviewed changes

crates/bevy_ecs/src/lib.rs Outdated Show resolved Hide resolved

simplified panic in a non-send resource scope test

3500039

Co-authored-by: Mike <mike.hsu@gmail.com>

DJMcNab approved these changes Sep 28, 2022

View reviewed changes

Pietrek14 and others added 2 commits September 28, 2022 11:22

Merge branch 'bevyengine:main' into main

d823b1b

changed the name of non-send struct used for testing

f68eb33

alice-i-cecile approved these changes Sep 28, 2022

View reviewed changes

hymm added the S-Ready-For-Final-Review label Sep 28, 2022

alice-i-cecile approved these changes Sep 28, 2022

View reviewed changes

bors bot changed the title ~~Allow access to non-send resource through World::resource_scope~~ [Merged by Bors] - Allow access to non-send resource through World::resource_scope Sep 28, 2022

bors bot closed this Sep 28, 2022

james7132 mentioned this pull request Dec 4, 2022

NonSend resources cannot be accessed using World::resource_scope #4159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Allow access to non-send resource through `World::resource_scope` #6113

[Merged by Bors] - Allow access to non-send resource through `World::resource_scope` #6113

Pietrek14 commented Sep 27, 2022 •

edited

Loading

alice-i-cecile Sep 27, 2022

alice-i-cecile commented Sep 27, 2022

hymm Sep 27, 2022

hymm Sep 27, 2022

hymm Sep 27, 2022

Pietrek14 commented Sep 27, 2022

hymm Sep 27, 2022

Pietrek14 Sep 27, 2022

hymm Sep 27, 2022

DJMcNab left a comment

DJMcNab Sep 28, 2022

Pietrek14 Sep 28, 2022

DJMcNab Sep 28, 2022

Pietrek14 Sep 28, 2022

alice-i-cecile left a comment

bors bot commented Sep 28, 2022

[Merged by Bors] - Allow access to non-send resource through World::resource_scope #6113

[Merged by Bors] - Allow access to non-send resource through World::resource_scope #6113

Conversation

Pietrek14 commented Sep 27, 2022 • edited Loading

Objective

Solution

Changelog

Choose a reason for hiding this comment

alice-i-cecile commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pietrek14 commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

bors bot commented Sep 28, 2022

[Merged by Bors] - Allow access to non-send resource through `World::resource_scope` #6113

[Merged by Bors] - Allow access to non-send resource through `World::resource_scope` #6113

Pietrek14 commented Sep 27, 2022 •

edited

Loading