-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Document guarantees of poisoning #144185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Document guarantees of poisoning #144185
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,10 +46,10 @@ use crate::sys::sync as sys; | |
/// | ||
/// # Poisoning | ||
/// | ||
/// An `RwLock`, like [`Mutex`], will 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. | ||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should point to the module-level documentation to explain the "usually". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
/// | ||
/// # Examples | ||
/// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if we wanted to commit to that guarantee. Consider the following situation:
panicking: true
.panicking: true
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aMutexGuard
across acatch_unwind
then it will usually work as you expect".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I've added a list. Does this look good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! I'd just add another bullet point about foreign exceptions and this should be good to go.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally forgot about that, thanks for the reminder. I added a note.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Amanieu sorry for pinging -- could I get an r+?