tokio-rs / tokio-core

I/O primitives and event loop for async I/O in Rust
Apache License 2.0
634 stars 116 forks source link

Reactor is not RefUnwindSafe #285

Open Marwes opened 6 years ago

Marwes commented 6 years ago

Specifically this is because of the use of a UnsafeCell inside a long list of types so I am not sure which part exactly needs to implement this (guessing it is the Queue that would assert that it is safe?).

error[E0277]: the trait bound `std::cell::UnsafeCell<*mut futures::sync::mpsc::queue::Node<std::option::Option<tokio_core::reactor::Message>>>: std::panic::RefUnwindSafe` is not satisfied in `gluon::Thread`
  --> tests\main.rs:91:9
   |
91 |         tensile::test(name.clone(), move || -> Result<(), String> {
   |         ^^^^^^^^^^^^^ the type std::cell::UnsafeCell<*mut futures::sync::mpsc::queue::Node<std::option::Option<tokio_core::reactor::Message>>> may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   |
   = help: within `gluon::Thread`, the trait `std::panic::RefUnwindSafe` is not implemented for `std::cell::UnsafeCell<*mut futures::sync::mpsc::queue::Node<std::option::Option<tokio_core::reactor::Message>>>`
   = note: required because it appears within the type `futures::sync::mpsc::queue::Queue<std::option::Option<tokio_core::reactor::Message>>`
   = note: required because it appears within the type `futures::sync::mpsc::Inner<tokio_core::reactor::Message>`
   = note: required because it appears within the type `alloc::arc::ArcInner<futures::sync::mpsc::Inner<tokio_core::reactor::Message>>`
   = note: required because it appears within the type `std::marker::PhantomData<alloc::arc::ArcInner<futures::sync::mpsc::Inner<tokio_core::reactor::Message>>>`
   = note: required because it appears within the type `std::ptr::Shared<alloc::arc::ArcInner<futures::sync::mpsc::Inner<tokio_core::reactor::Message>>>`
   = note: required because it appears within the type `std::sync::Arc<futures::sync::mpsc::Inner<tokio_core::reactor::Message>>`
   = note: required because it appears within the type `futures::sync::mpsc::Sender<tokio_core::reactor::Message>`
   = note: required because it appears within the type `futures::sync::mpsc::UnboundedSender<tokio_core::reactor::Message>`
   = note: required because it appears within the type `tokio_core::reactor::Remote`
   = note: required because it appears within the type `std::option::Option<tokio_core::reactor::Remote>`
   = note: required because it appears within the type `gluon::<unnamed>::vm::GlobalVmState`
   = note: required because it appears within the type `alloc::arc::ArcInner<gluon::<unnamed>::vm::GlobalVmState>`
   = note: required because it appears within the type `std::marker::PhantomData<alloc::arc::ArcInner<gluon::<unnamed>::vm::GlobalVmState>>`
   = note: required because it appears within the type `std::ptr::Shared<alloc::arc::ArcInner<gluon::<unnamed>::vm::GlobalVmState>>`
   = note: required because it appears within the type `std::sync::Arc<gluon::<unnamed>::vm::GlobalVmState>`
   = note: required because it appears within the type `gluon::Thread`
   = note: required because of the requirements on the impl of `std::panic::UnwindSafe` for `*const gluon::Thread`
   = note: required because it appears within the type `gluon::<unnamed>::gc::GcPtr<gluon::Thread>`
   = note: required because it appears within the type `gluon::RootedThread`
   = note: required because it appears within the type `[closure@tests\main.rs:91:37: 93:10 vm:gluon::RootedThread, name:std::string::String, filename:std::path::PathBuf]`
   = note: required because of the requirements on the impl of `tensile::Testable` for `[closure@tests\main.rs:91:37: 93:10 vm:gluon::RootedThread, name:std::string::String, filename:std::path::PathBuf]`
   = note: required by `tensile::test`
alexcrichton commented 6 years ago

Thanks for the report! I think though that since aribtrary trait objects are contained, I'm not sure this can be unwind safe? Could you also elaborate a bit on why you'd like the reactor to be unwind safe?

Marwes commented 6 years ago

Chain of consequences here are.

Need to dispatch futures from an interpreter -> I need to hold a Reactor in the interpreter - The test library I use for some tests catches panics and I pass interpreters across this boundary -> Need to make the gluon interpreter RefUnwindSafe (which Remote is the only part that is not)

Nothing I can't work around locally but if it can be fixed upstream then that is obviously preferably (since I think the types here should all be RefUnwindSafe, they just don't have the impl).

alexcrichton commented 6 years ago

Ok thanks for the explanation! My point here though is that the reactor isn't unwind safe as it contains arbitrary trait objects, so I don't actually think we can fix this at the Core layer. That being said I was hoping to be able to help out with the chain of things that led to this, but I'm not so sure now :(

Marwes commented 6 years ago

Ah, didn't get that Remote holds arbitrary trait objects. I suppose that if it were to only hold arbitrary trait objects but not execute any code in them then it could be made RefUnwindSafe, (As it is right now I think it executes arbitrary objects as well).

alexcrichton commented 6 years ago

Oh sorry I thought this was Core not Remote! In that sense I think this should definitely be RefUnwindSafe most likely (although this may best be tested against tokio-rs/tokio perhaps