rt: unhandled panic config for current thread rt #4518
Conversation
Allows the user to configure the runtime's behavior when a spawned task panics. Currently, the panic is propagated to the JoinHandle and the runtime resumes. This patch lets the user set the runtime to shutdown on unhandled panic. So far, this is only implemented for the current-thread runtime. Refs: #4516
| @@ -473,13 +477,20 @@ fn poll_future<T: Future>(core: &CoreStage<T>, cx: Context<'_>) -> Poll<()> { | |||
| let output = match output { | |||
| Ok(Poll::Pending) => return Poll::Pending, | |||
| Ok(Poll::Ready(output)) => Ok(output), | |||
| Err(panic) => Err(JoinError::panic(panic)), | |||
| Err(panic) => { | |||
| scheduler.unhandled_panic(); | |||
There was a problem hiding this comment.
It's only unhandled if the JoinHandle gets dropped, no?
There was a problem hiding this comment.
"unhandled" refers to unhandled by the spawn task. As per #4516, two separate behaviors are to always shutdown if the task panics or to only shutdown if the JoinHandle is dropped. I implemented the first one here.
There was a problem hiding this comment.
FWIW, I think this option will be most useful for tests where the application is expected to gracefully handle all possible user inputs.
Darksonn
added
A-tokio
|
This should be ready to review. |
|
Unless there are objections or known changes that should be made, I'd like to merge this as it is an unstable API. |
|
I added initial support for This raises some questions. One, For example, should |
There was a problem hiding this comment.
overall this looks correct.
one thing i noticed that i'm not the biggest fan of is that unhandled panics are propagated by creating a new panic, rather than continuing the original panic. this feels not great to me, personally --- although the original panic will still be printed out (by the default panic handler) it won't be the last panic printed before the program exited/the test failed.
i understand the reason we don't continue the original panic is that it has to be stored in the task's JoinHandle. but, it seems like we could potentially come up with a scheme where the panic is stored in a cell that can be taken by either the JoinHandle or the runtime's unhandled panic handler, if the joinhandle doesn't consume the panic. this does seem a bit more complex though, and i would be fine with saving something like that for a follow-up PR.
| self.owned.close_and_shutdown_all(); | ||
| } | ||
| } | ||
| _ => panic!("runtime core not set in CURRENT thread-local"), |
There was a problem hiding this comment.
AFAICT, this will only happen in the event of a Tokio bug, so this should probably be
| _ => panic!("runtime core not set in CURRENT thread-local"), | |
| _ => unreachable!("runtime core not set in CURRENT thread-local"), |
| Some(ret) => ret, | ||
| None => { | ||
| // `block_on` panicked. | ||
| panic!("a spawned task panicked and the runtime is configured to shutdown on unhandled panic"); |
There was a problem hiding this comment.
it's kind of a bummer that we just create a new panic here, rather than continuing the original panic...
There was a problem hiding this comment.
I agree, but I don't think its a big deal. The panic is still printed to stderr by the panic handler.
| // TODO: This should be set as a builder | ||
| Arc::get_mut(&mut self.context.shared) | ||
| .expect("TODO: we shouldn't panic") |
There was a problem hiding this comment.
do we want to address this TODO before merging, or is it being saved for later?
There was a problem hiding this comment.
I'm saving it for later. It is tracked as an "open question" in the original issue (related to LocalSet::builder().
|
@hawkw I considered this, but unless we never propagate the panic to the Anyway, I will track the question in the original issue. |
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hmm, yeah, that's a good point; I was considering this specifically in the case of the current thread runtime; it does get a lot messier when the runtime is shared... |
Extracts the refactor from #4518. The basic scheduler takes many configuration options as arguments to the constructor. This cleans it up a bit by defining a `Config` struct and using that to pass arguments to the constructor.
Extracts the refactor from #4518. The basic scheduler takes many configuration options as arguments to the constructor. This cleans it up a bit by defining a `Config` struct and using that to pass arguments to the constructor.
|
Closed in favor of #4770 |
Allows the user to configure the runtime's behavior when a spawned task
panics. Currently, the panic is propagated to the JoinHandle and the
runtime resumes. This patch lets the user set the runtime to shutdown on
unhandled panic.
So far, this is only implemented for the current-thread runtime.
I will work on documentation next.
Refs: #4516