Can We Make `Connection` Unwind Safe?

[zicklag]

I was trying to protect my app from unexpected crashes by catching panics using catch_unwind, but Connection isn't unwind safe. Do you think it's possible to make unwind safe?

Also, I disabled the default features and haven't enabled runtime-tokio, so I'm not sure why it's pulling in tokio types. Unless some tokio synchronization primitives are still used even when not using the runtime, which could make sense.

error[E0277]: the type `UnsafeCell<AtomicUsize>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   --> crates/matchmaker/src/game_server.rs:17:9
    |
17  |         std::panic::catch_unwind(|| {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^ `UnsafeCell<AtomicUsize>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
    |
    = help: within `quinn::connection::ConnectionInner`, the trait `RefUnwindSafe` is not implemented for `UnsafeCell<AtomicUsize>`
    = note: required because it appears within the type `tokio::loom::std::atomic_usize::AtomicUsize`
    = note: required because it appears within the type `tokio::sync::notify::Notify`
    = note: required because it appears within the type `quinn::connection::Shared`
    = note: required because it appears within the type `quinn::connection::ConnectionInner`
    = note: required because of the requirements on the impl of `UnwindSafe` for `std::sync::Arc<quinn::connection::ConnectionInner>`
    = note: required because it appears within the type `quinn::connection::ConnectionRef`
    = note: required because it appears within the type `quinn::Connection`
    = note: required because of the requirements on the impl of `UnwindSafe` for `Unique<quinn::Connection>`
    = note: required because it appears within the type `alloc::raw_vec::RawVec<quinn::Connection>`
    = note: required because it appears within the type `Vec<quinn::Connection>`
note: required because it's used within this closure
   --> crates/matchmaker/src/game_server.rs:17:34
    |
17  |         std::panic::catch_unwind(|| {
    |                                  ^^
note: required by a bound in `std::panic::catch_unwind`
   --> /home/zicklag/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:136:40
    |
136 | pub fn catch_unwind<F: FnOnce() -> R + UnwindSafe, R>(f: F) -> Result<R> {
    |                                        ^^^^^^^^^^ required by this bound in `std::panic::catch_unwind`

error[E0277]: the type `UnsafeCell<tokio::util::linked_list::LinkedList<tokio::sync::notify::Waiter, tokio::sync::notify::Waiter>>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   --> crates/matchmaker/src/game_server.rs:17:9
    |
17  |         std::panic::catch_unwind(|| {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^ `UnsafeCell<tokio::util::linked_list::LinkedList<tokio::sync::notify::Waiter, tokio::sync::notify::Waiter>>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
    |
    = help: within `quinn::connection::ConnectionInner`, the trait `RefUnwindSafe` is not implemented for `UnsafeCell<tokio::util::linked_list::LinkedList<tokio::sync::notify::Waiter, tokio::sync::notify::Waiter>>`
    = note: required because it appears within the type `lock_api::mutex::Mutex<parking_lot::raw_mutex::RawMutex, tokio::util::linked_list::LinkedList<tokio::sync::notify::Waiter, tokio::sync::notify::Waiter>>`
    = note: required because it appears within the type `tokio::loom::std::parking_lot::Mutex<tokio::util::linked_list::LinkedList<tokio::sync::notify::Waiter, tokio::sync::notify::Waiter>>`
    = note: required because it appears within the type `tokio::sync::notify::Notify`
    = note: required because it appears within the type `quinn::connection::Shared`
    = note: required because it appears within the type `quinn::connection::ConnectionInner`
    = note: required because of the requirements on the impl of `UnwindSafe` for `std::sync::Arc<quinn::connection::ConnectionInner>`
    = note: required because it appears within the type `quinn::connection::ConnectionRef`
    = note: required because it appears within the type `quinn::Connection`
    = note: required because of the requirements on the impl of `UnwindSafe` for `Unique<quinn::Connection>`
    = note: required because it appears within the type `alloc::raw_vec::RawVec<quinn::Connection>`
    = note: required because it appears within the type `Vec<quinn::Connection>`
note: required because it's used within this closure
   --> crates/matchmaker/src/game_server.rs:17:34
    |
17  |         std::panic::catch_unwind(|| {
    |                                  ^^
note: required by a bound in `std::panic::catch_unwind`
   --> /home/zicklag/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:136:40
    |
136 | pub fn catch_unwind<F: FnOnce() -> R + UnwindSafe, R>(f: F) -> Result<R> {
    |                                        ^^^^^^^^^^ required by this bound in `std::panic::catch_unwind`

[djc]

It's especially surprising that loom types appear in this call stack? IIRC you have to opt in to that with RUSTFLAGS?

[zicklag]

Oh, and in case it helps, here's my Cargo.toml dependency. I'm using the master release right now.

quinn = { git = "https://github.com/quinn-rs/quinn.git", default-features = false, features = ["futures-io", "native-certs", "tls-rustls"] }

---

I haven't ( intentionally ) tweaked the RUSTFLAGS at all, and I don't know much about loom.

[Ralith]

Unless some tokio synchronization primitives are still used even when not using the runtime, which could make sense.

This is the case. Notify is a really handy unopinionated primitive. This issue should probably be raised upstream with them.

[zicklag]

Opened an issue in Tokio: https://github.com/tokio-rs/tokio/issues/5122.

[djc]

Going to close this for now, assuming there is no further work on our side.