Improve safety

[joshlf]

Currently, this API aims to make deadlocks difficult to produce, but it does not make them impossible. This issue describes known holes which safe code can fall into, and proposes API changes to fix them.

There's a philosophical conversation to be had about whether things that are not memory safety properties should be considered "safe" vs unsafe. I'll leave that discussion for another issue/thread - for the context of this issue, let's assume that we're taking the stance that this crate should try to provide an API that is deadlock-proof rather than deadlock-resistant so long as the user does not rely on unsafe code.

Multiple LockedAts per thread

Since LockedAt::new is safe to call and performs no runtime or type-level validation, safe code can construct multiple LockedAts per thread, and then use them to violate deadlock safety.

Proposed solutions

IMO it would make sense to support at least the first two of these, and optionally the third.

Unsafe constructor

Rename new to new_unchecked and make it unsafe to call. The safety precondition is that the caller promises to only ever construct one LockedAt per thread. I've spoken to an internal Google customer who said that they'd prefer this option over the following ones, and that they don't mind the minor increase in verbosity. Even for customers who still prefer to construct their own LockedAts, this has the advantage of sign-posting to readers that there's an obligation on the user here.

thread::spawn wrapper

The standard library thread::spawn function currently has (approximately) this signature:

fn spawn<F: FnOnce()>(f: F)

This crate could define a wrapper around spawn which constructs and injects a new LockedAt:

fn spawn<F: FnOnce(LockedAt)>(f: F)

In combination with making the constructor unsafe, this would ensure that the only way to safely acquire a new LockedAt ensures that there's only ever one per thread.

Checked constructor

Add a new constructor which uses thread-local storage to record when a new LockedAt is created, and panics if the user attempts to create two on a single thread. This would require the spawn solution to also record LockedAt creation in thread-local storage, but it would not require the unsafe constructor to do so since the unsafe constructor's caller must already promise to avoid creating multiple LockedAts.

Forgetting guards

The LockedAt::lock method takes a &mut self and returns a guard with the same lifetime as self. This has the effect of making self unusable so long as the guard exists, which is designed to prevent deadlocks. In particular, the guard must be dropped, which has the effect of unlocking the lock, before self is usable again.

However, a user can still mem::forget the guard. This has the effect of allowing its lifetime to end without dropping it, and thus without unlocking the lock.

Proposed solutions

This one is harder to solve ergonomically, and I don't love any of these proposed solutions. This really needs linear types for a good solution, but maybe there's a better ergonomic solution that works with Rust today that I haven't thought of.

Callback-based locking API

There are two variants of this solution. The first, and easiest, is to change lock to have (approximately) the following signature:

fn lock<'a, NewLock: LockAfter<L> + 'a + LockLevel<Method = MutualExclusion>, T: MutexLock, F: FnOnce(&mut T::Guard<'a>) -> O, O>(
    &'a mut self,
    t: &'a T,
    f: F,
) -> Result<O, T::Error<'a>>

This modified lock locks the mutex internally, and then passes a &mut reference to the resulting guard to the callback, f. Since the guard is not passed by value, lock can guarantee that it is not forgotten, and can take responsibility for dropping it after f returns (or if f panics).

The second, and somewhat more annoying solution, is only relevant if for some reason having the guard by value is important. In this case, lock must force f to return the guard by value so that lock can drop it. lock needs some way of ensuring that the same guard is returned that was passed to f in the first place, and not merely some guard with the same type. For this, it can use the invariant lifetimes trick.

Force guard return

I'm not even sure if this one is possible to express in Rust, but I figured I'd write it down.

lock should consume self by value. When it returns, self is gone, but it has returned a guard in its place. In order to obtain a new LockedAt, the caller must call a new LockedAt::from_guard constructor. This ensures that the guard can't have been forgotten in the meantime.

This solution has the advantage of being significantly more ergonomic than the callback-based solution.

There are a few caveats here:

• This relies on the caller not being able to construct new LockedAts via other means (see previous section)
• This relies on the caller not being able to provide a different guard than the one that was originally constructed

This second problem might actually not be a problem. In general this is the problem with "unforgeable token"-style APIs in Rust. However, this crate already tries to guarantee that there is only ever one guard in existence at a time. So maybe it's not a problem for this crate in particular because of the shape of its API? Need to think through that one more...