Dropping NonSend resources is unsound

[alice-i-cecile]

Bevy version

0.5 (and present main)

What you did

use bevy::prelude::*;
use std::cell::UnsafeCell;
struct MyNonSend(UnsafeCell<()>);

impl Drop for MyNonSend {
    fn drop(&mut self) {
        dbg!("dropped");
    }
}

fn main() {
    let mut world = World::new();

    world.insert_non_send(MyNonSend(UnsafeCell::new(())));

    std::thread::spawn(move || {
        drop(world);
    })
    .join()
    .unwrap();
}

What you expected to happen

The NonSend resource is dropped correctly.

What actually happened

"Dropped" is printed.

Additional information

Full credit to @TheRawMeatball, @jakobhellermann @BoxyUwU and @DJMcNab. Blame them; I'm just the ticketmaster!

[Guvante]

Ideas of how to resolve "you can't print "dropped" from a different thread".

1. Make World non-send
   • Remove unsafe impl Send for World{} to prevent move || { drop(world); } from being valid
   • I feel like it wouldn't be a ticket if this was wanted
   • Never mind my thought that this wouldn't work was correct, I just messed up testing it. World has to be Send.
2. Implement drop to panic if not on the main thread
   • impl Drop for World { fn drop(&mut self) { if !self.main_thread_validator.is_main_thread()) { panic!("attempted to drop World off of the main thread"); } }
   • This feels even worse than making World non-send
3. Implement drop and check for a non-send and if so panic
   • False positives could certainly be a thing here unless mechanisms were specifically added to track dropability
4. Implement drop and mem::forget anything that is non-send
   • Technically correct and safe but not exactly intuitive
5. Move the drop of the things that are non-send to the main thread
   • impl Drop would be required of course
   • Would require a queue of abstract work that is pumped every frame, I know Unity has one but don't know about Bevy.

[DJMcNab]

So importantly, NonSend is not only for the main thread

It can be any thread, so long as it is only 'accessed' from that single thread.

Which makes 1 unviable, since it might be needed that things are added on different threads. 2 would be incorrect by the same token.

5 is not possible in general, and unviable anyway because of the same reason.

[Guvante]

Each World object has a main thread that is set on creation. Sure it doesn't have to be the original thread of the program but it also isn't any thread. There is certainly a main thread for the World.

Allowing N threads to create Worlds and queue destructors would require a mechanism to dequeue on each of those threads but could totally be done. Similarly you could try to queue the destructor and panic if you can not due to there being no queue for the main thread of the World.

Honestly though unless the intention is to be ambiguous it would be nice to clarify if this means mem::forget or not.

[DJMcNab]

Hmm yeah, sorry

I misunderstood how NonSend's validation works.