From 5568a16e4f380b3471cde3f96b38a5e879405338 Mon Sep 17 00:00:00 2001 From: Hayden Stainsby Date: Wed, 20 Jul 2022 14:34:21 +0200 Subject: [PATCH] task: add track_caller to public APIs Functions that may panic can be annotated with `#[track_caller]` so that in the event of a panic, the function where the user called the panicking function is shown instead of the file and line within Tokio source. This change adds `#[track_caller]` to all the public APIs in the task module of the tokio crate where the documentation describes how the function may panic due to incorrect context or inputs. In cases where `#[track_caller]` does not work, it has been left out. For example, it currently does not work on async functions, blocks, or closures. So any call stack that passes through one of these before reaching the actual panic is not able to show the calling site outside of tokio as the panic location. The following functions have call stacks that pass through closures: * `task::block_in_place` * `task::local::spawn_local` Tests are included to cover each potentially panicking function. The following functions already had `#[track_caller]` applied everywhere it was needed and only tests have been added: * `task::spawn` * `task::LocalKey::sync_scope` Refs: #4413 --- tokio/src/runtime/mod.rs | 1 + tokio/src/task/local.rs | 1 + tokio/src/task/task_local.rs | 2 + tokio/tests/task_panic.rs | 92 ++++++++++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+) create mode 100644 tokio/tests/task_panic.rs diff --git a/tokio/src/runtime/mod.rs b/tokio/src/runtime/mod.rs index cf2b927d218..c0e2d96db8a 100644 --- a/tokio/src/runtime/mod.rs +++ b/tokio/src/runtime/mod.rs @@ -475,6 +475,7 @@ cfg_rt! { /// ``` /// /// [handle]: fn@Handle::block_on + #[track_caller] pub fn block_on(&self, future: F) -> F::Output { #[cfg(all(tokio_unstable, feature = "tracing"))] let future = crate::util::trace::task(future, "block_on", None, task::Id::next().as_u64()); diff --git a/tokio/src/task/local.rs b/tokio/src/task/local.rs index d37607a4b5d..ac77a3571e2 100644 --- a/tokio/src/task/local.rs +++ b/tokio/src/task/local.rs @@ -487,6 +487,7 @@ impl LocalSet { /// [`Runtime::block_on`]: method@crate::runtime::Runtime::block_on /// [in-place blocking]: fn@crate::task::block_in_place /// [`spawn_blocking`]: fn@crate::task::spawn_blocking + #[track_caller] #[cfg(feature = "rt")] #[cfg_attr(docsrs, doc(cfg(feature = "rt")))] pub fn block_on(&self, rt: &crate::runtime::Runtime, future: F) -> F::Output diff --git a/tokio/src/task/task_local.rs b/tokio/src/task/task_local.rs index 40447236789..2f6ab302314 100644 --- a/tokio/src/task/task_local.rs +++ b/tokio/src/task/task_local.rs @@ -287,6 +287,7 @@ impl LocalKey { /// # Panics /// /// This function will panic if the task local doesn't have a value set. + #[track_caller] pub fn get(&'static self) -> T { self.with(|v| *v) } @@ -425,6 +426,7 @@ enum ScopeInnerErr { } impl ScopeInnerErr { + #[track_caller] fn panic(&self) -> ! { match self { Self::BorrowError => panic!("cannot enter a task-local scope while the task-local storage is borrowed"), diff --git a/tokio/tests/task_panic.rs b/tokio/tests/task_panic.rs new file mode 100644 index 00000000000..661bbe23625 --- /dev/null +++ b/tokio/tests/task_panic.rs @@ -0,0 +1,92 @@ +#![warn(rust_2018_idioms)] +#![cfg(all(feature = "full", not(target_os = "wasi")))] + +use futures::future; +use std::error::Error; +use tokio::{runtime::Builder, spawn, task}; + +mod support { + pub mod panic; +} +use support::panic::test_panic; + +#[test] +fn local_set_block_on_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let rt = Builder::new_current_thread().enable_all().build().unwrap(); + let local = task::LocalSet::new(); + + rt.block_on(async { + local.block_on(&rt, future::pending::<()>()); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn spawn_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + spawn(future::pending::<()>()); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn local_key_sync_scope_panic_caller() -> Result<(), Box> { + tokio::task_local! { + static NUMBER: u32; + } + + let panic_location_file = test_panic(|| { + NUMBER.sync_scope(1, || { + NUMBER.with(|_| { + let _ = NUMBER.sync_scope(1, || {}); + }); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn local_key_with_panic_caller() -> Result<(), Box> { + tokio::task_local! { + static NUMBER: u32; + } + + let panic_location_file = test_panic(|| { + NUMBER.with(|_| {}); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn local_key_get_panic_caller() -> Result<(), Box> { + tokio::task_local! { + static NUMBER: u32; + } + + let panic_location_file = test_panic(|| { + NUMBER.get(); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +}