-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Changes from 1 commit
8e1cf59
cddbcb9
0196c31
aaf73a8
f406d0e
e1fe84a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -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: FnOnce() + panic::UnwindSafe>(func: Func) -> Option<String> { | ||||||||||||||||||||||
lazy_static! { | ||||||||||||||||||||||
static ref SET_UP_PANIC_HOOK: Once = Once::new(); | ||||||||||||||||||||||
static ref PANIC_MUTEX: Arc<Mutex<()>> = Arc::new(Mutex::new(())); | ||||||||||||||||||||||
static ref PANIC_FILE: Mutex<String> = Mutex::new(String::new()); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have the dependencies to do this. There's no reason to add lazy_static.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! I was hoping to get a comment like this. |
||||||||||||||||||||||
|
||||||||||||||||||||||
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<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 { | ||||||||||||||||||||||
tokio::time::sleep(std::time::Duration::from_millis(100)).await; | ||||||||||||||||||||||
}); | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the timer isn't needed at all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awaiting a handle doesn't affect whether the task starts running or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understood, fixed now. Thanks! |
||||||||||||||||||||||
|
||||||||||||||||||||||
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() | ||||||||||||||||||||||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 acurrent_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).
There was a problem hiding this comment.
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