ACP: Freezable Mutex Guards

[SpaceBroetchen]

Proposal

Problem statement

Rusts Locks are wonderful, most multithreaded code would be impossible to write safe without them. But handling locks
over more than one scope is just a nightmare. This is because you always require the ownership of the guard if you want
to release the lock. This often leads to call chains where every method requires the owned Guard and the Mutex reference
as parameter and the owned Guard as return type. This produces unreadable code and leads to a lot of useless parameter
passing. The second approach if passing the Mutex is not wanted is to lock and unlock it in every scope needed. With
this style the code is much more readable, but it takes a lot of unnecessary locks and unlocks of the Mutex. This
approach is less performant.

Motivating examples or use cases

Unlocks the mutex temporary and lock it after the calling func again, while only holding a mutable reference.
If the Mutex is poisoned upon trying to lock it again, the heal function will be called to fix the inner data. After
this, the poison will be cleared and the mutex guard is valid again.

Example

use std::thread;
use std::sync::Mutex;
use std::sync::Condvar;
use std::time::Duration;

pub fn request_data(guard: &mut MutexGuard<usize>, condvar: &Condvar) {
    MutexGuard::freeze_guard(
        guard,
        || { 
            &condvar.notify_one();
            thread::sleep(Duration::from_millis(100));
        },
        |poison| { panic!() }
    );
}

pub fn main() {
    let mutex = Mutex::new(0usize);
    let condvar = Condvar::new();

    thread::scope(|s| {
        let outer_guard = mutex.lock().unwrap();

        s.spawn(|| {
            let inner_guard = mutex.lock().unwrap();
            
            let inner_guard_ref = &mut inner_guard;
            assert_eq!(**inner_guard_ref, 0);
            request_data(inner_guard_ref, &condvar);
            assert_eq!(**inner_guard_ref, 123);

        });

        let outer_guard = &condvar.wait(outer_guard).unwrap();
        **outer_guard = 123;
        drop(outer_guard)
    })
}

Solution sketch

The concept is to add a method with three parameters for the guard. First, obviously the mutable reference to the guard.
The second parameter is a closure containing code that should be run while the Mutex is unlocked. This by itself would
already work, but it might cause problems if another thread panics while holding the data. Therefore, the third
parameter is a "heal" closure. It takes a PoisonError with the guard as parameter to allow the thread to fix the inner
data.

This would allow always just using the mutable reference instead of the owned object. Which leads to better readability
and maintainability while still being safe.

impl<'a, T: ?Sized> MutexGuard<'a, T> {
    pub fn freeze_guard<F, H>(orig: &mut Self, func: F, heal: H) -> () 
    where
        F: FnOnce() -> (),
        H: FnOnce(PoisonError<T>) -> (),
    {
        // Unlocking the mutex
        unsafe {
            orig.lock.poison.done(&self.poison);
            orig.lock.inner.unlock();
        }
        
        // FIXME: Upon panicking in func the drop of self will be called which will lead to problems because the mutex
        //  is not actually locked by this guard. This might need a small change in the drop logic of the guard.
        
        // calling func
        func();
        
        // trying to acquire lock again
        let lock_result = orig.lock.lock();
        let new_guard = match lock_result {
            Ok(guard) => guard,
            Err(poison_error) => {
                heal(poison_error);
                orig.lock.clear_poison();
                // consider the case that the mutex is getting healed, another thread grabs the cleared mutex and poisons
                // it again. The heal function is currently an FnOnce and therefore it cannot be cleared again.
                orig.lock.lock().unwrap()
            },
        };
        // replacing the old variable with the new
        std::mem::swap(orig, &new_guard);
        // forgetting the value to prevent unlocking of the mutex again
        std::mem::forget(new_guard)
    }
}

Safety

During the call of func the mutable reference will be in the scope of freeze_guard and therefore it is not possible
to access it. The heal function will always restore the cleared mutex state and therefore guarantees a valid guard
after freeze_guard. If heal panics the mutex guard will remain poisoned as before.

Panics:

If the thread panics while running the func closure the mutex will remain clear because it is not locked by the
current thread (excluding the case that the thread is acquiring the lock in the func). If the thread panics in the
heal function the thread mutex remains poisoned.

Alternatives

There are currently three ways to achieve the same behavior:

1. Passing the owned Guard across the call stack. This results in bad readability and maintainability.
2. Locking and unlocking the Mutex at every call. If the mutex is heavily used, this costs a lot of unnecessary performance.
3. Juggling with alot of unsafe code to somehow access the inner lock and poison to manually achieve the behavior of this
   function.

Links and related work

An prevoius discussion has been made here: rust-lang/rfcs#3870

[the8472]

Does this do anything that Arc<Mutex> doesn't?

[SpaceBroetchen]

@the8472
It does not have to do anything with the Arc, it only applies to the Mutex and MutexGuard. It allows to temporary unlock the Mutex while only holding a mutable reference to the guard (usually you need the guard owned).

Please explain your question a bit better probably with an example how you would achive this behaviour with an Arc<Mutex>.

[kennytm]

Continuing rust-lang/rfcs#3870 (comment).

But I don't have to gurantee it at the start and end of the request_data scope if i use a guard reference instead

But all your request_data doing is unlock the mutex and notify the condvar. The entire example does not look like needing a long-life MutexGuard at all

#![feature(lock_value_accessors)]
use std::thread;
use std::sync::Mutex;
use std::sync::Condvar;
use std::time::Duration;

pub fn request_data(condvar: &Condvar) {
    condvar.notify_one();
    thread::sleep(Duration::from_millis(100));
}

pub fn main() {
    let mutex = Mutex::new(0usize);
    let condvar = Condvar::new();

    thread::scope(|s| {
        s.spawn(|| {
            assert_eq!(mutex.get_cloned().unwrap(), 0);
            request_data(&condvar);
            assert_eq!(mutex.get_cloned().unwrap(), 123);
        });

        let mut outer_guard = condvar.wait(mutex.lock().unwrap()).unwrap();
        *outer_guard = 123;
        drop(outer_guard)
    })
}

[SpaceBroetchen]

Yes the example is too small to acutally demonstrate the problem. But I don't want to put a too large example in here. The github issue is supposed to show a simple example. I had multiple projects with guards passing across mutiple function scopes. It is fine as long as you only have a few functions. But doing this across 50 functions just because one function at the top of the call needs occasionally an unlock leads to very bad readability.

[kennytm]

I had multiple projects with guards passing across mutiple function scopes. It is fine as long as you only have a few functions. But doing this across 50 functions just because one function at the top of the call needs occasionally an unlock leads to very bad readability.

TBH that does not feel like an idiomatic use of mutex, holding a mutex guard blocks every other thread from accessing the mutex interior, if you need to lock the mutex for a long time then why need multiple threads in the first place 😕. It may be more readable if you instead just use a standard &mut T variable and occasionally spawn a local thread or engage in MPS communication for the out-of-thread updates.

[tgross35]

If you need this pattern, it's easy enough to implement with a refcell wrapper around the guard:

use std::sync::Mutex;
use std::sync::MutexGuard;
use std::cell::RefCell;

type DroppableGuard<'a, T> = RefCell<Option<MutexGuard<'a, T>>>;
static M: Mutex<u32> = Mutex::new(100);

fn main() {
    let g = M.lock().unwrap();
    let g = DroppableGuard::new(Some(g));
    assert!(M.try_lock().is_err());
    demo(&g);
    assert!(M.try_lock().is_ok());

}

fn demo(g: &DroppableGuard<u32>) {
    assert!(M.try_lock().is_err());
    drop(g.borrow_mut().take().expect("already unlocked"));
    assert!(M.try_lock().is_ok());
}

But I agree that the use pattern seems unusual.

[joshtriplett]

We talked about this in today's @rust-lang/libs-api. We weren't sold on the fully general case of "unlock and call an arbitrary function". However, for the specific case of unlocking to signal a a CondVar without having to pass the guard around by value, would it make sense to add a CondVar::wait_something method that takes a &mut MutexGuard? Would that solve the most egregious cases here?

[kennytm]

CondVar::wait_something taking &mut MutexGuard cannot couple the poison error with the guard, so this will solely rely on #[must_use] to enforce the extra .unwrap() to propagate the panic (the same case as Mutex::set so perhaps not a big problem).

impl CondVar {
    pub fn wait_something<T>(&self, guard: &mut MutexGuard<'_, T>) -> LockResult<()> {
        let poisoned = unsafe {
            let lock = mutex::guard_lock(guard);
            self.inner.wait(lock);
            mutex::guard_poison(guard).get()
        };
        if poisoned { Err(PoisonError::new(())) } else { Ok(()) }
    }
}

[Amanieu]

parking_lot has this as MutexGuard::unlocked. However the interface is much simpler since it doesn't need to support poisoning. Could we consider only supporting this on non-poisoning locks?

[kennytm]

For non-poisoning locks the MutexGuard::mutex() function seems more fundamental to add, since you could then use replace_with to get the same effect:

fn unlocked<'a, T: ?Sized + 'a, R>(guard_ref: &mut MutexGuard<'a, T>, f: impl FnOnce() -> R) -> R {
    let mutex = MutexGuard::mutex(guard_ref);
    replace_with_and_return(
        guard_ref, 
        || mutex.lock(),
        |guard| {
            drop(guard);
            (f(), mutex.lock())
        }
    )
}

[SpaceBroetchen]

If you need this pattern, it's easy enough to implement with a refcell wrapper around the guard:

In this example you don't even need the RefCell.
But with this solution you create something similar to a dangling pointer. You just relay on the "hope" that g will always be in a valid state. Mutable references on would allow you to prove the valid state with the compiler which leads (in my opinon) a lot safer code.

We talked about this in today's @rust-lang/libs-api. We weren't sold on the fully general case of "unlock and call an arbitrary function". However, for the specific case of unlocking to signal a a CondVar without having to pass the guard around by value, would it make sense to add a CondVar::wait_something method that takes a &mut MutexGuard? Would that solve the most egregious cases here?

I haven't thought about it until now. I agree that this would be useful. But it would not be possible to do something in the thread which unlocks the mutex while it is unlocked. Running functions while unlocking would allow you to achive the same behavior with a CondVar with something like this:

let mutex: &Mutex<T> = ...;
let guard: &mut MutexGuard<T> = ...;
let cvar: &CondVar = ...;

Mutex::unlocked(guard, || {
    let inner_guard = mutex.lock().unwrap();
    let _ = cvar.wait(inner_guard);
});

this requires 2 more lock and unlock steps but it will be done at states which are valid anyway. I still agree that this is not an ideal solution. But it would be a lot more difficult to somehow run a function by waiting on a CondVar.

CondVar::wait_something taking &mut MutexGuard cannot couple the poison error with the guard, so this will solely rely on #[must_use] to enforce the extra .unwrap() to propagate the panic (the same case as Mutex::set so perhaps not a big problem).

This would be a good way to implement it. Repairing of the Mutex would work somehow like this?:

let guard: &mut MutexGuard<T> = ...;
let mutex: &Mutex<T> = ...;
let cvar: &CondVar = ...;

if let LockResult::Err(_) = cvar.wait_something(guard) {
    do_repair(guard);
    mutex.clear_poison();
}

parking_lot has this as MutexGuard::unlocked. However the interface is much simpler since it doesn't need to support poisoning. Could we consider only supporting this on non-poisoning locks?

I agree on adding it to non-poison locks aswell, but limiting it to only them is not useful. The primary goal for this functionality is to allow easier access for long living guards (in terms of code). Non-poison locks are useful if you eighter only have very small lock scopes where a panic safety can be manually guaranteed or for types which are always valid which are mostly covered by atomics. Therefore the use of this ACP will be lost.

For non-poisoning locks the MutexGuard::mutex() function seems more fundamental to add, since you could then use replace_with to get the same effect:

This isn't working because the guards are not implementing Default. This is needed to prevent dropping the guard twice upon unwinding.