Fix LazyLock poison panic message
#145307
Conversation
rustbot
added
S-waiting-on-review
connortsui20
force-pushed
the
lazylock-poison-msg
branch
from
e1b7481 to
2aea6c2
Compare
Fixes an issue where if the underlying `Once` panics because it is poisoned, the panic displays the wrong message. Also consolidates all of the `Once` panic messages in one place so that we don't have to edit the message in 6 different files if we want to change it. Signed-off-by: Connor Tsui <[email protected]>
connortsui20
force-pushed
the
lazylock-poison-msg
branch
from
2aea6c2 to
e77c6fc
Compare
|
|
||
| // If the internal `Once` is poisoned, it will panic with "Once instance has previously been | ||
| // poisoned", but we want to panic with "LazyLock instance ..." instead. | ||
| let result = panic::catch_unwind(AssertUnwindSafe(call_once_closure)); |
There was a problem hiding this comment.
catch_unwind can't be used to override the panic message. By the time catch_unwind returns, the panic message has already been printed. In addition catch_unwind would cause an abort in case of a foreign exception.
Maybe you can have an internal method like call_once except with a panic message as argument?
There was a problem hiding this comment.
Hmm that does make sense, but at the same time I think it's fine if the user gets 2 error messages?
I'm a bit worried about the footprint of updating the internal call_once method with a panic message as argument since that would require changing all of the underlying implementations as well. But if that is the only acceptable way then I'm happy to do it
| /// instance has previously been poisoned") instead of the underlying `Once` error message. | ||
| #[test] | ||
| #[cfg_attr(not(panic = "unwind"), ignore = "test requires unwinding support")] | ||
| #[should_panic(expected = "LazyLock instance has previously been poisoned")] |
There was a problem hiding this comment.
The reason this test passes is because #[should_panic(expected = "...")] likely checks the last panic message, not the first.
Fixes the issue raised in #144872 (comment)
r? @Amanieu