rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.88k stars 12.67k forks source link

Add Mutex::with #61974

Closed jsgf closed 5 years ago

jsgf commented 5 years ago

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 commented 5 years ago

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

skade commented 5 years ago

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 commented 5 years ago

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.

Mark-Simulacrum commented 5 years ago

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.