Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add track_caller to public APIs (#4413) #4772

Merged
merged 6 commits into from Jun 22, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions tokio/src/runtime/builder.rs
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Comment on lines -327 to 331
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, does this function have two sections describing its panics?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny you should ask... the first Panic section (as far as I can tell) isn't actually true. Calling worker_threads for a current_thread runtime doesn't panic, at least not immediately and not after scheduling a task on the runtime either.

I was going to submit an issue for this to get some guidance on the preferred behaviour (fix docs or fix code).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created issue for it not panicking in the way described by the first Panic section: #4773

assert!(val > 0, "Worker threads cannot be set to 0");
self.worker_threads = Some(val);
Expand All @@ -255,7 +256,7 @@ impl Builder {
///
/// The default value is 512.
///
/// # Panic
/// # Panics
///
/// This will panic if `val` is not larger than `0`.
///
Expand All @@ -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");
Expand Down
1 change: 1 addition & 0 deletions tokio/src/runtime/context.rs
Expand Up @@ -15,6 +15,7 @@ pub(crate) fn try_current() -> Result<Handle, crate::runtime::TryCurrentError> {
}
}

#[track_caller]
pub(crate) fn current() -> Handle {
match try_current() {
Ok(handle) => handle,
Expand Down
3 changes: 2 additions & 1 deletion tokio/src/runtime/handle.rs
Expand Up @@ -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
Expand Down Expand Up @@ -129,6 +129,7 @@ impl Handle {
/// # });
/// # }
/// ```
#[track_caller]
pub fn current() -> Self {
context::current()
}
Expand Down
1 change: 0 additions & 1 deletion tokio/src/runtime/mod.rs
Expand Up @@ -469,7 +469,6 @@ cfg_rt! {
/// ```
///
/// [handle]: fn@Handle::block_on
#[track_caller]
pub fn block_on<F: Future>(&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());
Expand Down
1 change: 1 addition & 0 deletions tokio/src/runtime/task/error.rs
Expand Up @@ -82,6 +82,7 @@ impl JoinError {
/// }
/// }
/// ```
#[track_caller]
pub fn into_panic(self) -> Box<dyn Any + Send + 'static> {
self.try_into_panic()
.expect("`JoinError` reason is not a panic.")
Expand Down
105 changes: 105 additions & 0 deletions tokio/tests/rt_panic.rs
@@ -0,0 +1,105 @@
#![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 tokio::runtime::{Builder, Handle, Runtime};

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 test_current_handle_panic_caller() -> Result<(), Box<dyn Error>> {
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<dyn Error>> {
let panic_location_file = test_panic(move || {
let rt = basic();
rt.block_on(async {
let handle = tokio::spawn(async { future::pending::<()>().await });
hds marked this conversation as resolved.
Show resolved Hide resolved

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<dyn Error>> {
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<dyn Error>> {
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()
}