Document guarantees of poisoning #144185
Document guarantees of poisoning #144185
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This mostly documents the current behavior of `Mutex` and `RwLock` as imperfect. It's unlikely that the situation improves significantly in the future, and even if it does, the rules will probably be more complicated than "poisoning is completely reliable", so this is a conservative guarantee. We also explicitly specify that `OnceLock` never poisons, even though it has an API similar to mutexes.
purplesyringa
force-pushed
the
poisoning-wording
branch
from
029caf5 to
5b2c61e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
rustbot
added
A-concurrency
rustbot
removed
the
S-waiting-on-review
rustbot
added
the
S-waiting-on-review
| /// An `RwLock`, like [`Mutex`], will usually become poisoned on a panic. Note, | ||
| /// however, that an `RwLock` may only be poisoned if a panic occurs while it is | ||
| /// locked exclusively (write mode). If a panic occurs in any reader, then the | ||
| /// lock will not be poisoned. |
There was a problem hiding this comment.
This should point to the module-level documentation to explain the "usually".
There was a problem hiding this comment.
My PR describes the pitfalls in Mutex docs, rather than module-level docs, since they are specific to Mutex/RwLock and not other poisoning locks. I'll make "usually" point to the "Poisoning" section in Mutex docs instead, if that's okay with you.
| //! providing a way to deal with the poisoned state. | ||
| //! See [`Mutex`'s documentation](Mutex#poisoning) for more. | ||
| //! Panicking while holding the lock typically poisons the mutex, but it is | ||
| //! not guaranteed to detect this condition in all circumstances. |
There was a problem hiding this comment.
It would be good to clarify that poisoning is only unreliable when you have multiple pending panics or foreign exceptions. In particular it works fine in the "normal" case of a single panic.
There was a problem hiding this comment.
I wasn't sure if we wanted to commit to that guarantee. Consider the following situation:
- A panic is thrown.
- A drop guard locks a mutex and stores the guard into a cell. The guard records
panicking: true. - The drop guard exists and the panic is caught and dropped.
- Another panic is thrown.
- Another drop guard loads the guard from the cell and drops it. Since the guard has
panicking: trueand we're currently panicking as well, the mutex is not poisoned.
Yet there's never two active panics at the same time. Only one panic has been thrown while the mutex was locked -- the other panic was only caught.
That's a lot of detail I don't know if I can formalize in a few words in the docs without making a mistake and guaranteeing something we can't provide.
There was a problem hiding this comment.
The problem with your suggested docs is that the semantics are too vague to be of any use. Essentially we're saying that if a panic occurs while a MutexGuard is live then the lock may or may not be poisoned. While it's fine to not make hard guarantees, it would be nice to have a reassuring sentence along the lines of "if you're not doing fancy things like multiple live panics or preserving a MutexGuard across a catch_unwind then it will usually work as you expect".
There was a problem hiding this comment.
Or alternatively give specific examples of cases where poisoning doesn't work properly, with a note that this list isn't exhaustive. This will at least reassure people that poisoning probably works if they're not doing those things.
There was a problem hiding this comment.
Makes sense, I've added a list. Does this look good?
There was a problem hiding this comment.
Perfect! I'd just add another bullet point about foreign exceptions and this should be good to go.
There was a problem hiding this comment.
Totally forgot about that, thanks for the reminder. I added a note.
This mostly documents the current behavior of
MutexandRwLock(#143471) as imperfect. It's unlikely that the situation improves significantly in the future, and even if it does, the rules will probably be more complicated than "poisoning is completely reliable", so this is a conservative guarantee.We also explicitly specify that
OnceLocknever poisons, even though it has an API similar to mutexes.Fixes #143471 by improving documentation.
r? @Amanieu