Add Mutex::unlock

[mark-i-m]

Tracking issue: #81872

Discussion: #79434 (comment)

r? @m-ou-se

[the8472]

Is this really worth it? An explicit call to drop would leave the small footgun that if there's some other guard acquired after the lock that you want to drop inside the critical section then you have to explicitly call drop() before the explicit unlock. So it's not that much of an ergonomic improvement.

If we want a no-worries convenience method wouldn't a scope-based one be more appropriate? Something like with_lock(scope: impl Fn()).

[mark-i-m]

@the8472 The point of the PR is not ergonomics. Rather it is readability and performance -- readability because unlock is more self-documenting than drop or the end of a scope, and performance because sometimes you want to keep your critical sections as short as possible.

FWIW, I agree with you about some sort of Mutex::with, and the idea has been proposed a few times before (include by me), but the libs team doesn't feel that it is worth it because one can just create a syntactic scope:

{
  let locked = foo.lock().unwrap();
  ...
} // drop lock

I feel like the value of explicitly demarcating the critical section is being underestimated, but I don't really have any new arguments to make...

[tavianator]

Would it be a bit nicer as a method on MutexGuard? I.e. guard.unlock() instead of Mutex::unlock(guard)?

[mark-i-m]

@tavianator That's how I originally implemented it, but the problem is that MutexGuard implements Deref so that you can transparently call methods of the underlying T. If we add an unlock methed to MutexGuard, it breaks all code where T has a method named unlock.

[m-ou-se]

FWIW, I agree with you about some sort of Mutex::with, and the idea has been proposed a few times before (include by me), but the libs team doesn't feel that it is worth it because one can just create a syntactic scope:

I feel like the value of explicitly demarcating the critical section is being underestimated, but I don't really have any new arguments to make...

The main problem with such a with function is that there's no good way to handle the poisoned state.

[mark-i-m]

The main problem with such a with function is that there's no good way to handle the poisoned state.

@m-ou-se I don't wish to derail this thread, so feel free to point me elsewhere, but I'm curious what is wrong with:

impl<T> Mutex<T> {
  pub fn with<R, F: Fn(&mut T) -> R>(&self, f: F) -> Result<R, PoisonErr> {
    self.lock().map_ok(f)
  }
}

/// ...

foo.with(|locked| locked.bar()).unwrap();

This seems roughly like what I actually want to do with a lock usually...

[m-ou-se]

I'm curious what is wrong with

The problem is that the lock would still be locked after that function returns in the case of a poisoned lock, as the PoisonError will contain the guard. That'd defeat the purpose of making it clear when the lock is and isn't borrowed. The alternatives of ignoring poison or always panicking on poison are not great either.

[m-ou-se]

r=me with the commits squashed.

[the8472]

The problem is that the lock would still be locked after that function returns in the case of a poisoned lock

But the lock should unlock at the end of the with closure anyway. So as the function returns the lock will always be unlocked. The only difference is whether the body executed or was skipped due to poison, which will be handled by the Result.

[m-ou-se]

A PoisonError keeps the lock locked until you process that error.

[the8472]

That'd defeat the purpose of making it clear when the lock is and isn't borrowed.

Not quite. It would still ensure that other drop types made in the critical section get dropped before the lock is released. And it would also keep the lock lifetime shorter than the outer scope in the happy path. Only in the poison case it could live longer than expected.

We could also return an error that drops the guard. That's not exactly ignoring the problem (there's still an error) but the poison will be foisted on next user of the lock.

[m-ou-se]

Feel free to write an RFC if you believe there's a good solution. But there were too many unresolved questions/problems like this to add that as a regular PR, since it wasn't clear at all how any solution compares to the alternatives.

[the8472]

For the scope of this PR it would make sense to add some wording that using this method (or mem::drop for that matter) affects drop order compared to the end-of-scope drop and may lead to things acquired inside the critical section to be dropped after the lock is released.

[mark-i-m]

@m-ou-se Squashed.

I had forgotten that the PoisonErr also continues to hold the lock. Perhaps it would make sense to create a NonBlockingPoisonErr -- similar to how try_lock only returns the lock if it can be acquired and fails otherwise? Or maybe that creates too many variants?

[m-ou-se]

Squashed.

Thanks!

@bors r+ rollup

I had forgotten that the PoisonErr also continues to hold the lock. Perhaps it would make sense to create a NonBlockingPoisonErr -- similar to how try_lock only returns the lock if it can be acquired and fails otherwise? Or maybe that creates too many variants?

Questions like those are exactly why I don't think we can add a function like that in a PR directly, without a good design discussion or RFC first.

[bors]

📌 Commit e92e5fd has been approved by m-ou-se