From 8e1cf592606bee0fe166622f697fb069a77c9fd1 Mon Sep 17 00:00:00 2001 From: "Stainsby, Hayden" Date: Tue, 31 May 2022 18:05:01 +0200 Subject: [PATCH 1/5] add track_caller to public APIs (#4413) 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/Cargo.toml | 1 + 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 ++++++++++++++++++++++++++++++++ 7 files changed, 115 insertions(+), 5 deletions(-) create mode 100644 tokio/tests/rt_panic.rs diff --git a/tokio/Cargo.toml b/tokio/Cargo.toml index c9d2e4e6d82..4bb77681547 100644 --- a/tokio/Cargo.toml +++ b/tokio/Cargo.toml @@ -140,6 +140,7 @@ version = "0.3.6" tokio-test = { version = "0.4.0", path = "../tokio-test" } tokio-stream = { version = "0.1", path = "../tokio-stream" } futures = { version = "0.3.0", features = ["async-await"] } +lazy_static = "1.4" mockall = "0.11.1" tempfile = "3.1.0" async-stream = "0.3" diff --git a/tokio/src/runtime/builder.rs b/tokio/src/runtime/builder.rs index f6d78f736a0..eef2100ddcd 100644 --- a/tokio/src/runtime/builder.rs +++ b/tokio/src/runtime/builder.rs @@ -200,7 +200,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. @@ -237,9 +237,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); @@ -255,7 +256,7 @@ impl Builder { /// /// The default value is 512. /// - /// # Panic + /// # Panics /// /// This will panic if `val` is not larger than `0`. /// @@ -267,6 +268,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 aebbe18755a..c31caf203ac 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 118d537d2f1..14101070cd3 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 a030ccdf0d2..bec43359fce 100644 --- a/tokio/src/runtime/mod.rs +++ b/tokio/src/runtime/mod.rs @@ -469,7 +469,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 22b688aa221..7cf602abd33 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 00000000000..6a5db92a5f5 --- /dev/null +++ b/tokio/tests/rt_panic.rs @@ -0,0 +1,105 @@ +#![warn(rust_2018_idioms)] +#![cfg(feature = "full")] + +use lazy_static::lazy_static; +use std::error::Error; +use std::panic; +use std::sync::{Arc, Mutex, Once}; +use tokio::runtime::{Builder, Handle, Runtime}; + +fn test_panic(func: Func) -> Option { + lazy_static! { + static ref SET_UP_PANIC_HOOK: Once = Once::new(); + static ref PANIC_MUTEX: Arc> = Arc::new(Mutex::new(())); + static ref PANIC_FILE: Mutex = Mutex::new(String::new()); + } + + SET_UP_PANIC_HOOK.call_once(|| { + panic::set_hook(Box::new(|panic_info| { + let panic_location = panic_info.location().unwrap(); + PANIC_FILE + .lock() + .unwrap() + .clone_from(&panic_location.file().to_string()); + })); + }); + + { + let _guard = PANIC_MUTEX.lock(); + + let result = panic::catch_unwind(func); + + if result.is_err() { + Some(PANIC_FILE.lock().ok()?.clone()) + } else { + None + } + } +} + +#[test] +fn test_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 test_into_panic_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(move || { + let rt = basic(); + rt.block_on(async { + let handle = tokio::spawn(async { + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + }); + + 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 test_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 test_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() +} From cddbcb9dcc4be665bad43ae5ad08205a37b164d7 Mon Sep 17 00:00:00 2001 From: "Stainsby, Hayden" Date: Fri, 17 Jun 2022 09:30:59 +0200 Subject: [PATCH 2/5] fix race condition and remove lazy_static Based on feedback (thank-you!), the timer in the into_panic test has been replaced with `futures::future::pending()` and the lazy_static dev-dependency has been removed and replaced with `const_mutex` from `parking_lot`. Additionally, the panic catching has been reworked a little bit to set and unset the panic hook around the part of the test where the panic is expected. This fixes the issue that the normal test output for a failed test was being eaten up by the custom panic hook. --- tokio/Cargo.toml | 1 - tokio/tests/rt_panic.rs | 42 ++++++++++++++++++++--------------------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/tokio/Cargo.toml b/tokio/Cargo.toml index 4bb77681547..c9d2e4e6d82 100644 --- a/tokio/Cargo.toml +++ b/tokio/Cargo.toml @@ -140,7 +140,6 @@ version = "0.3.6" tokio-test = { version = "0.4.0", path = "../tokio-test" } tokio-stream = { version = "0.1", path = "../tokio-stream" } futures = { version = "0.3.0", features = ["async-await"] } -lazy_static = "1.4" mockall = "0.11.1" tempfile = "3.1.0" async-stream = "0.3" diff --git a/tokio/tests/rt_panic.rs b/tokio/tests/rt_panic.rs index 6a5db92a5f5..66fcce15ad2 100644 --- a/tokio/tests/rt_panic.rs +++ b/tokio/tests/rt_panic.rs @@ -1,36 +1,38 @@ #![warn(rust_2018_idioms)] #![cfg(feature = "full")] -use lazy_static::lazy_static; +use futures::future; +use parking_lot::{const_mutex, Mutex}; use std::error::Error; use std::panic; -use std::sync::{Arc, Mutex, Once}; +use std::sync::Arc; use tokio::runtime::{Builder, Handle, Runtime}; fn test_panic(func: Func) -> Option { - lazy_static! { - static ref SET_UP_PANIC_HOOK: Once = Once::new(); - static ref PANIC_MUTEX: Arc> = Arc::new(Mutex::new(())); - static ref PANIC_FILE: Mutex = Mutex::new(String::new()); - } - - SET_UP_PANIC_HOOK.call_once(|| { - panic::set_hook(Box::new(|panic_info| { - let panic_location = panic_info.location().unwrap(); - PANIC_FILE - .lock() - .unwrap() - .clone_from(&panic_location.file().to_string()); - })); - }); + 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() { - Some(PANIC_FILE.lock().ok()?.clone()) + panic_file.lock().clone() } else { None } @@ -54,9 +56,7 @@ fn test_into_panic_panic_caller() -> Result<(), Box> { let panic_location_file = test_panic(move || { let rt = basic(); rt.block_on(async { - let handle = tokio::spawn(async { - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - }); + let handle = tokio::spawn(async { future::pending::<()>() }); handle.abort(); From 0196c311195f4edf99e77c2d20a148faf72f0dbd Mon Sep 17 00:00:00 2001 From: "Stainsby, Hayden" Date: Fri, 17 Jun 2022 11:52:06 +0200 Subject: [PATCH 3/5] add await to future::pending Caught by the clippy lint. --- tokio/tests/rt_panic.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tokio/tests/rt_panic.rs b/tokio/tests/rt_panic.rs index 66fcce15ad2..118eb5c26b0 100644 --- a/tokio/tests/rt_panic.rs +++ b/tokio/tests/rt_panic.rs @@ -56,7 +56,7 @@ fn test_into_panic_panic_caller() -> Result<(), Box> { let panic_location_file = test_panic(move || { let rt = basic(); rt.block_on(async { - let handle = tokio::spawn(async { future::pending::<()>() }); + let handle = tokio::spawn(async { future::pending::<()>().await }); handle.abort(); From aaf73a86cfe5b6ebc5bdfe250b05aaf2a39dfd59 Mon Sep 17 00:00:00 2001 From: Hayden Stainsby Date: Fri, 17 Jun 2022 12:01:00 +0200 Subject: [PATCH 4/5] use future::pending directly in tokio::spawn As suggested in review, instead of wrapping it in async+await. Co-authored-by: Alice Ryhl --- tokio/tests/rt_panic.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tokio/tests/rt_panic.rs b/tokio/tests/rt_panic.rs index 118eb5c26b0..94d0af1e6c9 100644 --- a/tokio/tests/rt_panic.rs +++ b/tokio/tests/rt_panic.rs @@ -56,7 +56,7 @@ fn test_into_panic_panic_caller() -> Result<(), Box> { let panic_location_file = test_panic(move || { let rt = basic(); rt.block_on(async { - let handle = tokio::spawn(async { future::pending::<()>().await }); + let handle = tokio::spawn(future::pending::<()>()); handle.abort(); From f406d0eb84082acadbc515957c22a2882b9e75c0 Mon Sep 17 00:00:00 2001 From: "Stainsby, Hayden" Date: Tue, 21 Jun 2022 18:43:05 +0200 Subject: [PATCH 5/5] Remove "test_" prefix from test functions I noticed that no other tests in tokio are prefixed with `test_`, so I removed it from the new ones added. --- tokio/tests/rt_panic.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tokio/tests/rt_panic.rs b/tokio/tests/rt_panic.rs index 94d0af1e6c9..240c3d0c606 100644 --- a/tokio/tests/rt_panic.rs +++ b/tokio/tests/rt_panic.rs @@ -40,7 +40,7 @@ fn test_panic(func: Func) -> Option } #[test] -fn test_current_handle_panic_caller() -> Result<(), Box> { +fn current_handle_panic_caller() -> Result<(), Box> { let panic_location_file = test_panic(|| { let _ = Handle::current(); }); @@ -52,7 +52,7 @@ fn test_current_handle_panic_caller() -> Result<(), Box> { } #[test] -fn test_into_panic_panic_caller() -> Result<(), Box> { +fn into_panic_panic_caller() -> Result<(), Box> { let panic_location_file = test_panic(move || { let rt = basic(); rt.block_on(async { @@ -74,7 +74,7 @@ fn test_into_panic_panic_caller() -> Result<(), Box> { } #[test] -fn test_builder_worker_threads_panic_caller() -> Result<(), Box> { +fn builder_worker_threads_panic_caller() -> Result<(), Box> { let panic_location_file = test_panic(|| { let _ = Builder::new_multi_thread().worker_threads(0).build(); }); @@ -86,7 +86,7 @@ fn test_builder_worker_threads_panic_caller() -> Result<(), Box> { } #[test] -fn test_builder_max_blocking_threads_panic_caller() -> Result<(), Box> { +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(); });