-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Mutex::with #61976
Add Mutex::with #61976
Conversation
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/libstd/sync/mutex.rs
Outdated
pub fn with<U>( | ||
&self, | ||
func: impl FnOnce(&mut T) -> U, | ||
) -> Result<U, PoisonError<MutexGuard<'_, T>>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would the PoisonError contain the guard in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was making it as similar to LockResult
as possible, with the same error type. I guess the rationale is that if you have a poisoned lock, and you can actually recover from that, then having the guard would allow you to do so. On the other hand, that's at odds with point of this API, which is to be simple and low fuss.
I think the alternatives are:
- Use
PoisonError<()>
- Don't return
Result
at all, and just panic on a poisoned lock
Since I think poisoned locks are unrecoverable in practice, 2. is possibly justifiable, but it seems a bit rude for a libstd function to do that. And as far as 1. goes, if we're going to return an error anyway, it may as well have the guard in it, even if its never used in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a try_with
variant for the non-panicing version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with
panics, try_with
returns a Result
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated to include try_with
, but TBH I think it is overkill. I don't think this
match mx.with(|v| *v += 1234) {
Ok(()) => (),
Err(err) => ... handle err ...,
is any improvement over this:
match mx.lock() {
Ok(lk) => *lk += 1234,
Err(err) => ... handle err ...,
}
src/libstd/sync/mutex.rs
Outdated
/// assert_eq!(*mutex.lock().unwrap(), 1); | ||
/// ``` | ||
#[unstable(feature = "mutex_with", issue = "61974")] | ||
pub fn with<U>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub fn with<U>( | |
pub fn with<U, F: FnOnce(&mut T) -> U>( |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@jsgf Yep. |
I added I also added |
Tho I'm inclined to drop the try variants - we can add them later if they turn out to be useful. |
I would definitely like feedback about whether to include the |
@rust-lang/core Hello from triage, can this PR have another reviewer? Thanks. |
Please don't ping core from triage. @rust-lang/libs is who you want (Shouldn't this PR come with an RFC? I guess it's fine if it's unstable but we usually RFC those) |
@Manishearth As I understand it, standard practice for most libs features is to do it by PR if 2-3 people agree its a good idea and maybe a libs team reviewer for a larger change or if there are design questions. (unless it has lang implications...) |
|
Reading the current state of the PR, I'd agree with @Amanieu and add that I found the implementations of the functions added here to be pretty surprising in terms of how they handled poisoning and such. The functions seem very small and not really adding much benefit over calling |
I agree with the criticism of I proposed plain And sure its just a deref away, but its a lot of extra noise. Something like:
is a lot cleaner than
|
Helper which makes taking a `Mutex` for the duration of a single closure lower friction. Issue rust-lang#61974
Rebased; removed confusing try_ variants, just leaving basic with/with_read/with_write. |
That's a good point about dereferencing the guards returned. I feel like this API as-is would be reasonable if we didn't have poisioning, but with mutexes that support poisoning it seems out of place as one of the only APIs that doesn't operate with a result. If you do indeed return a result then the comparison between the two APIs I think is much less compelling: let res = mutex.with(|inner| undulate(inner)).unwrap();
// ... vs ...
let res = mutex.map(|inner| undulate(&mut *inner).unwrap(); I don't think anyone will argue that the latter is more ergonomic, but I'm not sure it's more ergonomic enough to warrant a new API. |
Yeah. I was trying to find any examples of anyone using poison for anything other than unwrap/expect, but couldn't find any. This diff is basically treating poison as legacy API cruft. |
If that's the case I don't think that can really happen on this PR. I don't think we should add a one-off API that takes poisoning as API cruft, but we can of course have a discussion about poisoning in general and how to handle it. |
Hello from triage |
I think we're at an impasse. I think this is useful on its own terms, but it does introduce a new policy for handling poisoned locks (always panic rather than giving the caller an option to handle it). So I think the best option for now is to mothball it and revisit later on. |
Helper which makes taking a
Mutex
for the duration of a single closure lower friction.Issue #61974