Add some convenience methods for locks. by mark-i-m · Pull Request #79434 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This PR adds three new methods (under two new features) for Mutex and MutexGuard:

We add MutexGuard::unlock, which is equivalent to drop(guard) but more self-documenting. This method is useful for explicitly cutting short the duration for which a lock is held. It is often helpful to do this to shorten critical sections or avoid deadlocks.
We add Mutex::with and Mutex::try_with, which accept closures and run the closures with the lock held, dropping the lock immediately after the closure returns. These methods are useful for running critical sections in a syntactically clear way and avoiding holding the lock for longer than needed.

This is my first PR adding a std feature, so please let me know if something is missing.

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

The return types of with and try_with feel a bit odd to me. In case the mutex was poisoned, the guard is not dropped right after the call, but returned inside the PoisonError instead, which is very different than the non-poison behaviour where it drops it right after calling the closure.

I'm not sure what a good alternative would be, though. Sending the PoisonError into the closure is also not great.

Any thoughts about this?

Comment on lines +518 to +521

#[unstable(feature = "guard_unlock", issue = "none")]
pub fn unlock(self) {
drop(self);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MutexGuard doesn't have any methods right now, because it implements Deref. If the T already has an .unlock(), this will make it harder to call that .unlock() through this MutexGuard. Any existing code calling such an .unlock() would change meaning with this change.

Other types like Box solve this by not taking self in their methods (e.g. Box::into_raw), and must be called with the more verbose Box::into_raw(mybox) syntax.

But I think MutexGuard::unlock(guard); wouldn't be a big improvement over drop(guard);. :(

What do you think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I had not thought of that. I guess the primary difference in my mind was to make the code a bit self-documenting. It is not obvious to me when I see drop(foo) that a lock might be unlocked. I'm ok with the more verbose syntax, personally, but if we are going to do that perhaps we could do it like this instead:

impl Mutex { pub fn unlock(guard: MutexGuard) { drop(guard); } }

Then, one would use it like

which seems about as self-documenting as it gets...

The return types of with and try_with feel a bit odd to me. In case the mutex was poisoned, the guard is not dropped right after the call, but returned inside the PoisonError instead, which is very different than the non-poison behavior where it drops it right after calling the closure.

I can think of 3 options:

Accept this asymmetry (and keep the current API)
Return an error indicating that the lock was poisoned, but don't return a PoisonError. Instead, users that want to address the error would need to try locking using the normal lock method. This seems less ergonomic to users that actually do want to handle the error, but the API is symmetric.
Pass a LockResult into the closure. This seems much less ergonomic, as it probably forces the user to either unwrap in the closure or have the closure return Result.

On balance, I think I still prefer the current API with an explicit note in the documentation. This is mostly based on my own experience that often PoisonError just result in a panic somewhere, and I think those who care would likely be more careful anyway. But I think the second option could also be reasonable.

@mark-i-m Thanks for your detailed comment.

I'm not sure which of these I'd prefer. All of them feel like they do not fit the current mutex api very well. :(

This problem wouldn't exist if mutexes didn't get poisoned. There's been some discussion about adding new locks in std that don't have the concept of poisoning, just like those in parking_lot. For those, with and try_with would have a more straightforward interface.

I'm not sure what the status of that is, but I've put it on the agenda for the libs team meeting in two days.

@m-ou-se Any updates? I saw the survey on the blog, and I assumed that was related.

We've discussed this PR in the libs team meeting a few days ago. In this meeting we agreed:

Mutex::unlock(..) could be a good addition to try out. If it works well, we could even warn about drop(mutex_guard) at some point.
with and try_with are probably not worth it, considering 1) it provides another way of doing the same thing, which might be confusing (see also lock_api: Add a with(&self, closure) method to Mutex? Amanieu/parking_lot#216 (comment)), and 2) the signature is not going to be great w.r.t. poisoning, regardless of which way it handles it. Whether we should consider these functions for a possible future mutex that doesn't have poisoning is a separate question, but might not be worth it either. (That parking_lot issue is relevant, as parking_lot does not have poisoning.)

Feel free to open a tracking issue for Mutex::unlock. It'd be good to add a note about about RwLock::unlock() (which would be a bit more complicated) to the unresovled questions on that issue.

m-ou-se added S-waiting-on-author