Skip to content

Commit

Permalink
time: add track_caller to public APIs
Browse files Browse the repository at this point in the history
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: tokio-rs#4413
  • Loading branch information
hds committed Jun 27, 2022
1 parent 55078ff commit 91682c8
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 0 deletions.
4 changes: 4 additions & 0 deletions tokio/src/time/clock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -189,6 +191,7 @@ cfg_test_util! {
clock
}

#[track_caller]
pub(crate) fn pause(&self) {
let mut inner = self.inner.lock();

Expand All @@ -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();

Expand Down
2 changes: 2 additions & 0 deletions tokio/src/time/driver/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
Expand Down Expand Up @@ -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)
}
Expand Down
1 change: 1 addition & 0 deletions tokio/src/time/driver/sleep.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>>,
Expand Down
127 changes: 127 additions & 0 deletions tokio/tests/time_panic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
#![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: FnOnce() + panic::UnwindSafe>(func: Func) -> Option<String> {
static PANIC_MUTEX: Mutex<()> = const_mutex(());

{
let _guard = PANIC_MUTEX.lock();
let panic_file: Arc<Mutex<Option<String>>> = 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<dyn Error>> {
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<dyn Error>> {
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<dyn Error>> {
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<dyn Error>> {
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<dyn Error>> {
// let rt = Builder::new_current_thread().build().unwrap();
// rt.block_on(async {
// let _ = timeout(Duration::from_millis(5), future::pending::<()>());
// });
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()
}

0 comments on commit 91682c8

Please sign in to comment.