-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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.
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
UnwindSafe
is unergonomic
#40628
Comments
UnwindSafe
has terrible ergonomy.UnwindSafe
has terrible ergonomics
documentation for catch_unwind |
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. |
@nagisa I don’t think your point makes this issue any less valid, unless you mean that |
@SimonSapin Hmpf, I would've assumed the typical inputs to FFI would be |
@eddyb I wouldn't expect using trait objects across a I don't think the concern with proliferation of But in the very least, we could allow |
@abonander FWIW, if someone wants to write an RFC for extending the allowed "extra traits" from |
The problem I described affects people who are not using Also, remember that if it's all too cumbersome to do, then people will just get used to ignoring everything and adding
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 |
@eddyb is it "any auto trait" or "any trait with no methods"? I mean, implementation-wise. |
Right now if you just remove the error, it's specifically "any number of auto traits". |
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: #27719 (comment) So the blocker on |
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:
Both of these causes have a single root cause:
Obviously adding this now would be a problem due to backwards compatibility, but maybe we could introduce some compiler warnings for when |
@Diggsey I'm not sure |
We might want to make this change for Rust2018. Also, why are these traits and |
That does not defeats its purpose. Apparently, that's actually part of its purpose. Think of
Adding an |
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 |
A big problem is that generic code, taking a This happens, for example, with I think the real problem is that So I'd rather remove the |
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. |
The only way for the test to fail is if they have |
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, Without unsafe code, |
UnwindSafe never protects from UB, it's there to protect against logic errors.
Static variables must be |
Yet this does not prevent logic errors. |
Not sure I follow? Everything |
This safe Rust program has a logic error in the presence of a 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
This is what I mean by " We agree that What |
Your code has a logic error without The point is that there are some logic errors that are not possible without 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. |
The Rust Abstract machine guarantees that 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 |
rust-lang/rust#40628, rust-lang/rust#65717 and rust-lang/rfcs#3260 all show that unwind safety isn't particularly ergonomic to use and implement, and ultimately leads to people slapping `AssertUnwindSafe` everywhere until the compiler stops complaining. This situation has led to built-in test framework using `catch_unwind(AssertUnwindSafe(...))` (see https://github.com/rust-lang/rust/blob/1.73.0/library/test/src/lib.rs#L649) and libraries like tower-http doing the same (see https://docs.rs/tower-http/0.4.4/src/tower_http/catch_panic.rs.html#198). As people have mentioned in the threads above, trying to implement this correctly is akin to fighting windmills at the moment. Since the above cases demonstrated that `catch_unwind(AssertUnwindSafe(...))` is currently the easiest way to deal with this situation, this commit does the same and refactors our background job runner code accordingly.
rust-lang/rust#40628, rust-lang/rust#65717 and rust-lang/rfcs#3260 all show that unwind safety isn't particularly ergonomic to use and implement, and ultimately leads to people slapping `AssertUnwindSafe` everywhere until the compiler stops complaining. This situation has led to built-in test framework using `catch_unwind(AssertUnwindSafe(...))` (see https://github.com/rust-lang/rust/blob/1.73.0/library/test/src/lib.rs#L649) and libraries like tower-http doing the same (see https://docs.rs/tower-http/0.4.4/src/tower_http/catch_panic.rs.html#198). As people have mentioned in the threads above, trying to implement this correctly is akin to fighting windmills at the moment. Since the above cases demonstrated that `catch_unwind(AssertUnwindSafe(...))` is currently the easiest way to deal with this situation, this commit does the same and refactors our background job runner code accordingly.
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 inAssertUnwindSafe
" defeating the whole purpose of it. I've also seen comments thatUnwindSafe
is a PITA - sentiment that I'm beginning to share. I am tempted to ignoreUnwindSafe
completely as well, even though I'd like to do stuff properly. Please hear me out.UnwindSafe
andRefUnwindSafe
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
andSync
anystruct
will getUnwindSafe
andRefUnwindSafe
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 ofimpl Trait
on stable, that person most probably should have doneBox<Trait + RefUnwindSafe>
. Otherwise thatBox<Trait>
does not satisfySafeUnwind
, 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: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 addingUnwindSafe
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 meanBox<Trait + RefUnwindSafe>
, and users should be able to opt-out of it withBox<Trait + !RefUnwindSafe>
. This way, unaware uses would get their typesUnwindSafe
without knowing it.The text was updated successfully, but these errors were encountered: