Mutex lock() error checking

help
1

Looking over some synchronous code of mine I am wondering about my bare Mutex .lock().unwrap()s and thinking I should turn them into something more, um, respectable. I see at least one question here about recovering from poisoned mutexes, but nothing on the best way to just blowup. Maybe get fancy with .unwrap_or_else() and a closure that panics with a custom message?

If nothing else, I feel like t would be good to have an .expect() with some message as a marker that I considered the point.

Or is a better to just leave the .unwrap()s as-is?

Thanks,

-kb

2

I've seen .lock.expect("poisoned") which seems reasonable since poisoning is the only reason for an error return. But I don't know how useful this is for debugging, vs just unwrap(), as I've never experienced a poisoned mutex.

1 Like
3

Thanks,

-kb

1 Like
4

Adding a message isn't necessary for debugging, as the unwrap() will print PoisonError {} or something similar. But it might help readers of the code immediately understand when the code would panic.

6 Likes
6

My worry is the quick way to write Rust is:

  • Call a function.
  • Get back a Some()
  • .unwrap()
  • forget about the None possibility,
  • move on!

So when I look at my code and I see an .unwrap() I worry I did that. It makes me want to turn every single .unwrap() into something that convinces me I thought about it. I don't want to accidentally ignore a None that is actually to be quite expected.

That's one worry.

My other worry is if I put .expect()s everywhere it might convince me I considered each case, but then other programmers will look at my code, shake their heads, and…

Part of the risk is, unlike C, Rust is pretty nice about letting me play with something new in just a few lines, with the compiler nicely inferring types and telling me what to do to until my code works. (Take that, Python.) But I worry I will leave behind some expedient .unwrap()s.

What do you think if you see .expect()s everywhere in someone else's code? What is the best way to make sure I don't crash on any Nones?

3 Likes
7

If you can't statically prove to the compiler why something is safe, and yet you don't want to be burdened with the mental load of reviewing some documentation on why it is safe, then your only alternative is to just be okay with the consequences... If panic is not an option, you then you must do something else.

Hard problems are hard. There's no magic to make hard problems go away.

8 Likes
8

One option if you don’t need poisoning is parking_lot::Mutex which does not support poisoning and therefore does not need the unwrap().

6 Likes
9

You're correct that this is generally considered code smell in non-throwaway/example code. But poisoned Mutexes are a special case in that it means something else already panicked, and often you just want to propagate that panic; most programs don't have panic recovery, or only have them at very particular places.

There is other common code which panics on error, perhaps the most common of which are Index and IndexMut implementations.

Alternatively you could ignore all poisoning:

let guard = mutex.lock().unwrap_or_else(|p| p.into_inner());

There are other Mutexes available which don't use poisoning, so you don't have to do that dance -- oop, I see @mbrubeck pointed this out already :slight_smile:.

3 Likes
10

It's a difficult one, like @arifd said. At one point I was using expect instead of unwrap for the reasons you mentioned, but it's just a convention that those reading my code won't know about. So I switched to adding a TODO comment when I unwrap temporarily. That's obvious to everyone, although verbose when prototyping.

One argument for raw unwrap being just fine is that we index arrays and slices all the time with no special indication that they may panic due to a bug. Raw unwrap can be thought of in the same way.

3 Likes
11

You can write your own extension method to give a meaningful name to unwrap, which is easily distinguished when reading the code:

trait IgnorePoisoned<T> {
    fn ignore_poisoned(self) -> T;
}

impl<T> IgnorePoisoned<T> for Result<T, PoisonError<T>> {
    fn ignore_poisoned(self) -> T {
        self.expect("poisoned")
    }
}

fn main() {
    let m = Mutex::new(0i32);
    let guard: MutexGuard<i32> = m.lock().ignore_poisoned();
    dbg!(guard);
}
1 Like
12

IMO this is the wrong mindset. The crash is the symptom, not the cause. If you've got a None where you shouldn't (which btw is not always the case, half of the times it is a reasonable result and you should handle that) then you have a problem in the code that gave you the None and you should fix that. If you don't want to crash/abort the current task in response to a bug in another part of your code then you can only continue with nonsensical values, which IMO is even worse because it makes debugging harder while still not doing the right thing.

6 Likes
13

Sorry if I was unclear, I'm not trying to avoid a panic on a .lock() that failed, I'm trying to avoid panicking on a None that I know how to handle but forgot about.

I like having the Rust compiler tell me where I have messed up, and telling me when I ignored a case is part of that.

If I do a match I need to explicitly put in a catch-all branch if I want a catch-all, or the compiler says no. And if I, or someone else, is looking at the code every place where there is a catch-all branch is a place to wonder why.

Enter .unwrap(). If it is normal to have them on lock()s where I want to blow up if it fails (because I have a runtime bug), then the places where I mistakenly put one on a Some/None are places where I have turned a compile-time bug into a runtime bug, but they are harder to spot than they should be.

To say the solution is to not make that mistake reminds me of programming in Python or C.

This is something the compiler is capable of warning me about but the general-purpose .unwrap() seems too liberal (works for Results and Options) because it defeats these checks, something that should be in example code, not production.

I'm thinking I want to write myself a .lock_or_panic(), to make clear that I want to panic on a failure, make the code a bit shorter, and not be something I can accidentally slap on a None and have the compiler shrug and say "That's what you asked for, we can blow up later, that's okay.".

-kb, the Kent who wants every bug that can be a compile time bug to be a compile time bug.

14

The Mutex lock is somewhat of a weird case because it is rare to not unwrap, so writing a lock_or_panic extension method is fine if that helps you when reading code. But it sounds like you have a bigger complaint about unwrap in general:

The compiler is doing everything it can: it gives an error if you assign an Option<T> to a T. If you think you have a Some(T), and calling unwrap is not explicit enough for you because it's used in lots of different places, then use expect with a clear message, or a match. It's up to you how explicit you want to be, the tools are there. There are also clippy lints to enforce different rules you may want, for example unwrap_used.

2 Likes
15

This seems to work:

pub trait LockOrPanic<T> {
    fn lock_or_panic(&self) -> MutexGuard<T>;
}

impl<T> LockOrPanic<T> for Mutex<T> {
    fn lock_or_panic(&self) -> MutexGuard<'_, T> {
        if let Ok(x) = self.lock() {
            x
        } else {
            panic!()
        }
    }
}

Now I can do some_mutex.lock_or_panic() and not put in any scary .unwrap()s. (Similar works for Condvar and RwLock.)

Thanks to all,

-kb

1 Like
16

If you have static guarantees about your types and most of the work to construct them can be done at compile-time, a [very complex] build script might be just the thing for you.

For example, if you need to read an image file that you know will always be there, you can include_bytes!() it into the executable to remove all file I/O and its associated error handling. If you know its format is always PNG, you can decode it to raw pixels in a build script and avoid not only the I/O, but also any potential decoding errors! Now you have a fully decoded image ready to use without any need to unwrap at all.

It can be a structured type, too! Not just raw bytes. The build script can write a Rust module that inlines the entire type construction, either as a static allocation or as a constructor function.

I discussed this technique in Discussion: is cargo really a package manager? - #15 by parasyte

There are limitations to what can be known at compile-time. For instance, parsing a string that you receive from a socket is going to require runtime error handling. Not just for the parsing, but also the I/O errors that potentially occur when a network is involved. For these cases, you only have two options: Unwrap early or use ? to propagate the sad path to a single location that knows how to deal with None or Err.

2 Likes
17

I would probably do this inside a function that returns Result<T, Error>, so I would use ? and possibly wrap the Error into another Error for more context.

18

About "more context": I have been frustrated that it is hard to see what context I get for free—simulating these errors isn't as easy as I anticipated. Likely I could improve the information that appears if they do happen.

But anyway, I have satisfied my initial want of getting rid of all of my .unwrap() and .expect() calls, because I have decided they are both evil in production code; they are both far too convenient a way to move bugs from compile time to runtime.

There is the perennial question of when to return an error and when to panic.

Certainly a library that is passed bad parameters should return an error that will be useful in the context of the application code that called it, but in my code I think these specific error cases all mean something very bad and unplanned has gone wrong in the program logic. Probably a different thread died over there and and over here I have no idea why that should have happened, so I want to crash nice and loud and quickly, as close to the discovery of the problem as possible, and do it in testing if possible. (My latest change was probably a bad one…)

This is also why I like the idea of asserts of the obvious to try to catch bad code changes as quickly as possible.

Were I doing critical embedded code, things would be different, as panics are very bad. Avoid such bugs! But what if there is a bad logic bug where there is no useful recovery? In those cases any "panic" should probably trigger larger restarts in the system and not merely log an error and halt. (In addition there should be also be watchdog timers to restart dead stuff, and eventually we get into the kinds of really interesting engineering that goes into space probes, etc.)

Thanks,

-kb

19

I would suggest reading BurntSushi on Error Handling in Rust and the same author on Using unwrap() in Rust is Okay.

The core of these answers to the question (which I agree with, FWIW) is that a panic, including one from unwrap() or expect(), is a sign that the program has a bug and needs a programmer to debug it. If the problem can be resolved by the caller (which, in some cases, will be the end user of the program, and not another programmer), then it's "return an error" time, using Result or Option as appropriate.

In this model, unwrap, expect, panic!, unreachable!, todo! and all the other myriad ways to induce a panic (e.g. array[idx]) are markers for the human reader; they tell you that while this state cannot be proven impossible via the type system (and thus checked by the compiler), the author believes that this state cannot plausibly happen, and if it does, entering this state means that you need to debug the program.

For the specific case of Mutex::lock(), an error is returned if a thread panicked while holding the lock. There are ways to recover from this, where you re-initialize the contained value (see the example for Mutex::clear_poison) or replace it with a new lock and drop the poisoned lock completely, so there's a case for it being an error.

But if you are asserting that your threads never panic while holding this lock (absent an unknown bug in the program), then lock().expect("poison") is perfectly reasonable; you are saying that lock() can return an error, and if it does, it indicates that the program is in a state that you, as the programmer, believed was impossible. This is a great time to panic - you need a Rust programmer to dive in and find out what has happened to get the program into a state that the original programmer of this code believed was impossible, and fix it.

TL;DR: panic for cases where the only way to proceed is to get a Rust programmer to fix it, error where it's plausible that the caller can do something sensible.

6 Likes
20

Yes, I agree. When a condition implies the code is simply broken a panic is the right thing to do!

The problem with .unwrap() isn't that it can panic, the problem is that it is happy to panic on either an Err or a None, because the two are essentially…the same‽‽

Treating None and Err as interchangeable—in pedantically strongly typed Rust—is wrong. Sure, it is expedient in example or prototype code, but production Rust should not be about expediency.

21

That's two different functions. You can write Result::unwrap(thing) instead of thing.unwrap() if you want to be clear on the difference - this will fail to compile if thing can be None, and will only compile for the Err case.

And there's plenty of other functions in Rust that have the same name, but act on different data types; get, for example.

Also, there is a sense in which None and Err are the same, which pairs nicely with the use of unwrap; if Some(_) or Ok(_) represent "success", then None and Err(_) are their respective data type's "failure".

2 Likes