Is mutex poisoning guaranteed to always work properly?

[purplesyringa]

Is this code sound?

// Represents an abstract type with safety invariants
struct Corruptee { is_corrupted: bool }
impl Corruptee {
    // SAFETY: must be non-corrupted
    unsafe fn witness_uncorrupted(&self) {
        unsafe { core::hint::assert_unchecked(!self.is_corrupted) }
    }
    fn corrupt(&mut self) { self.is_corrupted = true; }
    fn uncorrupt(&mut self) { self.is_corrupted = false; }
}

struct Worker {
    corruptee: Mutex<Corruptee>,
}

impl Worker {
    fn do_work(&self, f: impl FnOnce()) {
        let mut corruptee = self.corruptee.lock().expect("poisoned");
        // SAFETY: `corruptee` can only be corrupted if `f` has ever panicked, but if it did, the
        // mutex would be poisoned and `expect` would panic.
        unsafe {
            corruptee.witness_uncorrupted();
        }
        corruptee.corrupt();
        f();
        corruptee.uncorrupt();
    }
}

I'm aware that poisoning is advisory in the sense that anyone with access to the Mutex can remove the poison if they wish. But in this example, Mutex is never revealed to users of the safe abstraction, and is only used an implementation detail.

Do we want to guarantee that poisoning can be relied upon for safety, like in this example, and never has false negatives?

I'm asking because mutex poisoning doesn't always work currently. But it's also never explicitly documented to be best-effort.

@workingjubilee

@rustbot label +A-concurrency +C-discussion +T-libs-api

(If poisoning was considered reliable, this would be T-libs I-unsound.)

[purplesyringa]

While we're at it, I'd like to ask for a formalization of when poisoning is guaranteed to not happen.

The docs currently say that "a mutex is considered poisoned whenever a thread panics while holding the mutex". But that's not quite true: if I catch the panic with catch_unwind before the guard is dropped, the mutex will stay unpoisoned, and I think that's a good thing.

But we also can't say that mutex poisoning depends exclusively on whether MutexGuard is dropped by hand with drop(...) or by going out of scope, as opposed to being dropped by the landing pad, see e.g. this example, where the guard is dropped by going out of scope, but is poisoned, since that happens inside a destructor that was itself called due to a panic.

We could say

A mutex is considered poisoned whenever a thread panics while holding the mutex, and that panic is not caught with catch_unwind before the mutex guard is dropped.

And I think this mirrors the ideal behavior. But I don't think we can implement that. If the mutex is locked while a panic is being handled, and then destroyed afterwards (e.g. by moving data out of a scopeguard closure), then the "active panic" at the construction and destruction changes. Under the semantics I was meaning to propose to fix the bug in the OP, that'd indicate that the mutex should be poisoned. Trying to be more clever about this would include checks like "is this panic handle an ancestor of that panic handle in the historical exception handling tree" and I just don't think we want to maintain that info.

But I also wouldn't want to guarantee that

A mutex is considered poisoned whenever a thread throws an uncaught panic or catches a panic with catch_unwind while holding the mutex.

Because I think that'd expose too many implementation details and leave us unable to tweak the behavior if necessary.

[bjorn3]

Mutex poisoning only happens when std::thread::panicking() returns true. When unwinding because of a non-rust exception, this will return false however as nobody has updated the thread local variable that stores the panic count.

[purplesyringa]

Yes, that's a good addition. But I think it's also important to figure out if that's an implementation detail or something we want to put focus on.

There's a problem I've discussed with @workingjubilee -- the std::thread::panicking() API is broken and we shouldn't define poisoning on top of that. I'll submit a separate issue about that a bit later, and I don't want to repeat myself in many unrelated comments, lest I make a subtle mistake, but the TL;DR is that std::thread::panicking() effectively tracks the presence of landing pads anywhere up the call stack, so the conditions under which f is called in the example snippet for the second time are that panicking is set for the whole duration of MutexGuard's life, and so it's impossible to draw any conclusions from the value of panicking at the time alone.

So I was hoping that we could answer the question in this issue without mentioning panicking in the semantics. That would be very valuable.

[Amanieu]

As per #143612 (comment), we could fix poisoning if we had a reliable panic_count, but it's not clear whether this is possible in the face of foreign exceptions.

Separately, we are also happy to just consider poisoning "best effort". It should therefore not be relied on by unsafe code for soundness. The documentation will have to be updated to reflect that.