Add lock! macro to handle mutex poisoning gracefully#471
Conversation
Mutex::lock().unwrap() panics if the mutex is poisoned (when a thread panicked while holding the lock). In practice, this is rarely the desired behavior - the data may still be valid and propagating panics across threads is usually unhelpful. This adds a lock! macro that recovers from poisoning via unwrap_or_else(|e| e.into_inner()), providing a concise and consistent way to acquire locks throughout the codebase. An alternative would be using parking_lot::Mutex which has no poisoning semantics, but this avoids adding an external dependency.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
I'd kinda rather make sure that none of the operations within any of our locks can possibly panic. I believe we're close but you identified one bug where we're holding a mutex for too long.
| // Try to get cached connection | ||
| let conn_opt = { | ||
| let state = self.r#async.lock().unwrap(); | ||
| let state = lock!(self.r#async); |
There was a problem hiding this comment.
I do not believe any of the calls within either lock here can panic at all.
| request: ParsedRequest, | ||
| ) -> Pin<Box<dyn Future<Output = Result<Response, Error>> + Send + 'a>> { | ||
| Box::pin(async move { | ||
| let conn = Arc::clone(&*self.0.lock().unwrap()); |
There was a problem hiding this comment.
ISTM all of the locks for this mutex are only held on one line and used to clone or assign the Arc, I don't believe that can panic?
| }; | ||
|
|
||
| let socket_timeout = *conn.socket_new_requests_timeout.lock().unwrap(); | ||
| let socket_timeout = *lock!(conn.socket_new_requests_timeout); |
There was a problem hiding this comment.
Hmm, good catch I believe this mutex shouldn't be held nearly as long as it is, it should only be required for the next line then should be dropped.
|
Please note development has migrated to https://git.rust-bitcoin.org/rust-bitcoin/corepc. Any further comments or pushes here on github may be ignored or lost. Please consider containing this PR on the forgejo instance. |
3 participants
Mutex::lock().unwrap() panics if the mutex is poisoned (when a thread panicked while holding the lock). In practice, this is rarely the desired behavior - the data may still be valid and propagating panics across threads is usually unhelpful.
This adds a lock! macro that recovers from poisoning via unwrap_or_else(|e| e.into_inner()), providing a concise and consistent way to acquire locks throughout the codebase.
An alternative would be using parking_lot::Mutex which has no poisoning semantics, but this avoids adding an external dependency.