From deeb4a268f807f364cf8da88729336b4b5bf5077 Mon Sep 17 00:00:00 2001 From: Braulio Valdivielso Date: Thu, 27 Jan 2022 21:04:09 +0000 Subject: [PATCH 1/3] runtime: swallow panics in drop(join_handle) fixes #4412 From the issue description: > In Tokio, panics are generally caught and not passed resumed when dropping the JoinHandle, however when dropping the JoinHandle of a task that has already completed, that panic can propagate to the user who dropped the JoinHandle. This PR fixes that. --- tokio/src/runtime/task/harness.rs | 12 +----------- tokio/tests/join_handle_panic.rs | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 11 deletions(-) create mode 100644 tokio/tests/join_handle_panic.rs diff --git a/tokio/src/runtime/task/harness.rs b/tokio/src/runtime/task/harness.rs index 0996e5232db..31285e96de0 100644 --- a/tokio/src/runtime/task/harness.rs +++ b/tokio/src/runtime/task/harness.rs @@ -165,8 +165,6 @@ where } pub(super) fn drop_join_handle_slow(self) { - let mut maybe_panic = None; - // Try to unset `JOIN_INTEREST`. This must be done as a first step in // case the task concurrently completed. if self.header().state.unset_join_interested().is_err() { @@ -175,21 +173,13 @@ where // the scheduler or `JoinHandle`. i.e. if the output remains in the // task structure until the task is deallocated, it may be dropped // by a Waker on any arbitrary thread. - let panic = panic::catch_unwind(panic::AssertUnwindSafe(|| { + let _ = panic::catch_unwind(panic::AssertUnwindSafe(|| { self.core().stage.drop_future_or_output(); })); - - if let Err(panic) = panic { - maybe_panic = Some(panic); - } } // Drop the `JoinHandle` reference, possibly deallocating the task self.drop_reference(); - - if let Some(panic) = maybe_panic { - panic::resume_unwind(panic); - } } /// Remotely aborts the task. diff --git a/tokio/tests/join_handle_panic.rs b/tokio/tests/join_handle_panic.rs new file mode 100644 index 00000000000..f7de92d4178 --- /dev/null +++ b/tokio/tests/join_handle_panic.rs @@ -0,0 +1,20 @@ +#![warn(rust_2018_idioms)] +#![cfg(feature = "full")] + +struct PanicsOnDrop; + +impl Drop for PanicsOnDrop { + fn drop(&mut self) { + panic!("I told you so"); + } +} + +#[tokio::test] +async fn test_panics_do_not_propagate_when_dropping_join_handle() { + let join_handle = tokio::spawn(async move { PanicsOnDrop }); + + // only drop the JoinHandle when the task has completed + // (which is difficult to synchronize precisely) + tokio::time::sleep(std::time::Duration::from_millis(3)).await; + drop(join_handle); +} From 2a6072ae6af538f688b23ba2956c05c6467fcb1c Mon Sep 17 00:00:00 2001 From: Braulio Valdivielso Date: Thu, 27 Jan 2022 21:50:23 +0000 Subject: [PATCH 2/3] explain why we swallow the panic --- tokio/src/runtime/task/harness.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tokio/src/runtime/task/harness.rs b/tokio/src/runtime/task/harness.rs index 31285e96de0..6bb07cf9a77 100644 --- a/tokio/src/runtime/task/harness.rs +++ b/tokio/src/runtime/task/harness.rs @@ -173,6 +173,10 @@ where // the scheduler or `JoinHandle`. i.e. if the output remains in the // task structure until the task is deallocated, it may be dropped // by a Waker on any arbitrary thread. + // + // Panics are delivered to the user via the `JoinHandle`. Given that + // they are dropping it, we assume they are not interested in it, + // and swallow it. let _ = panic::catch_unwind(panic::AssertUnwindSafe(|| { self.core().stage.drop_future_or_output(); })); From 512152b7efe88f2be40950ba7900bccc34ca1ba1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Braulio=20Valdivielso=20Mart=C3=ADnez?= Date: Thu, 27 Jan 2022 21:56:25 +0000 Subject: [PATCH 3/3] improve phrasing of the comment Co-authored-by: Alice Ryhl --- tokio/src/runtime/task/harness.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tokio/src/runtime/task/harness.rs b/tokio/src/runtime/task/harness.rs index 6bb07cf9a77..530e6b13c54 100644 --- a/tokio/src/runtime/task/harness.rs +++ b/tokio/src/runtime/task/harness.rs @@ -175,8 +175,8 @@ where // by a Waker on any arbitrary thread. // // Panics are delivered to the user via the `JoinHandle`. Given that - // they are dropping it, we assume they are not interested in it, - // and swallow it. + // they are dropping the `JoinHandle`, we assume they are not + // interested in the panic and swallow it. let _ = panic::catch_unwind(panic::AssertUnwindSafe(|| { self.core().stage.drop_future_or_output(); }));