Adding locks disregarding poison

[Aandreba]

Proposal

Problem statement

Currently, if you are to use any of the locks provided by the standard library, you are forced to handle lock poisoning.
Most of the time you aren't really interested in maneging these, so you're left with two options:

• Unwrap the result of the lock
• Take the lock from the returned error (with std::sync::PoisonError::into_inner)

Motivation, use-cases

use std::sync::*;

let hello = Mutex::new(2);

std::thread::scope(|s| {
    s.spawn(|| {
        let _guard = hello.lock();
        panic!("Ups!");
    });

    // May return `Err`, if `_guard` is obtained before `guard2`
    let guard2: LockResult<MutexGuard<i32>> = hello.lock();
})

This is the solution if you are to unwrap/expect the result

use std::sync::*;

let hello = Mutex::new(2);

std::thread::scope(|s| {
    s.spawn(|| {
        let _guard = hello.lock();
        panic!("Ups!");
    });

    // May panic, if `_guard` is obtained before `guard2` 
    let guard2: MutexGuard<i32> = hello.lock().unwrap();
})

This is the solution if you are to take the lock from the error

use std::sync::*;

let hello = Mutex::new(2);

std::thread::scope(|s| {
    s.spawn(|| {
        let _guard = hello.lock();
        panic!("Ups!");
    });

    // Never panics
    let guard2: MutexGuard<i32> = match hello.lock() {
        Ok(x) => x,
        Err(e) => e.into_inner()
    };
})

Solution sketches

Ideally, you should be using the last example if you aren't interested in managing poisonous locks, but having to make the same match expression every time you want to lock is very cumbersome, so perhaps we should add a method that makes the lock without checking if it's poisoned.

use std::sync::*;

let hello = Mutex::new(2);

std::thread::scope(|s| {
    s.spawn(|| {
        let _guard = hello.lock();
        panic!("Ups!");
    });

    // Never panics (name is an example, and I'm sure a better one exists)
    let guard2: MutexGuard<i32> = hello.lock_deep();
})

Another proposed solution is to create new lock types that would not implement poison-checking.

// Again, the name is probably improveable
pub struct DeepMutex<T: ?Sized> {
    /* ... */
}

impl<T: ?Sized> DeepMutex<T> {
    pub const fn new (t: T) -> Self where T: Sized { /* ... */ }
    pub fn try_lock (&self) -> Option<DeepMutexGuard<'_, T>> { /* ... */ }
    pub fn lock (&self) -> DeepMutexGuard<'_, T> { /* ... */ }
    /* ... */
}

Links and related work

• Adding locks disregarding poison rfcs#3378

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

[ChayimFriedman2]

The question is whether the common case really want to ignore error on other threads, maybe we want to panic on them?

[Aandreba]

I honestly think the default behaviour of checking for posion is a mistake, since it's rarely considered. But it's already consolidated and we are not about to break backwards compatibility for this, so I think the next best option is to implement a new seperate method that allows you to lock without checking if the lock is poisoned. If you want to panic when poisoned, I think the current way of doing it (lock.lock().unwrap()) is fine

[sfackler]

I think a separate set of types may be preferable to methods, which would avoid having the extra storage overhead of poison tracking.

[Aandreba]

I hadn't thought of it, that could solve the issue too

[the8472]

which would avoid having the extra storage overhead of poison tracking.

We can probably store the poison bit in the atomic used by the futex, which would remove the issue.

[thomcc]

We can probably store the poison bit in the atomic used by the futex, which would remove the issue.

Not all targets use futex-based atomics (not even all tier-1 targets). Also, a separate set of types may be beneficial anyway.

[sfackler]

Regardless of the implementation cost, having a separate type also makes it more clear how the lock is expected to be used through a codebase - you don't need to make a decision at every lock point of which method to call.

[Aandreba]

So what would be the next steps to move forwards?

[KodrAus]

I basically don't use the standard library's lock types specifically because they do both locking and poisoning.

I think we've considered something like std::mutex::Mutex or std::lock::Mutex in the past that could hold new non-poisoning Mutex types. I've also thought a generic std::panic::Poison<T> might make it possible to define the existing lock types as effectively type Mutex<T> = NonPoisoningMutex<Poison<T>>. Not sure how viable that actually is though.

More broadly, I think the accumulation of new APIs like OnceLock that sort of replace older ones like Once over time is going to stratify the standard library in ways that make it confusing to work with unless there's some path for each of these new variants to subsume the old ones. If we want to add non-poisoning locks, then I think we should do so with the intention of de-emphasizing the poisoning ones.

[pitaj]

What about an enum adt_const_generic parameter? (A bool would work but isn't as meaningful)

enum Behavior {
    Poisoning,
    NonPoisoning,
}

struct Mutex<T, Behavior = Behavior::Poisoning>;

impl<T> Mutex<T, Behavior::Poisoning> {
    fn lock(&self) -> LockResult<MutexGuard<T>>;
}

impl<T> Mutex<T, Behavior::NonPoisoning> {
    fn lock(&self) -> MutexGuard<T>;
}

[Aandreba]

If we want to add non-poisoning locks, then I think we should do so with the intention of de-emphasizing the poisoning ones.

I agree, the amount use cases where you need poison detection are quite small.
I still think a new type is the best approach, and it wouldn't be to hard to implement, since the locking implementation is already extracted for each architecture/OS.

[Aandreba]

What about an enum adt_const_generic parameter? (A bool would work but isn't as meaningful)

I don't think this is a good idea. In my opinion, if the struct has a function with the same name that returns fundementaly different types depending on it's generics, it's probably better to have two differens structs (in my opinion).

[bill-myers]

Disregarding poison that way (or using a mutex without poison support) is wrong in general, since it means that you are accessing data structures with broken invariants (due to a panic possibly having happened while the data structure was in the middle of a mutation).

Hence, it should not be supported in the standard library (and arguably should not be implemented at all in safe code).

Use unwrap() if you don't want to care about poison, which will correctly propagate the panic from the panicing thread into all other users of the same mutex whose data was potentially corrupted.

If you don't want panics to propagate between threads, pass messages using channels instead of using shared mutex-protected data.

Making panics abort the whole process along with spawning processes for subtasks is also an effective and even more reliable solution.

[KodrAus]

Disregarding poison that way (or using a mutex without poison support) is wrong in general, since it means that you are accessing data structures with broken invariants (due to a panic possibly having happened while the data structure was in the middle of a mutation).

I think poisoning is a great pattern for dealing with either explicit or implicit erroneous control flow breaking atomicity, but it's a distinct concept from mutual exclusion, even if they work well together. I have state that's not under a mutex that I use poisoning for, like file handles. I also have state under mutexes where poisoning isn't the chosen strategy for dealing with panics and errors.

Mutex<T> and Poison<T> are just two independently useful abstractions.

[joshtriplett]

We reviewed this in today's @rust-lang/rust meeting.

We're in favor of adding non-poisoning versions of all our mutex types. We'd like to see these all added under std::sync::nonpoison::*. We'd also like to see all the existing poison types moved to std::sync::poison with re-exported in std::sync. Then, in a future edition, we can change which type is exported as std::sync::Mutex.

[Aandreba]

I'm a bit busy nowadays, but I'll se if I can find a moment to implement it.

[joboet]

@Aandreba, do you still want to work on this? I'll take this up otherwise.

[Aandreba]

I currently have a bit of time to work on it, but I'm having a bit of trouble setting up the dev environment.

[joboet]

Great! Feel free to reach out if you have any questions (for instance on Zulip).