Edition 2027: Consider replacing `std::sync::Mutex` and other locks with `std::sync::nonpoison` versions

[tgross35]

This serves as a place to discuss changing default lock types to their nonpoison versions, which has come up a number of times before but not yet (to my knowledge) discussed in depth.

@davepacheco brings up a good point at #134645 (comment) that having posioning locks tends to be a safer default, and that the .unwrap() annoyance could be smoothed with panicking methods rather than changing out the type.

I'm not sure whether or not an RFC would be required but considering the impact, it doesn't seem like a bad idea (see also rust-lang/rfcs#3550 for a similar change, though that involved lang).

Related:

• Tracking issue for adding the types: Tracking Issue for sync_nonpoison and nonpoison_{condvar,mutex,rwlock} #134645
• Tracking issue for the modules: Tracking Issue for sync_poison_mod #134646

[anp]

One adjacent topic that IMO is worth considering is panic=abort. I work on a codebase that's largely compiled with panic=abort so the poisoning API doesn't offer any value to those programs. Given https://smallcultfollowing.com/babysteps/blog/2024/05/02/unwind-considered-harmful/ and other discussions I wonder whether it's worth considering the frequency of poisoned locks even being observable in users' programs in practice.

[bjorn3]

and that the .unwrap() annoyance could be smoothed with panicking methods rather than changing out the type.

I would be 100% in favor of this! If anything I did rather make .lock() panic on poison by default (with a different lock method returning a result that you can handle) than remove poison from the default mutex type. Without poisoning you are way too likely to keep limping around without making any forward progress ever again when an essential background thread panicked.

[mkeeter]

As a quick example, here's code which would panic with the current behavior, but would be UB (dereferencing a null pointer) with non-poisoning mutexes:

use std::sync::Mutex;

/// The mutex protects the invariant that the pointer is always valid
struct PointerWrapper(Mutex<*mut u32>);

const SHOULD_PANIC: bool = false;

fn poke_pointer(p: &PointerWrapper) {
    let mut p = p.0.lock().unwrap();
    // Briefly break invariants to manipulate the pointer
    let prev = std::mem::replace(&mut *p, std::ptr::null_mut());
    let next = do_something_that_could_panic(prev);
    *p = next;
}

fn do_something_that_could_panic(ptr: *mut u32) -> *mut u32 {
    if SHOULD_PANIC {
        panic!("oh no");
    }
    ptr
}

fn main() {
    let mut i = 123u32;
    let p = PointerWrapper(Mutex::new(&mut i));
    let _ = std::panic::catch_unwind(|| poke_pointer(&p));
    
    let value = p.0.lock().unwrap();
    println!("{}", unsafe { **value });
}

This is obviously a contrived example – and one could argue that updating to remove the .unwrap() would force people to consider the edition changes – but in a large codebase, the path of least resistance would be for people to remove
the unwrap() and go along with their day, unaware that the std::sync::Mutex behavior has changed underneath them.

(FWIW, I also like the idea of making .lock() panic on poisoning, which could happen at an edition boundary!)

[hawkw]

It's worth noting that if lock() is changed to always panic on poisoning at an edition boundary, it should probably only do so if !std::thread::panicking(); I've seen plenty of code where PoisonErrors are always unwrapped except in a Drop impl, to avoid recursive panics causing the program to immediately abort.

One downside of making the API unconditionally panic rather than return a Result is how it effects this case, actually. If poisoning is ignored when the thread is already panicking, then the Drop implementation can witness the potentially-violated invariants. I've seen code where Drop implementations that do something not required for memory safety (i.e. updating timing data or counters or similar that are inside of a lock) just only do that action when lock() returns Ok, and skip it if the lock has already been poisoned and the Drop impl was called in the midst of unwinding. Changing the lock() API to unconditionally panic instead does make that behavior unexpressable.

[bjorn3]

Changing the lock() API to unconditionally panic instead does make that behavior unexpressable.

We should still keep the non-panicking variant of the lock method around IMO. Just with a different name.

[tgross35]

This will need to be discussed by the team at some point, may as well nominate it now.

@rustbot label +I-libs-api-nominated

[theemathas]

it should probably only do so if !std::thread::panicking();

The situation is a bit more complicated. See #143612