Add Mutex and RwLock APIs for accessing values within a function scope

[EFanZh]

Proposal

Problem statement

This is a follow-up on #485 to add a set of more generalized APIs for accessing values inside lock objects.

Motivating examples or use cases

The most common pattern to access the value inside a lock object is:

1. Acquire a lock guard object from the lock object.
2. Access the value through the lock guard object.
3. Release the lock by dropping the lock guard object.

A problem is that the dropping of lock guards is often implicit, the scope where the lock object being held is not visually apparent, increasing the risk of locks being held for longer than necessary. For example in the code below, uses may not always realize that the mutex_1 is unlocked after the mutex_2.

let mut guard_1 = mutex_1.lock().unwrap();

// Use `guard_1`.

let mut guard_2 = mutex_2.lock().unwrap();

// Use `guard_2`.

// Implicit drop of `guard_2`.
// Implicit drop of `guard_1`.

Not only guard_1 is held longer than necessary, also if some other thread acquire these Mutexs in a different order, deadlock could happen.

Solution sketch

Add the following APIs to the standard library.

impl<T> Mutex<T>
where
    T: ?Sized,
{
    pub fn with_mut<F, R>(&self, f: F) -> Result<R, PoisonError<F>>
    where
        F: FnOnce(&mut T) -> R,
    {
        match self.lock() {
            Ok(mut guard) => Ok(f(&mut guard)),
            Err(_) => Err(PoisonError::new(f)),
        }
    }
}

impl<T> RwLock<T>
where
    T: ?Sized,
{
    pub fn with<F, R>(&self, f: F) -> Result<R, PoisonError<F>>
    where
        F: FnOnce(&T) -> R,
    {
        match self.read() {
            Ok(guard) => Ok(f(&guard)),
            Err(_) => Err(PoisonError::new(f)),
        }
    }

    pub fn with_mut<F, R>(&self, f: F) -> Result<R, PoisonError<F>>
    where
        F: FnOnce(&mut T) -> R,
    {
        match self.write() {
            Ok(mut guard) => Ok(f(&mut guard)),
            Err(_) => Err(PoisonError::new(f)),
        }
    }
}

With the proposed APIs above, the original example can be rewritten as:

mutex_1.with_mut(|value_1| { ... }).unwrap();
mutex_2.with_mut(|value_2| { ... }).unwrap();

In this way, the scope where the lock being held is more clear to user.

Alternatives

None.

Links and related work

This proposal is originally from: rust-lang/rust#133407 (comment).

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

[kennytm]

Returning PoisonError<F> is a bit strange though (same for #485 actually), since the documentation assumed the generic argument of PoisonError is a lock guard, rather than the original input.

[EFanZh]

Mutex::into_inner and Mutex::get_mut have already used non-guard types in PoisonError. Maybe we can relax the meaning of PoisonError’s generic argument?

[cuviper]

What would you do with the F in PoisonError<F>? Do you expect to retry the closure elsewhere?

(and yes, it's weird because the poison has nothing to do with the F it's wrapping...)

[EFanZh]

I don’t have a real use case for returning F back, but for maximal capability, I think it is generally a good idea to return back the unused argument. We don’t have to use the exact PoisonError type if it is too strange, we can create a new error type. The error type should indicate that the error is caused by poisoning and also contain the unused argument. PoisonError is just an error I have in hand that does the job.

[kennytm]

Mutex::into_inner and Mutex::get_mut have already used non-guard types in PoisonError. Maybe we can relax the meaning of PoisonError’s generic argument?

True, but also unlike #485 and this ACP, these two methods still return a Result<X, PoisonError<X>>, rather than Result<X, PoisonError<Y>>, in the sense that you can still get the same X by ignoring the poison.

In our case, there are two typical yet distinct choices when dealing with poison:

1. Treat the poison as some opaque error (propagate upwards later, or just unwrap and panic)

   fn with_mut_poisoned(&self, f: impl FnOnce(&mut T) -> R) -> Result<R, OpaquePoisonError<F>> {
       match self.lock() {
           Ok(mut guard) => Ok(f(&mut guard)),
           Err(_) => Err(OpaquePoisonError::new(f)),
       }
   }

2. Disregard the poison, note that we don't need to re-lock the mutex

   fn with_mut_disinfected(&self, f: impl FnOnce(&mut T) -> R) -> R {
       let mut guard = match self.lock() {
           Ok(guard) => guard,
           Err(err) => err.into_inner(),
       };
       f(&mut guard)
   }

The proposed API cannot fulfill the second choice.

---

So I think both use cases can be supported only if it was split into a 2-step process, the first step to handle the poison, and the second step to operate on the actual data. That seems like we should add the functions on the guard itself:

let result_poisoned = mutex
    .lock()
    .unwrap()
    .with(f);

let result_disinfected = mutex
    .lock()
    .match {  // allow RFC #3295 already
        Ok(guard) => guard,
        Err(e) => e.into_inner()
    }
    .with(f);

However, a MutexGuard::with(self, f) function is not valid, because MutexGuard implemented Deref so you must spell it with fully-qualified notation (like MutexGuard::map)

let result_poisoned = MutexGuard::with(mutex.lock().unwrap(), f);

(╯°□°)╯︵ ┻━┻

at this point why not just directly write

let result_poisoned = f(&mut mutex.lock().unwrap());

[EFanZh]

True, but also unlike #485 and this ACP, these two methods still return a Result<X, PoisonError<X>>, rather than Result<X, PoisonError<Y>>, in the sense that you can still get the same X by ignoring the poison.

#485 has already introduced such a case in Mutex::get, where its return type is Result<(), PoisonError<T>>.

The proposed API cannot fulfill the second choice.

Based on this comment in #485, I assume the standard library team don’t want a API that ignores poisoning in the current lock types.

at this point why not just directly write

let result_poisoned = f(&mut mutex.lock().unwrap());

This is exactly the situation I would like to avoid. The mutex guard will only be dropped after the entire statement being evaluated. You can imagine cases like:

let result_poisoned = f(&mut mutex.lock().unwrap());

g(result_poisoned);

Without a comment explaining the lock guard should be dropped as soon as possible, someone could rewrite the code above into:

g(f(&mut mutex.lock().unwrap()));

Unless the users have studied the drop order rules, it might be hard for them to notice that g is called with the locking being held. The problem is still unlocking being implicit. Comparing to my proposal:

let result_poisoned = mutex.with_mut(f).unwrap();

g(result_poisoned);

Rewriting the code above in the same way will not extend the scope where the lock is being held:

g(mutex.with_mut(f).unwrap());

[kennytm]

#485 has already introduced such a case in Mutex::get, where its return type is Result<(), PoisonError<T>>.

rust-lang/rust#133406 is not yet approved even

Based on this comment in #485, I assume the standard library team don’t want a API that ignores poisoning in the current lock types.

Yeah for sure no one wants a force_xxxx variant, like I'm not proposing a force_with function here either.

The situation here is that Result<X, PoisonError<Y>> is not like LockResult<X>, you can't meaningfully do anything other than treating it like an opaque error, even if you are sure no invariants of X can be broken even if there is panic while the lock is held. So to "ignore poisoning" when using get/set/replace/with the only choices are:

1. someone actually implement Adding locks disregarding poison #169; or
2. use third-party mutex packages like antidote or parking_lot (if they implement these short-cut that is); or
3. keep using the guards (.lock())

I'm not saying these are bad primitives, but either documentation of PoisonError should not say "allow access regardless" in their accessor descriptions, or they should return error types other than PoisonError.

[the8472]

Note that the Mutex examples already show how to achieve this by using blocks.

        let result = {
            let mut data = data_mutex_clone.lock().unwrap();
            // This is the result of some important and long-ish work.
            let result = data.iter().fold(0, |acc, x| acc + x * 2);
            data.push(result);
            result
            // The mutex guard gets dropped here, together with any other values
            // created in the critical section.
        };

[EFanZh]

Note that the Mutex examples already show how to achieve this by using blocks.

A block does not always work, for example, if the lock call is in the last expression of the block, the lifetime of the lock guard will be extended. For example, this code will deadlock:

let result = { mutex.lock().unwrap().iter().fold(0, |acc, x| acc + x * 2) } + {
    mutex.lock().unwrap().iter().fold(0, |acc, x| acc + x * 2)
};

You have to rewrite the code as:

let result = {
    let guard = mutex.lock().unwrap();

    guard.iter().fold(0, |acc, x| acc + x * 2)
} + {
    let guard = mutex.lock().unwrap();

    guard.iter().fold(0, |acc, x| acc + x * 2)
};

Comments may also need to be added to prevent someone else from refactoring it into the first one.

[Amanieu]

We discussed this in the libs-api meeting: considering the complexity around poisoning, we feel that that a "with" API should only be added to non-poisoning mutexes as proposed in #169. As such, we are partially accepting this ACP, and rejecting it for poisoning mutexes.