2

I am using Rust Axum as my web framework.

I am looking to use do mutable shared state in my route handlers. I came across this question:

https://github.com/tokio-rs/axum/discussions/629

which pointed to this example:

https://github.com/tokio-rs/axum/blob/dea36db400f27c025b646e5720b9a6784ea4db6e/examples/key-value-store/src/main.rs

I am concerned about the unwrap() at various places when reading and writing to the state:

let db = &state.read().unwrap().db;

state.write().unwrap().db.insert(key, bytes);

I thought unwrap() was a bad practice?

I also feel like there really won't be any situation (that I can think of) when the unwrap may fail. But maybe I am missing something? In what scenario would this unwrap fail?

Is it safe to use unwrap() in the above example context?

sudoExclaimationExclaimation
  • 7,992
  • 10
  • 47
  • 105
  • 1
    Please provide code directly instead of links to external sites. – Aleksander Krauze Aug 02 '23 at 09:34
  • @AleksanderKrauze I did provide the relevant code in my question. It's the `let db = &state.read().unwrap().db; state.write().unwrap().db.insert(key, bytes);` – sudoExclaimationExclaimation Aug 02 '23 at 09:37
  • 1
    In general when you panic in a handler the http connection with the client will be disrupted and no http response will be returned, however the hyper server will continue to run and accept new connections. You can use [CatchPanic](https://docs.rs/tower-http/0.4.3/tower_http/catch_panic/struct.CatchPanic.html) middleware from `tower-http` crate to catch those panic and turn them into `500 Internal Server Error` responses. – Aleksander Krauze Aug 02 '23 at 09:38
  • When you ask whether this is safe, do you mean as in [safe vs unsafe](https://doc.rust-lang.org/nomicon/meet-safe-and-unsafe.html), or just if it's sound? – jthulhu Aug 02 '23 at 14:59
  • @jthulhu The latter - whether it's sound. – sudoExclaimationExclaimation Aug 02 '23 at 23:14
  • If it doesn't contain unsafe code, it it always sound. I assume you mean whether it is good practice, i.e. won't panic? – Chayim Friedman Aug 03 '23 at 11:36

1 Answers1

1

TL;DR: Yes, it is fine to unwrap() on Mutex and RwLock locking.

std's Mutex and RwLock implement poisoning: if a thread panics while holding the lock (write lock for RwLock), the lock become poisoned. Any further attempt to lock will return Err(PoisonError). This is why the locking operations return Result.

The reason for that is that data is assumed to have invariants that should be always held. Code may break the invariants temporarily, and restore them after that. But if the code unexpectedly panics, the invariants may remain broken, and further code that will inspect the data may see it in an invalid state and produce wrong results (or panic). This is the same reason as with the existence of the unwind safety traits.

Normally, this is not a problem, because if the code panics, the code that called it will also panic (unwind or abort) and the data will be completely inaccessible. But there are two cases where it is a problem: when you catch the panic (the abovementioned unwind safety), and when you access the data from a different thread, because a thread that panics won't cause another thread to panic (in unwind mode). This is why the locks are poisoned in this case, to prevent you from seeing the broken data.

Sometimes, you want to deal with the panic and restore the invariants or exit gracefully. But most of the time, when an other thread panics and made the data junk, you want to panic too without accessing it. This is what the unwrap() does: it panics in this case.

Since it only panics if another thread panicked while holding the lock, it can only panic if there was already a panic. So even thought panics are generally discouraged in Rust, in this case they are fine.

Chayim Friedman
  • 47,971
  • 5
  • 48
  • 77
  • would the panic crash the entire app? For example, I am using the Axum framework and using their `Extension` to provide mutable state. Would a panic in one thread crash my entire app? – sudoExclaimationExclaimation Aug 04 '23 at 07:16
  • 1
    @sudoExclaimationExclaimation If it's in the main task, yes; but a panic on a task won't crash the entire app, and axum spawns a task for each request. – Chayim Friedman Aug 04 '23 at 12:05