fix: improves panic messages when there is no reactor/timer #2145
Conversation
tokio/src/time/error.rs
Outdated
| @@ -64,7 +64,9 @@ impl fmt::Display for Error { | |||
| fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { | |||
| use self::Kind::*; | |||
| let descr = match self.0 { | |||
| Shutdown => "timer is shutdown", | |||
| Shutdown => { | |||
| "the timer is shutdown, this code must be executed from within a tokio runtime" | |||
There was a problem hiding this comment.
This message is great, I think though this is actually the place where we want to add the panic message as well https://github.com/tokio-rs/tokio/blob/master/tokio/src/time/driver/handle.rs#L24
I am also curious if this is something we could test? maybe via a panic handler?
There was a problem hiding this comment.
Hey @LucioFranco - I've added the other panic, and also updated the error messages to match a similar panic code I found elsewhere in the code base. There are now 4 panic messages that include must be called from the context of Tokio runtime
I also added one test case for when there is no timer, but I'm having a hard time reproducing the other panic messages. I think the tests should look very similar to the one I have written, I just can't seem to speak the magic incantation to get it to actually panic that way.
EverlastingBugstopper
force-pushed
the
avery/fix-panic-messages
branch
from
d480598
to
29a6b0a
Compare
tokio/tests/no_rt.rs
Outdated
| let _ = timeout(dur, rx).await; | ||
| } | ||
|
|
||
| // #[test] |
There was a problem hiding this comment.
I assume we can remove this commented out test?
There was a problem hiding this comment.
we can - i just included it for now to show my thought process for such a test. like i said above - not quite sure how to test the other panic types, only one i could get was a timer panic
There was a problem hiding this comment.
Ah yup, sorry I totally missed that and went to the code right away!
So I think it should be possible to catch the io driver panic if you try to tcp connect? If not I am fine not testing all of them for now. If that is the case id like to remove the commented out test then we can merge this. Thanks!
EverlastingBugstopper
force-pushed
the
avery/fix-panic-messages
branch
2 times, most recently
from
e68f43b
to
058121e
Compare
EverlastingBugstopper
force-pushed
the
avery/fix-panic-messages
branch
from
058121e
to
fbc3439
Compare
|
Ready to go! |
This PR replaces #1789
I want to preface this by saying I'm not entirely sure if these error messages are exactly accurate but I wanted to take a crack at it.
Motivation
Providing a more actionable error message when tokio expects to be within a runtime. Fixes #1713
Solution
New error messages:
When there is no timer:
When there is no reactor:
I'm not in love with the "this code must be executed from within a tokio runtime", not sure what the best wording there is, would love some pointers :) I'm also not sure if it's entirely accurate to say "tokio runtime" as perhaps some other runtime could be used instead.