Enforce manual drop for locked mutex

Is there a way to enforce an explicit drop for each locked mutex in my codebase?

The Rust language itself doesn't provide this, but it's possible that clippy has a lint for it that you can enable.

yep I was looking in clippy but didn't find it. Are you aware about a related clippy lint?
Thanks

No, I don't know if clippy has such a lint.

What about structuring your API so that it is obvious how long the lock is held for?

struct MyMutex<T>(Mutex<T>);

impl<T> MyMutex<T> {
  fn lock<F, Ret>(&self, thunk: F) -> Ret 
  where
    F: FnOnce(&mut T) -> Ret,
  {
    let mut lock = self.0.lock();
    let return_value = thunk(&mut *lock);
    drop(lock);
    return_value
  }
}

fn main() {
  let mutex = MyMutex::new(42);

  let new_value: i32 = mutex.lock(|n| { *n += 1; *n });

  println!("Updated to {}", new_value);
}

Phrased this way, the end of the closure acts as your "explicit drop".

5 Likes

That seem like a viable solutions. I will try to apply it and let you know, I wonder why this is not the standard API?

It's less flexible. The current API lets you do stuff like return the guard from a function, and also in general the borrow checker is simply more flexible when there is no function boundary between things, and that API would introduce such a boundary.

2 Likes

One of the differences is that in the closure version, your lock is held for the duration of the function call, which is defined at compile-time. However, if I pass around a lock guard it can live for as long or short as I want, and that lifetime can change at runtime - kinda like &'a T vs Rc<T>.

You can also implement the closure-based form using a lock guard, but not the other way around. You could even create your own extension trait and implement it for Mutex, RwLock, and RefCell if you wanted.

I'm curious about the underlying motivation, though. Do you want to enforce explicit drops to help avoid deadlocks?

1 Like

Yep that is the only reason. I know that is not sufficient but it would be one less thing to review.

I generally find the closure-based API good in many other cases. For example, in async code there's a whole bunch of trouble with mutex guards and holding them across .await points, and you can completely avoid that with a closure-based API since you simply can't .await inside the closure.

1 Like

I think that I will try introduce the closure API using a struct that impl one method lock() that is the standard API and another method safe_lock() that is the closure one.
That for 3 reason:

  1. it is and improvement cause now I can "ignore" safe_lock()
  2. keeping standard lock() let me avoid to updated all the code base in one time
  3. I'm worried by the fact that for how is structured the code somewhere it would be impossible to use safe_lock()

What about creating an extension trait that gets implemented for std::sync::Mutex, std::sync::RwLock, and std::sync::RefCell?

trait LockExt {
  fn with_lock<F, R>(&self, thunk: F) -> R;
}

impl<T> LockExt for std::sync::Mutex<T> {
  fn with_lock<F, Ret>(&self, thunk: F) -> Ret 
  where
    F: FnOnce(&mut T) -> Ret,
  {
    ...
  }
}

That way you can pass around a normal Mutex which everyone knows, and in places where you feel like the closure-based method would be more readable, you use that.

2 Likes

Here's the full thing including the implementation :slight_smile: (and after fixing some minor typos/oversights)

trait LockExt<T> {
    fn with_lock<F, Ret>(&self, thunk: F) -> Ret
    where
        F: FnOnce(&mut T) -> Ret;
}

impl<T> LockExt<T> for std::sync::Mutex<T> {
    fn with_lock<F, Ret>(&self, thunk: F) -> Ret
    where
        F: FnOnce(&mut T) -> Ret,
    {
        thunk(&mut self.lock().unwrap())
    }
}

Actually, maybe an associated type is nicer:

trait LockExt<T> {
    type Value;
    fn with_lock<F, Ret>(&self, thunk: F) -> Ret
    where
        F: FnOnce(&mut Self::Value) -> Ret;
}

impl<T> LockExt<T> for std::sync::Mutex<T> {
    type Value = T;
    fn with_lock<F, Ret>(&self, thunk: F) -> Ret
    where
        F: FnOnce(&mut T) -> Ret,
    {
        thunk(&mut self.lock().unwrap())
    }
}
3 Likes

At the end I used a struct as I prefer make sure that the users (mostly but not only myself) use my particular Mutex. Then if possible I would like to remove lock(). The project is very early stage so I can do whatever I want.

The only reason for why I still have lock() is that I need, in few places, to await and keep the guard locked.

I really hope you're using an async mutex then, because otherwise that's going to deadlock.

2 Likes

I use both: async one define safe_lock() and lock(), standard one already implement only safe_lock()
I should have specified that, sorry.

I ended up replacing all the asynchronous Mutexes with the sync one that implement safe_lock. Cause I can be sure with safe_lock to not hold the guards between awaits points. Now everything is more clear. Very useful!

1 Like