From 159508b916038217ceb668d727d9618b567ea8b1 Mon Sep 17 00:00:00 2001 From: Hayden Stainsby Date: Wed, 22 Jun 2022 08:31:36 +0200 Subject: [PATCH] add track_caller to public APIs (#4413) (#4772) 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 non-unstable public APIs in Tokio core where the documentation describes how the function may panic due to incorrect context or inputs. Since each internal function needs to be annotated down to the actual panic, it makes sense to start in Tokio core functionality. Tests are needed to ensure that all the annotations remain in place in case internal refactoring occurs. The test installs a panic hook to extract the file location from the `PanicInfo` struct and clone it up to the outer scope to check that the panic was indeed reported from within the test file. The downside to this approach is that the panic hook is global while set and so we need a lot of extra functionality to effectively serialize the tests so that only a single panic can occur at a time. The annotation of `block_on` was removed as it did not work. It appears to be impossible to correctly chain track caller when the call stack to the panic passes through clojures, as the track caller annotation can only be applied to functions. Also, the panic message itself is very descriptive. --- tokio/src/runtime/builder.rs | 8 ++- tokio/src/runtime/context.rs | 1 + tokio/src/runtime/handle.rs | 3 +- tokio/src/runtime/mod.rs | 1 - tokio/src/runtime/task/error.rs | 1 + tokio/tests/rt_panic.rs | 105 ++++++++++++++++++++++++++++++++ 6 files changed, 114 insertions(+), 5 deletions(-) create mode 100644 tokio/tests/rt_panic.rs diff --git a/tokio/src/runtime/builder.rs b/tokio/src/runtime/builder.rs index 924aac98e..490bc96d5 100644 --- a/tokio/src/runtime/builder.rs +++ b/tokio/src/runtime/builder.rs @@ -287,7 +287,7 @@ impl Builder { /// /// The default value is the number of cores available to the system. /// - /// # Panic + /// # Panics /// /// When using the `current_thread` runtime this method will panic, since /// those variants do not allow setting worker thread counts. @@ -324,9 +324,10 @@ impl Builder { /// rt.block_on(async move {}); /// ``` /// - /// # Panic + /// # Panics /// /// This will panic if `val` is not larger than `0`. + #[track_caller] pub fn worker_threads(&mut self, val: usize) -> &mut Self { assert!(val > 0, "Worker threads cannot be set to 0"); self.worker_threads = Some(val); @@ -342,7 +343,7 @@ impl Builder { /// /// The default value is 512. /// - /// # Panic + /// # Panics /// /// This will panic if `val` is not larger than `0`. /// @@ -354,6 +355,7 @@ impl Builder { /// [`spawn_blocking`]: fn@crate::task::spawn_blocking /// [`worker_threads`]: Self::worker_threads /// [`thread_keep_alive`]: Self::thread_keep_alive + #[track_caller] #[cfg_attr(docsrs, doc(alias = "max_threads"))] pub fn max_blocking_threads(&mut self, val: usize) -> &mut Self { assert!(val > 0, "Max blocking threads cannot be set to 0"); diff --git a/tokio/src/runtime/context.rs b/tokio/src/runtime/context.rs index aebbe1875..c31caf203 100644 --- a/tokio/src/runtime/context.rs +++ b/tokio/src/runtime/context.rs @@ -15,6 +15,7 @@ pub(crate) fn try_current() -> Result { } } +#[track_caller] pub(crate) fn current() -> Handle { match try_current() { Ok(handle) => handle, diff --git a/tokio/src/runtime/handle.rs b/tokio/src/runtime/handle.rs index 118d537d2..14101070c 100644 --- a/tokio/src/runtime/handle.rs +++ b/tokio/src/runtime/handle.rs @@ -87,7 +87,7 @@ impl Handle { /// Returns a `Handle` view over the currently running `Runtime`. /// - /// # Panic + /// # Panics /// /// This will panic if called outside the context of a Tokio runtime. That means that you must /// call this on one of the threads **being run by the runtime**, or from a thread with an active @@ -129,6 +129,7 @@ impl Handle { /// # }); /// # } /// ``` + #[track_caller] pub fn current() -> Self { context::current() } diff --git a/tokio/src/runtime/mod.rs b/tokio/src/runtime/mod.rs index 399e602d2..b9e7e2de9 100644 --- a/tokio/src/runtime/mod.rs +++ b/tokio/src/runtime/mod.rs @@ -472,7 +472,6 @@ 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/runtime/task/error.rs b/tokio/src/runtime/task/error.rs index 22b688aa2..7cf602abd 100644 --- a/tokio/src/runtime/task/error.rs +++ b/tokio/src/runtime/task/error.rs @@ -82,6 +82,7 @@ impl JoinError { /// } /// } /// ``` + #[track_caller] pub fn into_panic(self) -> Box { self.try_into_panic() .expect("`JoinError` reason is not a panic.") diff --git a/tokio/tests/rt_panic.rs b/tokio/tests/rt_panic.rs new file mode 100644 index 000000000..240c3d0c6 --- /dev/null +++ b/tokio/tests/rt_panic.rs @@ -0,0 +1,105 @@ +#![warn(rust_2018_idioms)] +#![cfg(feature = "full")] + +use futures::future; +use parking_lot::{const_mutex, Mutex}; +use std::error::Error; +use std::panic; +use std::sync::Arc; +use tokio::runtime::{Builder, Handle, Runtime}; + +fn test_panic(func: Func) -> Option { + static PANIC_MUTEX: Mutex<()> = const_mutex(()); + + { + let _guard = PANIC_MUTEX.lock(); + let panic_file: Arc>> = Arc::new(Mutex::new(None)); + + let prev_hook = panic::take_hook(); + { + let panic_file = panic_file.clone(); + panic::set_hook(Box::new(move |panic_info| { + let panic_location = panic_info.location().unwrap(); + panic_file + .lock() + .clone_from(&Some(panic_location.file().to_string())); + })); + } + + let result = panic::catch_unwind(func); + // Return to the previously set panic hook (maybe default) so that we get nice error + // messages in the tests. + panic::set_hook(prev_hook); + + if result.is_err() { + panic_file.lock().clone() + } else { + None + } + } +} + +#[test] +fn current_handle_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let _ = Handle::current(); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn into_panic_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(move || { + let rt = basic(); + rt.block_on(async { + let handle = tokio::spawn(future::pending::<()>()); + + handle.abort(); + + let err = handle.await.unwrap_err(); + assert!(!&err.is_panic()); + + let _ = err.into_panic(); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn builder_worker_threads_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let _ = Builder::new_multi_thread().worker_threads(0).build(); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn builder_max_blocking_threads_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let _ = Builder::new_multi_thread().max_blocking_threads(0).build(); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +fn basic() -> Runtime { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap() +}