Skip to content
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 #61974

Closed
jsgf opened this issue Jun 19, 2019 · 4 comments
Closed

Add Mutex::with #61974

jsgf opened this issue Jun 19, 2019 · 4 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@jsgf
Copy link
Contributor

jsgf commented Jun 19, 2019

It would be useful to have a Mutex::with method with the signature:

pub fn with<U>(&self, func: impl FnOnce(&mut T) -> U) -> LockResult<U>

This would be a convenience for quick access to a lock, especially if managing the scope of the MutexGuard would be awkward.

cc @dtolnay

@jsgf
Copy link
Contributor Author

jsgf commented Jun 19, 2019

I guess -> Result<U, PoisonError<MutexGuard<'_, T>>>

@jonas-schievink jonas-schievink added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 19, 2019
@jsgf jsgf mentioned this issue Jun 19, 2019
@skade
Copy link
Contributor

skade commented Jun 20, 2019

I wonder which cases would be made less awkward? Rust currently handles those scopes using lexical scopes and ownership.

I know that the stdlib used to have quite a number of those APIs (e.g. CStr.with_ref in Rust 0.8, others followed), but has moved away from it completely. This seems like a strategy reversal here.

There's a number of cases in the stdlib where scoping could be controlled using a closure, e.g. a File API like

let data = with_file(|f| { 
    let mut buffer = String::new();
    f.read_to_string(&mut buffer);
    buffer
}

would make sure a file is not closed too late.

FWIW: I don't think adding such a method is problematic, but given that the strategy was widespread and then removed merits a longer explanation of problematic cases/motivation.

@jsgf
Copy link
Contributor Author

jsgf commented Jun 20, 2019

I think there's two rationales:

First is that controlling the lifetime of MutexGuards is really important, and failing to do so can lead to subtle problems. For example, holding a lock too long might not lead to a correctness problem, but it could lead to a performance one. I also think you could come up with a deadlock scenario (if there's two locks which are supposed to have non-overlapping regions, but end up overlapping and being taken in an inconsistent order.)

Second is just a code cleanlyness/ergonomics issue. Being able to take a lock in the middle of expression-oriented code is awkward without .with(). It's a lot nicer to embed mutex.with(|v| something(v)) in the middle of a larger expression rather than opening a whole new scope. (Our local version of this simply does .unwrap() rather than exposing the error; I think that's an option here too.)

std::thread::LocalKey has a with() method of this form, so it isn't completely without precedent.

At one point I proposed adding let ... in syntax to open a "little scope" which covers the same territory, but its not really clear that it helps enough to be worth it.

jsgf added a commit to jsgf/rust that referenced this issue Aug 3, 2019
Helper which makes taking a `Mutex` for the duration of a single closure lower friction.

Issue rust-lang#61974
@Mark-Simulacrum
Copy link
Member

The associated PR ended up being closed, and for reasons that are IMO likely to require an RFC: rethinking whether poisoning locks in std is still a good idea. As such, I'm going to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants