From 94f1863982f6fbe95d1622da3897d83a178c2386 Mon Sep 17 00:00:00 2001 From: "Stainsby, Hayden" Date: Fri, 24 Jun 2022 11:30:06 +0200 Subject: [PATCH] time: 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 non-unstable public APIs in tokio-util 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 public functions that could not have `#[track_caller]` added for this reason are: * `time::advance` Tests are included to cover each potentially panicking function. In the following cases, `#[track_caller]` had already been added, and only tests have been added: * `time::interval` * `time::interval_at` Refs: #4413 --- tokio/src/time/clock.rs | 4 ++ tokio/src/time/driver/handle.rs | 2 + tokio/src/time/driver/sleep.rs | 1 + tokio/tests/time_panic.rs | 122 ++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+) create mode 100644 tokio/tests/time_panic.rs diff --git a/tokio/src/time/clock.rs b/tokio/src/time/clock.rs index 41be9bac48c..c3b180eb5c6 100644 --- a/tokio/src/time/clock.rs +++ b/tokio/src/time/clock.rs @@ -96,6 +96,7 @@ cfg_test_util! { /// /// [`Sleep`]: crate::time::Sleep /// [`advance`]: crate::time::advance + #[track_caller] pub fn pause() { let clock = clock().expect("time cannot be frozen from outside the Tokio runtime"); clock.pause(); @@ -110,6 +111,7 @@ cfg_test_util! { /// /// Panics if time is not frozen or if called from outside of the Tokio /// runtime. + #[track_caller] pub fn resume() { let clock = clock().expect("time cannot be frozen from outside the Tokio runtime"); let mut inner = clock.inner.lock(); @@ -189,6 +191,7 @@ cfg_test_util! { clock } + #[track_caller] pub(crate) fn pause(&self) { let mut inner = self.inner.lock(); @@ -208,6 +211,7 @@ cfg_test_util! { inner.unfrozen.is_none() } + #[track_caller] pub(crate) fn advance(&self, duration: Duration) { let mut inner = self.inner.lock(); diff --git a/tokio/src/time/driver/handle.rs b/tokio/src/time/driver/handle.rs index b61c0476e15..136919d9e78 100644 --- a/tokio/src/time/driver/handle.rs +++ b/tokio/src/time/driver/handle.rs @@ -53,6 +53,7 @@ cfg_rt! { /// /// [`Builder::enable_time`]: crate::runtime::Builder::enable_time /// [`Builder::enable_all`]: crate::runtime::Builder::enable_all + #[track_caller] pub(crate) fn current() -> Self { crate::runtime::context::time_handle() .expect("A Tokio 1.x context was found, but timers are disabled. Call `enable_time` on the runtime builder to enable timers.") @@ -81,6 +82,7 @@ cfg_not_rt! { /// /// [`Builder::enable_time`]: crate::runtime::Builder::enable_time /// [`Builder::enable_all`]: crate::runtime::Builder::enable_all + #[track_caller] pub(crate) fn current() -> Self { panic!("{}", crate::util::error::CONTEXT_MISSING_ERROR) } diff --git a/tokio/src/time/driver/sleep.rs b/tokio/src/time/driver/sleep.rs index a629cb81ee9..2ff6ad592b2 100644 --- a/tokio/src/time/driver/sleep.rs +++ b/tokio/src/time/driver/sleep.rs @@ -252,6 +252,7 @@ cfg_not_trace! { impl Sleep { #[cfg_attr(not(all(tokio_unstable, feature = "tracing")), allow(unused_variables))] + #[track_caller] pub(crate) fn new_timeout( deadline: Instant, location: Option<&'static Location<'static>>, diff --git a/tokio/tests/time_panic.rs b/tokio/tests/time_panic.rs new file mode 100644 index 00000000000..6a262516f63 --- /dev/null +++ b/tokio/tests/time_panic.rs @@ -0,0 +1,122 @@ +#![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 std::time::Duration; +use tokio::runtime::{Builder, Runtime}; +use tokio::time::{self, interval, interval_at, timeout, Instant}; + +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 pause_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let rt = basic(); + + rt.block_on(async { + time::pause(); + time::pause(); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn resume_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let rt = basic(); + + rt.block_on(async { + time::resume(); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn interval_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let _ = interval(Duration::from_millis(0)); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn interval_at_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let _ = interval_at(Instant::now(), Duration::from_millis(0)); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn timeout_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + // Runtime without `enable_time` so it has no current timer set. + let rt = Builder::new_current_thread().build().unwrap(); + rt.block_on(async { + let _ = timeout(Duration::from_millis(5), future::pending::<()>()); + }); + }); + + // 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() +}