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.

Sign up for GitHub

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

Improve warning for Send resources marked as non_send #8000

Merged
merged 2 commits into from
Apr 17, 2023
Merged

Improve warning for Send resources marked as non_send #8000

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
461d7dd
Improve warning for Send+Sync resources marked as non_send
NiseVoid Mar 9, 2023
38d54f4
Add further clarification about the cause of the warning
NiseVoid Mar 25, 2023
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
8 changes: 6 additions & 2 deletions crates/bevy_ecs/src/storage/resource.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ impl<const SEND: bool> Resources<SEND> {
///
/// # Panics
/// Will panic if `component_id` is not valid for the provided `components`
/// If `SEND` is false, this will panic if `component_id`'s `ComponentInfo` is not registered as being `Send` + `Sync`.
/// If `SEND` is true, this will panic if `component_id`'s `ComponentInfo` is not registered as being `Send` + `Sync`.
pub(crate) fn initialize_with(
&mut self,
component_id: ComponentId,
Expand All @@ -273,7 +273,11 @@ impl<const SEND: bool> Resources<SEND> {
self.resources.get_or_insert_with(component_id, || {
let component_info = components.get_info(component_id).unwrap();
if SEND {
assert!(component_info.is_send_and_sync());
assert!(
component_info.is_send_and_sync(),
"Send + Sync resource {} initialized as non_send",
component_info.name(),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I already asked on Discord but nobody answered: this check only triggers if a resource is first inserted as non-Send and then as Send. If the order is swapped this won't trigger (there's even a test for this). IMO this is inconsistent, it should either always trigger or never.

Copy link
Member

Choose a reason for hiding this comment

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

It's perfectly safe to store a Send type in !Send storage, though probably a mistake to do so. We could fire a warning in the opposite case, but I'm against hard-failing when there's no safety issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's perfectly safe to store a Send type in !Send storage

It's also safe to store a Send type in Send storage though. The problem is that component_info is shared between Send and non-Send resources with the same TypeId. When a resource is inserted as both Send and non-Send then component_info.is_send_and_sync() will report only the value of the first one, which is however wrong for when the resource is first inserted as non-Send. This can be fixed by either:

  • giving two different ComponentIds to Send and non-Send resources with the same TypeId
    • for example this could be done by duplicating Components::resource_indices for non-Send resources
    • in this case the given assertion would just become a sanity check and should never trigger in practice
  • or making it a hard fail
    • this would essentially consist in making this assertion trigger in the opposite direction too, that is when a resource is inserted as Send first and then as non-Send
  • or updating the component_info to set is_send_and_sync when it is false but a Send Resource is being inserted
    • this feels the worst option since it doesn't really fix the issue, it just hides it

At the end I would expect these two functions to either both succeed or both fail (currently only the first one succeeds):

#[derive(Resource)]
struct Foo;
fn send_then_non_send() {
    let mut world = World::new();
    world.insert_resource(Foo);
    world.insert_non_send_resource(Foo);
}
fn non_send_then_send() {
    let mut world = World::new();
    world.insert_non_send_resource(Foo);
    world.insert_resource(Foo);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The case where I ran into this error is much simpler and simply requires insert_non_send_resource, then adding a system that uses it. Doing insert_resource first does prevent the error, which seems a bit odd but that doesn't really cause any usability problems

Copy link
Member

Choose a reason for hiding this comment

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

While this is a pretty big footgun as is, changing the behavior here would be a breaking change, and probably not suited for a PR aiming at 0.10.1. I'm OK with just changing the panic message for now, and have a separate PR resolve this for 0.11.

Hopefully we get !Send resources out of the World before then though.

ResourceData {
column: ManuallyDrop::new(Column::with_capacity(component_info, 1)),
Expand Down