rt: unhandled panic config for current thread rt #4770

carllerche · 2022-06-15T20:53:58Z

This is a rebased version of #4518.

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 and is flagged as unstable. The intent is
to build this incrementally and track outstanding questions and issues.

Refs: #4516

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

tokio/src/runtime/basic_scheduler.rs

Co-authored-by: Alice Ryhl <alice@ryhl.io>

carllerche · 2022-06-16T16:35:01Z

Running loom again to check a typo fix 😆

hawkw

overall, this looks good to me (and i previously approved it). i commented on a handful of minor things that may not be that important.

hawkw · 2022-06-16T18:31:18Z

tokio/src/runtime/basic_scheduler.rs

+                    // This hook is only called from within the runtime, so
+                    // `CURRENT` should match with `&self`, i.e. there is no
+                    // opportunity for a nested scheduler to be called.
+                    CURRENT.with(|maybe_cx| match maybe_cx {
+                        Some(cx) if Arc::ptr_eq(self, &cx.spawner.shared) => {
+                            let mut core = cx.core.borrow_mut();
+
+                            // If `None`, the runtime is shutting down, so there is no need to signal shutdown


this is an extremely silly and trivial nit, but it kinda bothers me that one comment is wrapped, and the other one isn't...

hawkw · 2022-06-16T18:40:05Z

tokio/src/runtime/basic_scheduler.rs

+    /// True if a task panicked without being handled and the runtime is
+    /// configured to shutdown on unhandled panic.
+    unhandled_panic: bool,


previously i remember thinking that it would be nice to be able to actually store the unhandled panic and propagate it, rather than creating a new panic that loses the original panic's message, but you pointed out that that doesn't work, because the panic is also shared with the JoinHandle and they could race for it.

it occurs to me, though, that now that we have task IDs, perhaps we should store the ID of the task that panicked, rather than just a bool indicating that a task panicked. that way, we could at least identify which task panicked when the runtime panics, which could help users debug the panic when combined with other logs/diagnostics. WDYT?

hawkw · 2022-06-16T18:40:43Z

tokio/src/runtime/basic_scheduler.rs

+
+    #[cfg(tokio_unstable)]
+    /// How to respond to unhandled task panics.
+    pub(crate) unhandled_panic: crate::runtime::UnhandledPanic,


nit/TIOLI: perhaps this should be called on_unhandled_panic or something, since there's also a field called unhandled_panic that indicates whether there was an unhandled panic, and that seems confusable?

hawkw · 2022-06-16T18:40:53Z

tokio/src/runtime/basic_scheduler.rs

                    }
                }

                for _ in 0..core.spawner.shared.config.event_interval {
+                    // Make sure we didn't hit an unhandled_panic


Suggested change

// Make sure we didn't hit an unhandled_panic

// Make sure we didn't hit an unhandled panic

hawkw · 2022-06-16T18:42:51Z

tokio/src/runtime/builder.rs

+        /// ```
+        ///
+        /// [`JoinHandle`]: struct@crate::task::JoinHandle
+        ShutdownRuntime,


this is maybe a silly nitpick but i don't know if i love the name ShutdownRuntime for this --- the word "shutdown" kind of suggests to me that the runtime just...shuts down, rather than panicking. perhaps this should be called PanicRuntime or something? idk. maybe this is worse...

Can you add the suggestion to #4516?

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.19.2` -> `1.20.0` | | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.19.2` -> `1.20.0` | --- ### Release Notes <details> <summary>tokio-rs/tokio</summary> ### [`v1.20.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.20.0) [Compare Source](tokio-rs/tokio@tokio-1.19.2...tokio-1.20.0) ##### 1.20.0 (July 12, 2022) ##### Added - tokio: add track_caller to public APIs ([#4772], [#4791], [#4793], [#4806], [#4808]) - sync: Add `has_changed` method to `watch::Ref` ([#4758]) ##### Changed - time: remove `src/time/driver/wheel/stack.rs` ([#4766]) - rt: clean up arguments passed to basic scheduler ([#4767]) - net: be more specific about winapi features ([#4764]) - tokio: use const initialized thread locals where possible ([#4677]) - task: various small improvements to LocalKey ([#4795]) ##### Fixed ##### Documented - fs: warn about performance pitfall ([#4762]) - chore: fix spelling ([#4769]) - sync: document spurious failures in oneshot ([#4777]) - sync: add warning for watch in non-Send futures ([#4741]) - chore: fix typo ([#4798]) ##### Unstable - joinset: rename `join_one` to `join_next` ([#4755]) - rt: unhandled panic config for current thread rt ([#4770]) [#4677]: tokio-rs/tokio#4677 [#4741]: tokio-rs/tokio#4741 [#4755]: tokio-rs/tokio#4755 [#4758]: tokio-rs/tokio#4758 [#4762]: tokio-rs/tokio#4762 [#4764]: tokio-rs/tokio#4764 [#4766]: tokio-rs/tokio#4766 [#4767]: tokio-rs/tokio#4767 [#4769]: tokio-rs/tokio#4769 [#4770]: tokio-rs/tokio#4770 [#4772]: tokio-rs/tokio#4772 [#4777]: tokio-rs/tokio#4777 [#4791]: tokio-rs/tokio#4791 [#4793]: tokio-rs/tokio#4793 [#4795]: tokio-rs/tokio#4795 [#4798]: tokio-rs/tokio#4798 [#4806]: tokio-rs/tokio#4806 [#4808]: tokio-rs/tokio#4808 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1458 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

DSharifi · 2023-12-18T12:51:55Z

Are there any updates for stabilizing this feature?

Darksonn · 2023-12-22T16:38:27Z

No.

carllerche requested review from Darksonn and hawkw June 15, 2022 20:53

github-actions bot added the R-loom Run loom tests on this PR label Jun 15, 2022

carllerche added 2 commits June 15, 2022 14:07

Fix doc tests

853a1e0

fix more docs

23f9765

Darksonn approved these changes Jun 16, 2022

View reviewed changes

tokio/src/runtime/basic_scheduler.rs Outdated Show resolved Hide resolved

Update tokio/src/runtime/basic_scheduler.rs

9a3a850

Co-authored-by: Alice Ryhl <alice@ryhl.io>

carllerche added C-enhancement Category: A PR with an enhancement or bugfix. A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Jun 16, 2022

fix broken test

a25ba9c

hawkw approved these changes Jun 16, 2022

View reviewed changes

carllerche mentioned this pull request Jun 16, 2022

rt: unhandled panic config for current thread rt #4518

Closed

carllerche merged commit c98be22 into master Jun 16, 2022

carllerche deleted the unhandled-panics-take-2 branch June 16, 2022 19:54

This was referenced Jun 28, 2022

Implement a test scheduler that propagates panics #3218

Closed

Detached tasks in tests #2699

Closed

Darksonn mentioned this pull request Jul 12, 2022

chore: prepare tokio v1.20.0 #4830

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rt: unhandled panic config for current thread rt #4770

rt: unhandled panic config for current thread rt #4770

carllerche commented Jun 15, 2022 •

edited

carllerche commented Jun 16, 2022

hawkw left a comment

hawkw Jun 16, 2022

hawkw Jun 16, 2022

hawkw Jun 16, 2022

hawkw Jun 16, 2022

hawkw Jun 16, 2022

carllerche Jun 16, 2022

DSharifi commented Dec 18, 2023

Darksonn commented Dec 22, 2023

	// Make sure we didn't hit an unhandled_panic
	// Make sure we didn't hit an unhandled panic

rt: unhandled panic config for current thread rt #4770

rt: unhandled panic config for current thread rt #4770

Conversation

carllerche commented Jun 15, 2022 • edited

carllerche commented Jun 16, 2022

hawkw left a comment

Choose a reason for hiding this comment

hawkw Jun 16, 2022

Choose a reason for hiding this comment

hawkw Jun 16, 2022

Choose a reason for hiding this comment

hawkw Jun 16, 2022

Choose a reason for hiding this comment

hawkw Jun 16, 2022

Choose a reason for hiding this comment

hawkw Jun 16, 2022

Choose a reason for hiding this comment

carllerche Jun 16, 2022

Choose a reason for hiding this comment

DSharifi commented Dec 18, 2023

Darksonn commented Dec 22, 2023

carllerche commented Jun 15, 2022 •

edited