Add functions to un-poison Mutex and RwLock

[tmccombs]

See discussion at https://internals.rust-lang.org/t/unpoisoning-a-mutex/16521/3

[dtolnay]

Nice, this looks good to me.

Could you please make a tracking issue and update the unstable attributes with the issue number?

[tmccombs]

I have created a tracking issue and updated the unstable attributes to reference it.

[dtolnay]

Reassigning to Mara at her request to look at how this fits in with "plans".

r? @m-ou-se

[dtolnay]

One thing I notice from looking at this a second time is it's not great RwLock unpoison only works if you have a write guard. If you have a read guard, you'd be out of luck.

match rwlock.read() {
    Ok(_) => …,
    Err(poison_error) => {
        let guard = poison_error.into_inner();
        /* :( */  //RwLock::clear_poison(&guard);

        // instead we have to do:
        drop(guard);
        let another_guard = rwlock.write().unwrap_err().into_inner(); // probably still poisoned (?)
        RwLock::clear_poison(&another_guard);
        drop(another_guard);
        // back to read, since we didn't actually want write to begin with
        let yet_another = rwlock.read().unwrap(); // probably not already re-poisoned (?)
        // now we can finally read
    }
}

[m-ou-se]

I have a few thoughts:

• Unpoisoning is probably most commonly done after resetting the state to some valid state, which usually implies write access. But it it might be perfectly reasonable to check the state is valid with only read access, and clear the poison if it didn't need any cleaning up before unpoisoning.

• There's nothing unsafe about just having a Mutex::clear_poison(&self) that does not take a MutexGuard at all, such that it can be used regardless of whether the lock is locked, and for RwLock also regardless of whether it's locked in read or write mode. Clearing the poison without locking the mutex is probably not a good idea in most cases, but it's not unsafe.

• For a likely future implementation of Mutex and RwLock, it is slightly more efficient to combine unlocking and unpoisoning into a single operation. To enable that, this API would have to look something like Mutex::unlock_and_clear_poison(MutexGuard) that consumes the MutexGuard.

• Mutex::get_mut can also return a poison error, but one that does not contain a MutexGuard, which wouldn't allow using the clear_poison api if it took a MutexGuard. However, with &mut access to the Mutex, one could replace the entire mutex with a fresh unpoisoned one through mem::replace as an alternative.

[m-ou-se]

I like the simplicity of just mutex.clear_poison() as an API. I suppose the main pitfall would be for users to clear the poison after unlocking the mutex when it should have been cleared just before, opening a tiny window in which another poison event could be missed.

I wonder how big of a pitfall that is in practice though, considering how the code would be structured. I imagine most usages would look something like this:

let x = m.lock().unwrap_or_else(|e| {
    e.get_mut().corrupted_state.reset();
    m.clear_poison();
    e.into_inner()
});
x.stuff.do_something();
// (implicit) drop(x);

That is, first handle the poison, and then continue using it afterwards for the reason you attempted to lock it. This would make it hard to put m.clear_poison(); in the wrong place, since the guard needs to stay alive for longer. (It'd also make an unlock_and_clear_poison API inconvenient.)

The only other possible pitfall I can imagine is folks doing m.clear_poison() right before locking it, in the hope that that will guarantee m.lock() will never give a poison error, forgetting about the brief period of time between m.clear_poison() and m.lock(). But that seems a bit contrived.

[m-ou-se]

Another option could be to add it as a method on PoisonError<MutexGuard>: error.clear_poison() as an equivalent of error.into_inner() that also unpoisons the mutex. Then both PoisonError<RwLockWriteGuard> and PoisonError<RwLockReadGuard> could have that method. And the snippet from above would reduce to this:

let x = m.lock().unwrap_or_else(|e| {
    e.get_mut().corrupted_state.reset();
    e.clear_poison()
});
x.stuff.do_something();
// (implicit) drop(x);

[tmccombs]

I'd be fine with any of those approaches.

[m-ou-se]

@dtolnay What do you think?

[m-ou-se]

We discussed this in the libs-api meeting just now, and we agreed that we should go forward with Mutex::clear_poison(&self), independent of MutexGuard and PoisonError.

(If the pattern of clearing poison right before doing PoisonError::into_inner() turns out to be a common one, we could consider PoisonError::clear_poison() as a short-hand for that pattern, but that would be a separate proposal.)

[tmccombs]

I've updated the PR to take &self instead of a guard.

[m-ou-se]

Thanks!

The doc comments still refer to the guards though. Can you update that too?

[tmccombs]

oh, oops, yeah, I can update that.

[m-ou-se]

Thanks!

@bors r+

[bors]

📌 Commit a65afd8 has been approved by m-ou-se

[bors]

⌛ Testing commit a65afd8 with merge cd73afa...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing cd73afa to master...

[rust-timer]

Finished benchmarking commit (cd73afa): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	1	1	1	1	2
mean2	4.9%	1.8%	-5.8%	-2.9%	-0.5%
max	4.9%	1.8%	-5.8%	-2.9%	-5.8%

Cycles

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: 😿 relevant regression found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	2	1	9	0	11
mean2	2.2%	4.9%	-3.3%	N/A	-2.3%
max	2.3%	4.9%	-4.9%	N/A	-4.9%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. number of relevant changes ↩ ↩²

2. the arithmetic mean of the percent change ↩ ↩²

[hankai17]

Feature is needed?
If local thread is active, the mutex can also still become poisoned at any time.

#![allow(unused)]

use std::sync::{Arc, Mutex};
use std::thread::spawn;
use std::thread;
use std::time::Duration;

fn sleep_ms(ms: u64) {
    use std::thread;
    use std::time::Duration;
    thread::sleep(Duration::from_millis(ms));
}

#[derive(Debug)]
struct User {
    id: u32
}

impl User {
    fn get_id(&self) -> u32 {
        self.id
    }
    fn set_id(&mut self, id: u32) {
        self.id = id;
    }
    fn run_loop(&mut self) {
        let user = get_current_user();
        println!("is_poisoned: {}", user.is_poisoned());  // there should be true
        sleep_ms(1000 * 3);
    }
}

use std::cell::Cell;
use std::cell::RefCell;
use std::thread_local;

thread_local! {
    pub static CURRENT_USER: RefCell<Arc<Mutex<User>>> = panic!("!");
}

fn get_current_user() -> Arc<Mutex<User>> {
    let ptr = CURRENT_USER.with(|user| -> *mut Arc<Mutex<User>> {return user.as_ptr()});
    unsafe {
        let clone = (*ptr).clone();
        clone
    }
}

fn main() {
    let u1 = User {id: 4};
    let mut user = Arc::new(Mutex::new(u1));
    CURRENT_USER.set(user.clone());
    user.lock().unwrap().run_loop();
}