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 91f9cc00550..8e1e6957eee 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 7d2c7773832..6e4e37d70ff 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 c796d129920..fa6f46b0a4c 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 ddded8ebb31..c2a2f061872 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 f8338fb0885..3cf626121b7 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 b8d5ba74e75..4c9899f940b 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 8ee0ef702d4..07b39d077b7 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 b856cfc8567..0492e4e9d39 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 00000000000..9aea9a89e08 --- /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() +}