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

Jump to bottom

insert_resource_by_id doesnt talk about Sendness #6278

Closed
BoxyUwU opened this issue Oct 17, 2022 · 2 comments
Closed

insert_resource_by_id doesnt talk about Sendness #6278

BoxyUwU opened this issue Oct 17, 2022 · 2 comments
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation P-Unsound A bug that results in undefined compiler behavior

Comments

@BoxyUwU
Copy link
Member

BoxyUwU commented Oct 17, 2022

World::insert_resource_by_id doesn't talk talk about whether type implements Send. It should either be part of safety invariants that the type implements Send, or if it implements !Send we're on the main thread. Or alternatively document a panic when called from not main thread with a !Send type (although I don't currently see a call to validate_non_send_access anywhere so I assume this is UB rn but it would be good for someone to check this)
@BoxyUwU BoxyUwU added A-ECS Entities, components, systems, and events S-Needs-Triage This issue needs to be labelled P-Unsound A bug that results in undefined compiler behavior labels Oct 17, 2022
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation and removed S-Needs-Triage This issue needs to be labelled labels Oct 17, 2022
@alice-i-cecile
Copy link
Member

Ping @jakobhellermann.

@jakobhellermann
Copy link
Contributor

Good catch. I would panic on non-main thread non-send insertion, because that matches the behaviour of get_resource_by_id and remove_resource_by_id.

@james7132 james7132 mentioned this issue Dec 3, 2022
Closed
@bors bors bot closed this as completed in aaf384a Jan 9, 2023
james7132 added a commit to james7132/bevy that referenced this issue Jan 21, 2023
@james7132
Panic on dropping NonSend in non-origin thread. (bevyengine#6534)
845b956 
# Objective

Fixes bevyengine#3310. Fixes bevyengine#6282. Fixes bevyengine#6278. Fixes bevyengine#3666.

## Solution
Split out `!Send` resources into `NonSendResources`. Add a `origin_thread_id` to all `!Send` Resources, check it on dropping `NonSendResourceData`, if there's a mismatch, panic. Moved all of the checks that `MainThreadValidator` would do into `NonSendResources` instead.

All `!Send` resources now individually track which thread they were inserted from. This is validated against for every access, mutation, and drop that could be done against the value. 

A regression test using an altered version of the example from bevyengine#3310 has been added.

This is a stopgap solution for the current status quo. A full solution may involve fully removing `!Send` resources/components from `World`, which will likely require a much more thorough design on how to handle the existing in-engine and ecosystem use cases.

This PR also introduces another breaking change:

```rust
    use bevy_ecs::prelude::*;

    #[derive(Resource)]
    struct Resource(u32);

    fn main() {
        let mut world = World::new();
        world.insert_resource(Resource(1));
        world.insert_non_send_resource(Resource(2));
        let res = world.get_resource_mut::<Resource>().unwrap();
        assert_eq!(res.0, 2);
    }
```

This code will run correctly on 0.9.1 but not with this PR, since NonSend resources and normal resources have become actual distinct concepts storage wise.

## Changelog
Changed: Fix soundness bug with `World: Send`. Dropping a `World` that contains a `!Send` resource on the wrong thread will now panic.

## Migration Guide
Normal resources and `NonSend` resources no longer share the same backing storage. If `R: Resource`, then `NonSend<R>` and `Res<R>` will return different instances from each other. If you are using both `Res<T>` and `NonSend<T>` (or their mutable variants), to fetch the same resources, it's strongly advised to use `Res<T>`.
alradish pushed a commit to alradish/bevy that referenced this issue Jan 22, 2023
@james7132 @alradish
Panic on dropping NonSend in non-origin thread. (bevyengine#6534)
9f66db7 
# Objective

Fixes bevyengine#3310. Fixes bevyengine#6282. Fixes bevyengine#6278. Fixes bevyengine#3666.

## Solution
Split out `!Send` resources into `NonSendResources`. Add a `origin_thread_id` to all `!Send` Resources, check it on dropping `NonSendResourceData`, if there's a mismatch, panic. Moved all of the checks that `MainThreadValidator` would do into `NonSendResources` instead.

All `!Send` resources now individually track which thread they were inserted from. This is validated against for every access, mutation, and drop that could be done against the value. 

A regression test using an altered version of the example from bevyengine#3310 has been added.

This is a stopgap solution for the current status quo. A full solution may involve fully removing `!Send` resources/components from `World`, which will likely require a much more thorough design on how to handle the existing in-engine and ecosystem use cases.

This PR also introduces another breaking change:

```rust
    use bevy_ecs::prelude::*;

    #[derive(Resource)]
    struct Resource(u32);

    fn main() {
        let mut world = World::new();
        world.insert_resource(Resource(1));
        world.insert_non_send_resource(Resource(2));
        let res = world.get_resource_mut::<Resource>().unwrap();
        assert_eq!(res.0, 2);
    }
```

This code will run correctly on 0.9.1 but not with this PR, since NonSend resources and normal resources have become actual distinct concepts storage wise.

## Changelog
Changed: Fix soundness bug with `World: Send`. Dropping a `World` that contains a `!Send` resource on the wrong thread will now panic.

## Migration Guide
Normal resources and `NonSend` resources no longer share the same backing storage. If `R: Resource`, then `NonSend<R>` and `Res<R>` will return different instances from each other. If you are using both `Res<T>` and `NonSend<T>` (or their mutable variants), to fetch the same resources, it's strongly advised to use `Res<T>`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@james7132 @ItsDoot
Panic on dropping NonSend in non-origin thread. (bevyengine#6534)
55201ae 
# Objective

Fixes bevyengine#3310. Fixes bevyengine#6282. Fixes bevyengine#6278. Fixes bevyengine#3666.

## Solution
Split out `!Send` resources into `NonSendResources`. Add a `origin_thread_id` to all `!Send` Resources, check it on dropping `NonSendResourceData`, if there's a mismatch, panic. Moved all of the checks that `MainThreadValidator` would do into `NonSendResources` instead.

All `!Send` resources now individually track which thread they were inserted from. This is validated against for every access, mutation, and drop that could be done against the value. 

A regression test using an altered version of the example from bevyengine#3310 has been added.

This is a stopgap solution for the current status quo. A full solution may involve fully removing `!Send` resources/components from `World`, which will likely require a much more thorough design on how to handle the existing in-engine and ecosystem use cases.

This PR also introduces another breaking change:

```rust
    use bevy_ecs::prelude::*;

    #[derive(Resource)]
    struct Resource(u32);

    fn main() {
        let mut world = World::new();
        world.insert_resource(Resource(1));
        world.insert_non_send_resource(Resource(2));
        let res = world.get_resource_mut::<Resource>().unwrap();
        assert_eq!(res.0, 2);
    }
```

This code will run correctly on 0.9.1 but not with this PR, since NonSend resources and normal resources have become actual distinct concepts storage wise.

## Changelog
Changed: Fix soundness bug with `World: Send`. Dropping a `World` that contains a `!Send` resource on the wrong thread will now panic.

## Migration Guide
Normal resources and `NonSend` resources no longer share the same backing storage. If `R: Resource`, then `NonSend<R>` and `Res<R>` will return different instances from each other. If you are using both `Res<T>` and `NonSend<T>` (or their mutable variants), to fetch the same resources, it's strongly advised to use `Res<T>`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation P-Unsound A bug that results in undefined compiler behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Merged by Bors] - Panic on dropping NonSend in non-origin thread. james7132/bevy
3 participants
@alice-i-cecile @BoxyUwU @jakobhellermann