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

`Sync` should imply `RefUnwindSafe` - lots of missing impls #54768

Open
Diggsey opened this Issue Oct 2, 2018 · 19 comments

Comments

Projects
None yet
5 participants
@Diggsey
Contributor

Diggsey commented Oct 2, 2018

Sync is a strictly stronger requirement on a type than RefUnwindSafe, because any type which can be accessed concurrently by multiple threads, can also be accessed after one of those threads panics.

However, if you compare the list of implementations for Sync:
https://doc.rust-lang.org/std/marker/trait.Sync.html#implementors

And those for RefUnwindSafe:
https://doc.rust-lang.org/std/panic/trait.RefUnwindSafe.html#implementors

There are clearly a lot missing.

Here's my best guess at some missing implementations:

impl RefUnwindSafe for Waker {}
impl RefUnwindSafe for std::sync::Once {}
impl RefUnwindSafe for std::sync::Condvar {}
impl<'a, T: ?Sized + Sync> RefUnwindSafe for MutexGuard<'a, T> {}
impl<'a, T: ?Sized + Sync> RefUnwindSafe for RwLockReadGuard<'a, T> {}
impl<'a, T: ?Sized + Sync> RefUnwindSafe for RwLockWriteGuard<'a, T> {}
impl<T> RefUnwindSafe for JoinHandle<T> {}

It's likely I've missed some where Sync is automatically implemented because of an explicit Sync implementation on a private type which has impacted automatic trait implementation. (Condvar was one of these)

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Oct 2, 2018

Note that some of these implementations (eg. the one on MutexGuard) would be a breaking change because they reduce the number of types implementing RefUnwindSafe.

These types incorrectly (but not unsoundly) implement RefUnwindSafe, as demonstrated here:
https://play.rust-lang.org/?gist=8b62bb5ef5f29e1760958e2429b42c35&version=nightly&mode=debug&edition=2015

This is very similar to issue #41622

I think a better fix for the MutexGuard issue would be to have it contain a PhantomData<&mut T> - that way auto traits should automatically have the desired behaviour without needing to write out all the impls.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Oct 2, 2018

Another side effect of this is that any time you have a Sync bound on a trait object, you should also have a RefUnwindSafe bound. The bound is almost never added, which results in a load of types being incorrectly !RefUnwindSafe, like the sentry::Client, which internally stored a user-provided Box<Fn(X) -> Y + Sync> (note the accidental omission of + RefUnwindSafe).

This contributes a lot to the usability issues with UnwindSafe.

@Centril

This comment has been minimized.

Contributor

Centril commented Oct 3, 2018

@RalfJung

This comment has been minimized.

Member

RalfJung commented Oct 5, 2018

I think a better fix for the MutexGuard issue would be to have it contain a PhantomData<&mut T>

Incidentally, that would also have prevented #41622. So yes please! Don't forget the guards of RwLock and of RefCell.

However, otherwise I am not sure what to add here. @Centril do you have any concrete question? As mentioned above, RefUnwindSafe is not critical for soundness, and I am afraid I don't have a clear enough model of the contract imposed by UnwindSafe to answer the question whether it is indeed implied by Sync.

That said... some things strike me as odd:

impl<'a, T: ?Sized> !UnwindSafe for &'a mut T

If Sync implies RefUnwindSafe, I'd expect by the usual symmetry that Send implies UnwindSafe?

Why is &AtomicUisze: UnwindSafe but &mut AtomicUsize is not? That also seems rather odd.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Oct 5, 2018

I'd expect by the usual symmetry that Send implies UnwindSafe?

I don't think that follows - things with internal mutability can be Send, but are usually not UnwindSafe.

The implications are:

T: Sync <=> &T: Send
T: RefUnwindSafe <=> &T: UnwindSafe
T: Sync => T: RefUnwindSafe
&T: Send => T: RefUnwindSafe
T: Sync => &T: UnwindSafe
&T: Send => &T: UnwindSafe

I don't think this says anything about non-reference types implementing Send or UnwindSafe.

Why is &AtomicUisze: UnwindSafe but &mut AtomicUsize is not? That also seems rather odd.

This is where the "logic" is a bit fuzzy, but I think it's based around expectation: you should expect that you might see intermediate results in an atomic type (since they are usually modified by another thread) whereas when dealing with a single thread, it is more surprising to see your invariants violated (why Cell is not RefUnwindSafe)

For this reason, a mutable reference to an atomic is not unwind safe, because code may change it to point to a completely different atomic variable (temporarily) and not expect the calling code to be able to observe that.

@Centril

This comment has been minimized.

Contributor

Centril commented Oct 6, 2018

@RalfJung I have no questions; just thought you should take a look at it as it is your area of expertise :)

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Oct 7, 2018

Thinking about this a bit more, I think the documentation could use some work too: the documentation indicates this is about poisoning (ie. Mutex is unwind safe because it implements poisoning) but I don't think this is correct.

This would mean that libraries like antidote (which provides poison-free mutexes) could not exist at all: they provide mutable access to any T even after a panic. Even having the antidote types require a RefUnwindSafe bound does not help. because that bound only refers to immutable access, whereas we already have mutable access to the interior.

The only way to have UnwindSafe make sense as a feature, is for it to specifically relate to single-threaded code: we simply cannot provide any guarantees about consistency at a language level for types shared between threads: you should assume that any types providing multi-threaded access (Mutex, RwLock, Atomic, etc) may give you back an inconsistent view if another thread panicked.

The poisoning feature provided by the standard library types is then just an additional safety net those types provide: related to, but separate from UnwindSafe.

By that logic, the set of types which should not be RefUnwindSafe can at most include those which:

  1. are not Sync
  2. and have interior mutability
  3. and do not implement poisoning

Mutex is unwind safe because of 1) so 3) is irrelevant.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Oct 7, 2018

Even having the antidote types require an UnwindSafe bound does not help. because that bound only refers to immutable access, whereas we already have mutable access to the interior.

Wait, RefUnwindSafe is about immutable access, but UnwindSafe is not. Am I missing something?

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Oct 7, 2018

Sorry, that should have read RefUnwindSafe - I've corrected it now. The point is that no bound on antidote::Mutex can be sufficient, because the antidote::MutexGuard gives out &mut T when locked, and &mut T is supposed to be !UnwindSafe regardless of the bounds on T.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Oct 8, 2018

the antidote::MutexGuard gives out &mut T when locked, and &mut T is supposed to be !UnwindSafe regardless of the bounds on T.

That's true, so antidote::Mutex should likely be !UnwindSafe. In sequential code, it's not all that different from a RefCell.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Oct 8, 2018

@RalfJung except that antidote::Mutex is Sync, so you could replace your catch_unwind with a thread or scoped thread, and pass in an Arc<antidote::Mutex>, to get exactly the same behaviour without any unwind safety issues.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Oct 8, 2018

Well, yeah. But this then goes back to "what does UnwindSafe actually want to guarantee".

56quarters added a commit to tshlabs/cadence that referenced this issue Nov 2, 2018

Implement RefUnwindSafe for StatsdClient
Make sure that the StatsdClient is unwind (panic) safe by ensuring
that pointers to sinks and error handlers require the object to be
unwind safe.

Make the QueuingMetricSink unwind safe by not using a CondVar and
Mutex but instead using an AtomicBool to indicate when the worker
is stopped. Additionally, assert that the Crossbeam MsQueue is
unwind safe because it implements Sync.

See rust-lang/rust#54768

Fixes #77

56quarters added a commit to tshlabs/cadence that referenced this issue Nov 4, 2018

Implement RefUnwindSafe for StatsdClient
Make sure that the StatsdClient is unwind (panic) safe by ensuring
that pointers to sinks and error handlers require the object to be
unwind safe.

Make the QueuingMetricSink unwind safe by not using a CondVar and
Mutex but instead using an AtomicBool to indicate when the worker
is stopped. Additionally, assert that the Crossbeam MsQueue is
unwind safe because it implements Sync.

See rust-lang/rust#54768

Fixes #77
@SoniEx2

This comment has been minimized.

SoniEx2 commented Nov 6, 2018

tries to read, fails to read

why's Cell not RefUnwindSafe? it's just like a Mutex but better - Cell works by-move or by-copy, whereas Mutex posions on panic. the by-move or by-copy semantics are roughly equivalent because you can't be in the middle of a computation while also being inside a &Cell.

Mutex: posions because you can be in the middle of a computation while also being inside the Mutex. this could lead to memory unsafety (altho I can't currently come up with any way to cause memory unsafety using e.g. Vec's methods, and if there was such a way I think it'd be considered unsound anyway) if the poisoned lock is accessed.

Cell: requires you to move the object before attempting to mutate it. at that point, an unwind would cause that object to be dropped, thus preventing it from being observed by the Cell.

Cell doesn't need poisoning because it makes stronger guarantees than poisoning.

or am I wrong?

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Nov 6, 2018

@SoniEx2 poisoning/unwind safety are not memory safety concerns - accessing a poisoned mutex cannot cause memory unsafety (and there is a safe API to do this).

Unwind safety is an artificial distinction introduced to separate single-threaded types with interior mutability into two categories:

  1. Those that you should expect to be in a "valid" state after a panic
  2. Those that might be in an "invalid" state after a panic

What is "valid" is entirely up to the user of the type to decide, and also this distinction makes no sense for multi-threaded (Sync) types (if they had two states, they could switch between them at any moment in another thread, effectively making it a single state from the point of view of someone accessing it)

Cell being !UnwindSafe is a choice: you could equally well have an UnwindSafe Cell with otherwise the exact same API. The benefit of being !UnwindSafe, is that when accessing the cell, you don't have to be concerned about panics happening whilst mutating the cell: you can assume that if the caller accesses the cell after you panic, that it is a logic error, and may give incorrect results. (It must still be memory safe though)

On the other hand, with an UnwindSafe cell, you would have to take care when mutating it to leave it in a "valid" state in case of a panic, because UnwindSafe says that you can still access it after a panic and expect to get back "valid" answers (whatever that means in your context).

For multi-threaded types, you always have to leave it in a "valid" state, otherwise accessing any of them might be a logic error if another thread happens to panic: it's just that one of those "valid" states may now be "poisoned", if a multi-threaded type chooses to implement poisoning (but it will still be UnwindSafe either way).

@SoniEx2

This comment has been minimized.

SoniEx2 commented Nov 6, 2018

You can't mutate a Cell, except through replacement, unsafe pointers, or &mut Cell.

Replacement doesn't panic. The panics cannot happen while it's being replaced. So you guarantee a Cell won't have an "invalid" state because any "invalid" state happens outside the Cell.

So Cell should be RefUnwindSafe?

Note that I'm explicitly not talking about RefCell or UnsafeCell.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Nov 6, 2018

The guarantees here have the little to do with the type itself - they are guarantees made by the user of the type to the user of the type.

In the case of Cell, a function could still swap in a temporary value, do some work, and then swap it back, not expecting the intermediate value to exposed. If Cell is !UnwindSafe, then that function would not have to worry about panics while doing the work. If the Cell is UnwindSafe, then that function should use a Drop implementation to swap back the correct value even in the case of a panic.

@SoniEx2

This comment has been minimized.

SoniEx2 commented Nov 6, 2018

And I'm not talking about UnwindSafe either, but about RefUnwindSafe...

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Nov 6, 2018

RefUnwindSafe is just used so that the standard library can provide blanket implementations of UnwindSafe for the reference types? They both relate to the same set of guarantees.

@SoniEx2

This comment has been minimized.

SoniEx2 commented Nov 6, 2018

&mut T isn't UnwindSafe. Rc is. Mutex is.

RefUnwindSafe indicates that being able to mutate a type is unwind-unsafe, but having access to it through & isn't. I think?

Cell should be RefUnwindSafe just like the Atomic*s, because they behave about the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment