Add get, set and replace methods to Mutex and RwLock

[EFanZh]

Proposal

Problem statement

I have observed that in many situations, the lifetime of lock guards obtained from Mutex and RwLock are unnecessary extended. Most of the situations are single reads and single writes. Unnecessary extended mutex guard can cause bad performance or even deadlock. It could be better if we can provide convenient methods for these situations where extending lifetime of a mutex guard is unnecessary, we could potentially avoid some bugs or performance degenerates, also make the code easier to understand.

Motivating examples or use cases

Using lock methods in a long expression

People might write something like:

let value = mutex.lock().unwrap().clone().into_iter().filter_map(...).collect::<Vec<_>>();

This unnecessarily extends the lifetime of the mutex guard, it could be better to write:

let list = mutex.lock().unwrap().clone(); // Release lock as soon as possible.
let value = list.into_iter().filter_map(...).collect::<Vec<_>>();

By providing a Mutex::get method, we could potentially prevent some unintended misuses.

Assigning to value inside a mutex

I have observed that many people would write this to assigning to a value inside a mutex:

*mutex.lock().unwrap() = value;

People might not realize that assignment will call the destructor of the old value. So if the value have non-trivial destructors, we might want to run the destructor without locking the mutex, it could be better to write:

// Release lock first.
let old_value = mem::replace(&mut *mutex.lock().unwrap(), value);

// Drop value later.
drop(old_value);

It is not easy to notice the hidden cost of the first one, and even if people realize that the second way is better for performance, they may still choose the first one because it is easier to write. By providing a Mutex::set method that implements the second behavior, it could be more easy for people to write pragmatic codes.

Solution sketch

Add these methods to the standard library:

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

    pub fn set(&self, value: T) -> Result<(), PoisonError<T>> {
        if mem::needs_drop::<T>() {
            self.replace(value).map(drop)
        } else {
            match self.lock() {
                Ok(mut guard) => {
                    *guard = value;

                    Ok(())
                }
                Err(_) => Err(PoisonError::new(value)),
            }
        }
    }

    pub fn replace(&self, value: T) -> Result<T, PoisonError<T>> {
        match self.lock() {
            Ok(mut guard) => Ok(mem::replace(&mut *guard, value)),
            Err(_) => Err(PoisonError::new(value)),
        }
    }

    pub fn force_get(&self) -> T
    where
        T: Clone,
    {
        (*self.lock().unwrap_or_else(PoisonError::into_inner)).clone()
    }

    pub fn force_set(&self, value: T) {
        if mem::needs_drop::<T>() {
            self.force_replace(value);
        } else {
            *self.lock().unwrap_or_else(PoisonError::into_inner) = value;
        }
    }

    pub fn force_replace(&self, value: T) -> T {
        mem::replace(&mut *self.lock().unwrap_or_else(PoisonError::into_inner), value)
    }
}

impl<T> RwLock<T> {
    pub fn get(&self) -> Result<T, PoisonError<()>>
    where
        T: Clone,
    {
        match self.read() {
            Ok(guard) => Ok((*guard).clone()),
            Err(_) => Err(PoisonError::new(())),
        }
    }

    pub fn set(&self, value: T) -> Result<(), PoisonError<T>> {
        if mem::needs_drop::<T>() {
            self.replace(value).map(drop)
        } else {
            match self.write() {
                Ok(mut guard) => {
                    *guard = value;

                    Ok(())
                }
                Err(_) => Err(PoisonError::new(value)),
            }
        }
    }

    pub fn replace(&self, value: T) -> Result<T, PoisonError<T>> {
        match self.write() {
            Ok(mut guard) => Ok(mem::replace(&mut *guard, value)),
            Err(_) => Err(PoisonError::new(value)),
        }
    }

    pub fn force_get(&self) -> T
    where
        T: Clone,
    {
        (*self.read().unwrap_or_else(PoisonError::into_inner)).clone()
    }

    pub fn force_set(&self, value: T) {
        if mem::needs_drop::<T>() {
            self.force_replace(value);
        } else {
            *self.write().unwrap_or_else(PoisonError::into_inner) = value;
        }
    }

    pub fn force_replace(&self, value: T) -> T {
        mem::replace(&mut *self.write().unwrap_or_else(PoisonError::into_inner), value)
    }
}

Alternatives

• Use third party crates like: AtomicCell, but it only supports loading from Copy types. Also people might not want to introduce third-party dependencies.
• Write extension traits MutexExt and RwLockExt to implement the methods above.

Links and related work

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.

[joshtriplett]

We discussed this in today's @rust-lang/libs-api meeting.

We were in favor of accepting the get, set, and replace methods, and thought these would be quite useful.

We don't want to accept the force_ variants, because we'd rather solve that problem via #169 . (We'd welcome an implementation of that ACP.)