Rust: `UnwindSafe` is unergonomic

[dpc]

As I expressed on IRC, I feel like, unless quickly taken care of, UnwindSafe is going to be a "failed feature". I've seen now quite a bit of people that told me that "they just wrap everything in AssertUnwindSafe" defeating the whole purpose of it. I've also seen comments that UnwindSafe is a PITA - sentiment that I'm beginning to share. I am tempted to ignore UnwindSafe completely as well, even though I'd like to do stuff properly. Please hear me out.

UnwindSafe and RefUnwindSafe are not too complicated, but they force people into writing a lot of boilerplate, and the worst part once used ... force that boilerplate on all other users, that might not even know what is it all about.

Static dispatch works OK, since similarly to Send and Sync any struct will get UnwindSafe and RefUnwindSafe if all fields of it satisfy it.

The problem is dynamic dispatch, trait object and trait bounds. Any time someone has to use eg. Box<TraitObject>, even if it's only due to lack of impl Trait on stable, that person most probably should have done Box<Trait + RefUnwindSafe>. Otherwise that Box<Trait> does not satisfy SafeUnwind, even though most probably that would be the intention. After all "This trait is namely not implemented by UnsafeCell, the root of all interior mutability".

And even if that person is aware of "unwind safety", after putting +RefUnwindSafe it won't work because:

only Send/Sync traits can be used as additional traits in a trait object

To get this working a lot of boilerplate needs to be added. Example in my code which is just a PITA. And the worst part: after putting UnwindSafe bound in types used in open-traits (to be implemented by users of a library), now all users have to satisfy that bound. They will have to remember about it, add blanket implementations adding UnwindSafe for every type used as trait object, and get libraries they might want to use it, to do the same...

I don't know. Maybe I'm missing something here, but I feel like at least +UnwindSafe should work just like +Send and +Sync.

Also, I think if the default would be different, there would be almost no problem. Box<Trait> should mean Box<Trait + RefUnwindSafe>, and users should be able to opt-out of it with Box<Trait + !RefUnwindSafe>. This way, unaware uses would get their types UnwindSafe without knowing it.

[nagisa]

It is not recommended to use this function for a general try/catch mechanism.

documentation for catch_unwind

[eddyb]

We should remove that limitation on trait objects, as the current implementation allows any number of OIBITs (aka "auto traits"), but that doesn't mean you should be abusing panic catching machinery.

[SimonSapin]

@nagisa I don’t think your point makes this issue any less valid, unless you mean that catch_unwind should never be used at all. Isn’t it necessary for C->Rust FFI?

[eddyb]

@SimonSapin Hmpf, I would've assumed the typical inputs to FFI would be UnwindSafe and thus not pose any problem, but I could very well be wrong, and I believe it'd be much graver concern than trait objects.

[abonander]

@eddyb I wouldn't expect using trait objects across a catch_unwind() boundary to constitute abuse of panic catching machinery. FFI isn't the only legitimate use of the mechanism.

I don't think the concern with proliferation of catch_unwind() (which is what led to the introduction of UnwindSafe) was well founded to begin with. We don't need to make unsafe-esque APIs deliberately unergonomic to try to compensate for some perceived propensity for laziness; I think the community has done really well at reinforcing good paradigms and correcting bad ones, and I think they could be trusted to treat unwind-safety with care as well.

But in the very least, we could allow UnwindSafe and RefUnwindSafe to be concatenated into trait objects. That, and I think Sync should imply RefUnwindSafe and Send should imply UnwindSafe, because it doesn't make sense why they shouldn't.

[eddyb]

@abonander FWIW, if someone wants to write an RFC for extending the allowed "extra traits" from Send and Sync to any "auto traits", they have my support, and the implementation is just removing the one error.

[dpc]

The problem I described affects people who are not using catch_unwind and might not even know what is it about. As a library I'm trying to make my core type Logger to be as robust as possible - my types require thread-safety so conceptually adding UnwindSafe to Send+Sync seems natural, but the toll will have to be paid be everyone and 99.9% of users don't want to deal with UnwindSafe-related matters.

Also, remember that if it's all too cumbersome to do, then people will just get used to ignoring everything and adding AssertUwindSafe without thinking.

But in the very least, we could allow UnwindSafe and RefUnwindSafe to be concatenated into trait objects. That, and I think Sync should imply RefUnwindSafe and Send should imply UnwindSafe, because it doesn't make sense why they shouldn't.

I'd like that, yes. I still think that unwind-traits should be opt-out (or they should be negative traits in the first place: we have unsafe { ... } blocks because we expect 99.9% of the code to be safe, and as far as I understand we want all types possible to be unwind safe, so we should maybe have RefUnwindUnsafe and UwindUnsafe traits in the first place. But since I completely missed the issue, I might not know about something. Right now it seems to me (maybe I'm very wrong about it), the ergonomy was not a concern to prevent people from abusing catch_unwind and it might backfire by people assuming it's not worth the trouble to get right.

[durka]

@eddyb is it "any auto trait" or "any trait with no methods"? I mean, implementation-wise.

[eddyb]

Right now if you just remove the error, it's specifically "any number of auto traits".

[abonander]

It is not recommended to use this function for a general try/catch mechanism.

It was weird to call it catch_unwind() then, especially given that the concerns in the original stabilization discussion about conflating it with general exception handling seem to have been ignored. Maybe it came up in the triage meeting but that wasn't really elaborated on: https://github.com/rust-lang/rust/issues/27719#issuecomment-199100215

So the blocker on Send and Sync implying UnwindSafe and RefUnwindSafe appears to have been coherence, but I think it's about time we re-addressed this in the context of specialization. Could it be done with default impl since that (presumably) doesn't require any items and allows more-specific impls? Tangentally, a PR to implement default impl has been open since November with no movement: https://github.com/rust-lang/rust/pull/37860

[Diggsey]

For reference, I recently tried to make one of my applications use unwind safe correctly, and out of three types I was storing (a sentry client, an r2d2 connection pool, and a cadence statsd client) all three were missing implementations of RefUnwindSafe.

There were two causes for this:
1) They stored trait objects containing a Sync bound, but not a RefUnwindSafe bound.
2) They used a standard library type which was incorrectly missing a RefUnwindSafe bound.

Both of these causes have a single root cause:

• The Sync marker trait should have RefUnwindSafe as a super trait.

Obviously adding this now would be a problem due to backwards compatibility, but maybe we could introduce some compiler warnings for when Sync is used without RefUnwindSafe? This would solve all of the usability issues I've run into so far.

[eddyb]

@Diggsey I'm not sure auto traits can even have supertraits.
(it should probably be sound if they are themselves auto traits)
cc @nikomatsakis

[gnzlbg]

Obviously adding this now would be a problem due to backwards compatibility, but maybe we could introduce some compiler warnings for when Sync is used without RefUnwindSafe?

We might want to make this change for Rust2018.

---

Also, why are these traits and AssertUnwindSafe not available in core::panic ?

[gnzlbg]

I've seen now quite a bit of people that told me that "they just wrap everything in AssertUnwindSafe" defeating the whole purpose of it.

That does not defeats its purpose. Apparently, that's actually part of its purpose. Think of UnwindSafe as a compiler warning with _a lot_ of false positives, and AssertUnwindSafe as a way to disable the warning. Disabling the warning is safe and always ok, and does not give your program UB. In many cases, e.g., in generic code, the warning is also useless, because without requiring a restrictive + UnwindSafe bounds on closures there is no way for the programmer to tell whether an operation is actually UnwindSafe, so one might just as well ignore the warning.

As I expressed on IRC, I feel like, unless quickly taken care of, UnwindSafe is going to be a "failed feature".

Adding an auto trait to the language to provide a warning with lot's of false positives that is always ok to ignore and for which a lot of code's only option is to actually ignore it, doesn't sound like a nice trade-off to me. So this might already be a failed feature.

[Diggsey]

That does not defeats its purpose. Apparently, that's actually part of its purpose. Think of UnwindSafe as a compiler warning with a lot of false positives, and AssertUnwindSafe as a way to disable the warning.

I think the point is that the compiler should know that things are unwind-safe in more cases so that the assertion is required in fewer cases, and therefore more consideration can be given to whether the code is actually correct in the small number of cases where AssertUnwindSafe is actually required. It's not all that different to safe vs unsafe: the fewer places where unsafe is required, the more valuable it is as a feature.

[gnzlbg]

A big problem is that generic code, taking a Fn, doesn't know if the Fn is UnwindSafe when passing it to catch_unwind. Such code might choose to use an AssertUnwindSafe blindly to avoid all callers from having to write AssertUnwindSafe by themselves.

This happens, for example, with libtest. When you write #[test], your tests are run inside a catch_panic (that's why the test harness doesn't stop when a test panics), but the tests themselves aren't necessarily unwind safe, so libtest needs to blindly AssertUnwindTest to be ergonomic and useful. This isn't a problem, because if using AssertUnwindSafe for a test is incorrect, that test already has a bug.

I think the real problem is that UnwindSafe is just not very useful. It cannot only diagnose the programs with bugs, so it reports a lot of false positives, which results in people using AssertUnwindSafe all over the place, which is always a correct thing to do because when it isn't the reason is that there is a bug somewhere else that has to be fixed anyways.

So I'd rather remove the UnwindSafe trait bound from catch_unwind, and deprecate using the UnwindSafe traits and the AssertUnwindSafe type.

[Diggsey]

but the tests themselves aren't necessarily unwind safe

Just to play devil's advocate for a moment: maybe they should be required to be unwind safe? If one failing test can cascade and cause other tests to fail, then it's harder to see where the problem is.

[gnzlbg]

The only way for the test to fail is if they have unsafe code that's incorrect in the presence of panics, but.... unsafe code must be correct in the presence of panics independently of whether there exists an UnwindSafe trait or not (it's required to, amongst other things, be able to run the tests in multiple threads, etc.).

[Diggsey]

Any kind of shared state between tests that are not unwind-safe could result in a panic from one test causing another test to fail. There's no need for any unsafe code?

[gnzlbg]

Any kind of shared state between tests that are not unwind-safe could result in a panic from one test causing another test to fail. There's no need for any unsafe code?

With unsafe code, UnwindSafe does not protect you from UB.

Without unsafe code, UnwindSafe does not protect you from logic errors either. Notice that #[test]s are just normal functions, which are UnwindSafe, yet you can create a logic error by running one test serially after the other because tests can access shared state via statics and UnwindSafe does not protect you from this (the functions are zero-sized types, they don't own anything).

[Diggsey]

With unsafe code, UnwindSafe does not protect you from UB.

UnwindSafe never protects from UB, it's there to protect against logic errors.

you can create a logic error by running one test serially after the other because tests can access shared state via statics

Static variables must be Sync, and everything Sync should also implement RefUnwindSafe, so all tests should automatically implement UnwindSafe.

[gnzlbg]

Static variables must be Sync, and everything Sync should also implement RefUnwindSafe, so all tests should automatically implement UnwindSafe.

Yet this does not prevent logic errors.

[Diggsey]

Yet this does not prevent logic errors.

Not sure I follow? Everything Sync should have sane behaviour in the event of panics anyway. However, single-threaded code is often not written with panic-safety in mind (because it's assumed the whole thread will panic) and so UnwindSafe exists to tell you when that's a problem: the only way state can cross the catch_unwind boundary is when it's captured by the closure (since the closure takes no arguments). Standalone functions do not capture any state and so should be UnwindSafe automatically.

[gnzlbg]

This safe Rust program has a logic error in the presence of a catch_unwind:

use std::sync::atomic::{AtomicU32, Ordering};

static X: AtomicU32 = AtomicU32::new(42);

fn foo() {
    X.store(13, Ordering::SeqCst);
    panic!("oh no, a panic!");
    X.store(42, Ordering::SeqCst);
}

fn bar() {
    assert_eq!(X.load(Ordering::SeqCst), 42, "logic error");
}

fn main() {
    let _ = std::panic::catch_unwind(|| foo());
    bar();
}

Removing the catch_unwind (e.g. by just calling foo()) removes the logic error (removing the AssertUnwindSafe use in libtest, or adding an UnwindSafe bound to libtest, does not report these logic errors).

UnwindSafe does not diagnose this logic error; no AssertUnwindSafe is necessary.

This is what I mean by "UnwindSafe does not prevent logic errors".

We agree that UnwindSafe does not prevent UB, but the documentation and RFC claim that it prevents logic errors, and AFAICT that's incorrect (if that were true, the example above would be diagnosed somehow).

What UnwindSafe might do is prevent some logic errors, but now we have a quite expensive feature (an auto trait) that does not prevent memory unsafety, does not prevent logic errors, and has a very large false positive rate that make many users rightfully ignore it completely. That's IMO a very bad value proposition: high cost for little reward.

[Diggsey]

Your code has a logic error without catch_unwind though... bar could be called on another thread.

The point is that there are some logic errors that are not possible without catch_unwind, eg. capturing a RefCell and panicking whilst modifying its interior. This is what UnwindSafe exists to prevent.

The reason why this is valuable is because single-threaded code is not written with panic-safety in mind, whereas multithreaded code must already be written with those considerations.

[gnzlbg]

Your code has a logic error without catch_unwind though... bar could be called on another thread.

The Rust Abstract machine guarantees that foo and bar are run sequentially on the same thread of execution in my example above, so I don't see how that could happen.

Unless you mean that, in a different program, that could happen. If that's the case, that's not the program I am talking about. I'm talking about this particular safe Rust single threaded program where catch_unwind introduces a logic bug that wouldn't exist without it, and that UnwindSafe does not catch (you can introduce the same bug with UnsafeCell instead of Atomics if you want to emphatize the "single-thread" constraint).