Looks like the author missed my main complaint about Rust mutexes, which is that the lock method returns a Result. There should be a try_unlock method for when someone actually wants to handle the rather obscure failure case, and the name lock should be used for a method that panics on failure but returns a value that doesn’t need to be unwrapped first. I see the current arrangement as being about as sensible as having array subscripting return a Result to handle the case of a failed bounds check.
I kind of disagree here. .lock() has the following behavior:
panic() if the lock is already held by this thread - should never happen
error - if the current lock holder paniced
The second case is incredibly rare, so it’s one of the few cases where I think .unwrap() makes sense in production code. But it should be an option to handle it in robust code that should never go down. This is rare, but it’s not so rare that we should force all locks to exist in a context where we can recover from panics.
.try_unlock() should never exist because there should only be one way to release a lock: drop(). Having a way to maybe unlock a mutex adds a ton of issues. If we assume this was a typo, .try_lock() absolutely exists, and it’s for a non-blocking lock.
try_lock already exists; it’s called lock. I just want a more convenient name and I want the name of the new method to be lock, but that ship has sailed.
I think a better solution would be to add a method called something like ulock that does a combined lock and unwrap.
My concern with lock+unwrap is only partly because of convenience; I also didn’t like it because I think it’s a bad idea to get people used to casually calling unwrap, because it tends to hide inadequate error handing.
Now that I think about it, I don’t like how unwrap can signal either “I know this can’t fail”, “the possible error states are too rare to care about” or “I can’t be bothered with real error handing right now”. In one or two of those cases you want to leave it in my production code, and in the last you want to audit all instances and replace them with proper error handing. Using the same function for all three cases makes that difficult.
a better solution would be to add a method called something like ulock that does a combined lock and unwrap.
That’s exactly what’s done above using an extension trait! You can mutex_val.ulock() with it!
Now that I think about it, I don’t like how unwrap can signal either “I know this can’t fail”, “the possible error states are too rare to care about” or “I can’t be bothered with real error handing right now”.
That’s why you’re told (clippy does that i think) to use expect instead, so you can signal “whatever string” you want to signal precisely.
Exactly! My code has a handful of “expect()” calls in it, and each one self-documents why it’s okay. It’s like a comment, but it appears in logs if it ever triggers.
Best practice when using .unwrap() in production code is to put a line of documentation immediately above the use of .unwrap() that describes the safety invariants which allow the unwrap to be safe.
Since code churn could eventually cause those safety invariants to be violated, I think it’s not a bad thing for a blunt audit of .unwrap() to bring your attention to those cases and prompt to reevaluate if the invariants are still satisfied.
Looks like the author missed my main complaint about Rust mutexes, which is that the
lockmethod returns aResult. There should be atry_unlockmethod for when someone actually wants to handle the rather obscure failure case, and the namelockshould be used for a method that panics on failure but returns a value that doesn’t need to be unwrapped first. I see the current arrangement as being about as sensible as having array subscripting return aResultto handle the case of a failed bounds check.If lock-ergonomicsⓒ is as relevant to you as indexing, you’re doing it wrong.
I would rather take indexing returning
Results than the other way around.One can always wrap any code in
{||{ //.. }}()and use question marks liberally anyway (I call them stable try blocks 😉).Just use the Mutex from the parking_lot crate.
I kind of disagree here.
.lock()has the following behavior:panic()if the lock is already held by this thread - should never happenThe second case is incredibly rare, so it’s one of the few cases where I think
.unwrap()makes sense in production code. But it should be an option to handle it in robust code that should never go down. This is rare, but it’s not so rare that we should force all locks to exist in a context where we can recover from panics..try_unlock()should never exist because there should only be one way to release a lock:drop(). Having a way to maybe unlock a mutex adds a ton of issues. If we assume this was a typo,.try_lock()absolutely exists, and it’s for a non-blocking lock.try_lockalready exists; it’s calledlock. I just want a more convenient name and I want the name of the new method to belock, but that ship has sailed.if you’re really that bothered…
use std::sync::{Mutex, MutexGuard}; trait ULock<'a> { type Guard; fn ulock(&'a self) -> Self::Guard; } impl<'a, T: 'a> ULock<'a> for Mutex<T> { type Guard = MutexGuard<'a, T>; fn ulock(&'a self) -> Self::Guard { self.lock().unwrap() } }or use a wrapper struct, if you really really want the method to be called exactly
lock.I think a better solution would be to add a method called something like ulock that does a combined lock and unwrap.
My concern with lock+unwrap is only partly because of convenience; I also didn’t like it because I think it’s a bad idea to get people used to casually calling unwrap, because it tends to hide inadequate error handing.
Now that I think about it, I don’t like how unwrap can signal either “I know this can’t fail”, “the possible error states are too rare to care about” or “I can’t be bothered with real error handing right now”. In one or two of those cases you want to leave it in my production code, and in the last you want to audit all instances and replace them with proper error handing. Using the same function for all three cases makes that difficult.
That’s exactly what’s done above using an extension trait! You can
mutex_val.ulock()with it!That’s why you’re told (clippy does that i think) to use
expectinstead, so you can signal “whatever string” you want to signal precisely.Exactly! My code has a handful of “expect()” calls in it, and each one self-documents why it’s okay. It’s like a comment, but it appears in logs if it ever triggers.
Best practice when using
.unwrap()in production code is to put a line of documentation immediately above the use of.unwrap()that describes the safety invariants which allow the unwrap to be safe.Since code churn could eventually cause those safety invariants to be violated, I think it’s not a bad thing for a blunt audit of
.unwrap()to bring your attention to those cases and prompt to reevaluate if the invariants are still satisfied.