From 4daeea8cad1ce8e67946bc0e17d499ab304b5ca2 Mon Sep 17 00:00:00 2001 From: Hayden Stainsby Date: Wed, 6 Jul 2022 12:53:52 +0200 Subject: [PATCH] sync: add track_caller to public APIs (#4808) 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 sync 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: * `sync::watch::Sender::send_modify` * `sync::watch::Sender::send_if_modified` Additionally, in the above functions it is a panic inside the supplied closure which causes the function to panic, and so showing the location of the panic itself is desirable. The following functions are async: * `sync::mpsc::bounded::Sender::send_timeout` Tests are included to cover each potentially panicking function. Refs: #4413 --- tokio/src/future/block_on.rs | 2 + tokio/src/runtime/enter.rs | 1 + tokio/src/sync/broadcast.rs | 1 + tokio/src/sync/mpsc/bounded.rs | 3 + tokio/src/sync/mpsc/unbounded.rs | 1 + tokio/src/sync/mutex.rs | 1 + tokio/src/sync/oneshot.rs | 1 + tokio/src/sync/rwlock.rs | 2 + tokio/tests/sync_panic.rs | 165 +++++++++++++++++++++++++++++++ 9 files changed, 177 insertions(+) create mode 100644 tokio/tests/sync_panic.rs diff --git a/tokio/src/future/block_on.rs b/tokio/src/future/block_on.rs index 91f9cc005..8e1e6957e 100644 --- a/tokio/src/future/block_on.rs +++ b/tokio/src/future/block_on.rs @@ -1,6 +1,7 @@ use std::future::Future; cfg_rt! { + #[track_caller] pub(crate) fn block_on(f: F) -> F::Output { let mut e = crate::runtime::enter::enter(false); e.block_on(f).unwrap() @@ -8,6 +9,7 @@ cfg_rt! { } cfg_not_rt! { + #[track_caller] pub(crate) fn block_on(f: F) -> F::Output { let mut park = crate::park::thread::CachedParkThread::new(); park.block_on(f).unwrap() diff --git a/tokio/src/runtime/enter.rs b/tokio/src/runtime/enter.rs index 7d2c77738..6e4e37d70 100644 --- a/tokio/src/runtime/enter.rs +++ b/tokio/src/runtime/enter.rs @@ -31,6 +31,7 @@ cfg_rt! { /// Marks the current thread as being within the dynamic extent of an /// executor. + #[track_caller] pub(crate) fn enter(allow_blocking: bool) -> Enter { if let Some(enter) = try_enter(allow_blocking) { return enter; diff --git a/tokio/src/sync/broadcast.rs b/tokio/src/sync/broadcast.rs index c796d1299..fa6f46b0a 100644 --- a/tokio/src/sync/broadcast.rs +++ b/tokio/src/sync/broadcast.rs @@ -430,6 +430,7 @@ const MAX_RECEIVERS: usize = usize::MAX >> 2; /// /// This will panic if `capacity` is equal to `0` or larger /// than `usize::MAX / 2`. +#[track_caller] pub fn channel(mut capacity: usize) -> (Sender, Receiver) { assert!(capacity > 0, "capacity is empty"); assert!(capacity <= usize::MAX >> 1, "requested capacity too large"); diff --git a/tokio/src/sync/mpsc/bounded.rs b/tokio/src/sync/mpsc/bounded.rs index ddded8ebb..c2a2f0618 100644 --- a/tokio/src/sync/mpsc/bounded.rs +++ b/tokio/src/sync/mpsc/bounded.rs @@ -105,6 +105,7 @@ pub struct Receiver { /// } /// } /// ``` +#[track_caller] pub fn channel(buffer: usize) -> (Sender, Receiver) { assert!(buffer > 0, "mpsc bounded channel requires buffer > 0"); let semaphore = (semaphore::Semaphore::new(buffer), buffer); @@ -281,6 +282,7 @@ impl Receiver { /// sync_code.join().unwrap() /// } /// ``` + #[track_caller] #[cfg(feature = "sync")] pub fn blocking_recv(&mut self) -> Option { crate::future::block_on(self.recv()) @@ -650,6 +652,7 @@ impl Sender { /// sync_code.join().unwrap() /// } /// ``` + #[track_caller] #[cfg(feature = "sync")] pub fn blocking_send(&self, value: T) -> Result<(), SendError> { crate::future::block_on(self.send(value)) diff --git a/tokio/src/sync/mpsc/unbounded.rs b/tokio/src/sync/mpsc/unbounded.rs index f8338fb08..3cf626121 100644 --- a/tokio/src/sync/mpsc/unbounded.rs +++ b/tokio/src/sync/mpsc/unbounded.rs @@ -206,6 +206,7 @@ impl UnboundedReceiver { /// sync_code.join().unwrap(); /// } /// ``` + #[track_caller] #[cfg(feature = "sync")] pub fn blocking_recv(&mut self) -> Option { crate::future::block_on(self.recv()) diff --git a/tokio/src/sync/mutex.rs b/tokio/src/sync/mutex.rs index b8d5ba74e..4c9899f94 100644 --- a/tokio/src/sync/mutex.rs +++ b/tokio/src/sync/mutex.rs @@ -411,6 +411,7 @@ impl Mutex { /// } /// /// ``` + #[track_caller] #[cfg(feature = "sync")] pub fn blocking_lock(&self) -> MutexGuard<'_, T> { crate::future::block_on(self.lock()) diff --git a/tokio/src/sync/oneshot.rs b/tokio/src/sync/oneshot.rs index 8ee0ef702..07b39d077 100644 --- a/tokio/src/sync/oneshot.rs +++ b/tokio/src/sync/oneshot.rs @@ -1052,6 +1052,7 @@ impl Receiver { /// sync_code.join().unwrap(); /// } /// ``` + #[track_caller] #[cfg(feature = "sync")] pub fn blocking_recv(self) -> Result { crate::future::block_on(self) diff --git a/tokio/src/sync/rwlock.rs b/tokio/src/sync/rwlock.rs index b856cfc85..0492e4e9d 100644 --- a/tokio/src/sync/rwlock.rs +++ b/tokio/src/sync/rwlock.rs @@ -506,6 +506,7 @@ impl RwLock { /// assert!(rwlock.try_write().is_ok()); /// } /// ``` + #[track_caller] #[cfg(feature = "sync")] pub fn blocking_read(&self) -> RwLockReadGuard<'_, T> { crate::future::block_on(self.read()) @@ -840,6 +841,7 @@ impl RwLock { /// assert_eq!(*read_lock, 2); /// } /// ``` + #[track_caller] #[cfg(feature = "sync")] pub fn blocking_write(&self) -> RwLockWriteGuard<'_, T> { crate::future::block_on(self.write()) diff --git a/tokio/tests/sync_panic.rs b/tokio/tests/sync_panic.rs new file mode 100644 index 000000000..9aea9a89e --- /dev/null +++ b/tokio/tests/sync_panic.rs @@ -0,0 +1,165 @@ +#![warn(rust_2018_idioms)] +#![cfg(feature = "full")] + +use std::error::Error; +use tokio::{ + runtime::{Builder, Runtime}, + sync::{broadcast, mpsc, oneshot, Mutex, RwLock}, +}; + +mod support { + pub mod panic; +} +use support::panic::test_panic; + +#[test] +fn broadcast_channel_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let (_, _) = broadcast::channel::(0); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn mutex_blocking_lock_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let rt = basic(); + rt.block_on(async { + let mutex = Mutex::new(5_u32); + mutex.blocking_lock(); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn oneshot_blocking_recv_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let rt = basic(); + rt.block_on(async { + let (_tx, rx) = oneshot::channel::(); + let _ = rx.blocking_recv(); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn rwlock_with_max_readers_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let _ = RwLock::::with_max_readers(0, (u32::MAX >> 3) + 1); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn rwlock_blocking_read_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let rt = basic(); + rt.block_on(async { + let lock = RwLock::::new(0); + let _ = lock.blocking_read(); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn rwlock_blocking_write_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let rt = basic(); + rt.block_on(async { + let lock = RwLock::::new(0); + let _ = lock.blocking_write(); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn mpsc_bounded_channel_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let (_, _) = mpsc::channel::(0); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn mpsc_bounded_receiver_blocking_recv_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let rt = basic(); + let (_tx, mut rx) = mpsc::channel::(1); + rt.block_on(async { + let _ = rx.blocking_recv(); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn mpsc_bounded_sender_blocking_send_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let rt = basic(); + let (tx, _rx) = mpsc::channel::(1); + rt.block_on(async { + let _ = tx.blocking_send(3); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn mpsc_unbounded_receiver_blocking_recv_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let rt = basic(); + let (_tx, mut rx) = mpsc::unbounded_channel::(); + rt.block_on(async { + let _ = rx.blocking_recv(); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +fn basic() -> Runtime { + Builder::new_current_thread().enable_all().build().unwrap() +}