Tracking Issue for `lock_value_accessors`

[EFanZh]

Feature gate: #![feature(lock_value_accessors)]

This is a tracking issue for feature lock_value_accessors.

Public API

impl<T> Mutex<T> {
    pub fn get_cloned(&self) -> Result<T, PoisonError<()>> where T: Clone { ... }
    pub fn set(&self, value: T) -> Result<(), PoisonError<T>> { ... }
    pub fn replace(&self, value: T) -> LockResult<T> { ... }
}

impl<T> RwLock<T> {
    pub fn get_cloned(&self) -> Result<T, PoisonError<()>> where T: Clone { ... }
    pub fn set(&self, value: T) -> Result<(), PoisonError<T>> { ... }
    pub fn replace(&self, value: T) -> LockResult<T> { ... }
}

impl<T> nonpoison::Mutex<T>
where
    T: ?Sized
{
    pub fn with_mut<F, R>(&self, f: F) -> R where F: FnOnce(&mut T) -> R { ... }
}

impl<T> nonpoison::RwLock<T>
where
    T: ?Sized
{
    pub fn with<F, R>(&self, f: F) -> R where F: FnOnce(&T) -> R { ... }
    pub fn with_mut<F, R>(&self, f: F) -> R where F: FnOnce(&mut T) -> R { ... }
}

Steps / History

• ACP: Add get, set and replace methods to Mutex and RwLock libs-team#485
• Implementation: Add value accessor methods to Mutex and RwLock #133406
• Implementation: Implement non-poisoning Mutex::with_mut, RwLock::with and RwLock::with_mut #147328
• Final comment period (FCP)¹
• Stabilization PR

Unresolved Questions

• Whether we should checking poisoning first and avoid unnecessary lock acquire attempts.

Links

• Follow-up ACP: Add Mutex and RwLock APIs for accessing values within a function scope libs-team#497

Footnotes

1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

[Noratrieb]

I think the name get is too generic and "default", it makes you think that this is what you usually want, even if it is often not (as it clones). Certainly a useful method, but I think the clone should be more explicit in the name, I'd suggest get_cloned (maybe someone else has a better name).

[EFanZh]

I am OK with get_cloned.

[marmeladema]

Would it make sense to have a get_copied()?

Or even a get_with which accepts a closure to retrieve the return value? This could cover clone and copy types as well as allow to return a value from a nested field like mutex.get_with(|val| val.field.clone())

[EFanZh]

@marmeladema: I am in favor of adding a set of more generalized APIs:

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)),
        }
    }
}

These new APIs have the advantage of being explicit about locking scopes. Traditional APIs rely on the destructing of lock guard objects, which is not visually apparent to users, increasing the risk of locks being held for longer than necessary. With the new APIs, the chances of unintentionally holding locks for extended periods can be reduced.

A new ACP has been accepted for non-poisoning types: rust-lang/libs-team#497.

[arvidfm]

Sorry if this has been mentioned elsewhere, maybe it's obvious but it wasn't clear to me - is there a reason that get_cloned returns a Result<T, PoisonError<()>> instead of a Result<T, PosionError<T>>? The latter seems like it would better mirror the signature of Mutex::lock, or (and especially) Mutex::into_inner. That is, why not:

pub fn get_cloned(&self) -> Result<T, PoisonError<T>>
where
    T: Clone,
{
    match self.lock() {
        Ok(guard) => Ok((*guard).clone()),
        Err(e) => Err(PoisonError::new((*e.into_inner()).clone())),
    }
}

[EFanZh]

@arvidfm: I am not sure that whether calling clone is a good idea on the Err path. In my personal experience, most of the time, users just write mutex.lock().unwrap(), so for get_clone, I assume they will also just write mutex.get_cloned().unwrap(). Introducing a clone() here will unnecessarily increase the overhead for the more common case.

[arvidfm]

@EFanZh Is it worth optimising for that case, considering that the user was already prepared to pay the cost of the clone to begin with by calling get_cloned, and the error path is presumably a lot less common?

As it stands, the signature breaks the pattern of get_clone().unwrap_or_else(PoisonError::into_inner) which works for Mutex::lock and Mutex::into_inner, and if the user did want to clone the value regardless of poison status they would need to write something similar to what I proposed above, at which point they're essentially duplicating the functionality of get_cloned.

[EFanZh]

@arvidfm: If user want to get the value regardless of the poison status, maybe its better to use the non-poison version? #134645