-
-
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
Conversation
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.
/// # Panic | ||
/// # Panics | ||
/// | ||
/// This will panic if `val` is not larger than `0`. | ||
#[track_caller] | ||
pub fn worker_threads(&mut self, val: usize) -> &mut Self { |
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 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).
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
tokio/tests/rt_panic.rs
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Use futures::future::pending()
instead of a timer where the test fails if the timer elapses.
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.
I think the timer isn't needed at all. abort()
is called on the handle before being awaited. So I've removed it entirely.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, fixed now. Thanks!
tokio/tests/rt_panic.rs
Outdated
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 comment
The 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.
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()); | |
} | |
use parking_lot::{const_mutex, Mutex}; | |
static SET_UP_PANIC_HOOK: Once = Once::new(); | |
static PANIC_MUTEX: Mutex<()> = const_mutex(()); | |
static PANIC_FILE: Mutex<String> = const_mutex(String::new()); |
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.
Thank you! I was hoping to get a comment like this.
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.
Caught by the clippy lint.
As suggested in review, instead of wrapping it in async+await. Co-authored-by: Alice Ryhl <[email protected]>
I noticed that no other tests in tokio are prefixed with `test_`, so I removed it from the new ones added.
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.
Looks good to me. Thanks!
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.19.2` -> `1.20.0` | | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.19.2` -> `1.20.0` | --- ### Release Notes <details> <summary>tokio-rs/tokio</summary> ### [`v1.20.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.20.0) [Compare Source](tokio-rs/tokio@tokio-1.19.2...tokio-1.20.0) ##### 1.20.0 (July 12, 2022) ##### Added - tokio: add track_caller to public APIs ([#​4772], [#​4791], [#​4793], [#​4806], [#​4808]) - sync: Add `has_changed` method to `watch::Ref` ([#​4758]) ##### Changed - time: remove `src/time/driver/wheel/stack.rs` ([#​4766]) - rt: clean up arguments passed to basic scheduler ([#​4767]) - net: be more specific about winapi features ([#​4764]) - tokio: use const initialized thread locals where possible ([#​4677]) - task: various small improvements to LocalKey ([#​4795]) ##### Fixed ##### Documented - fs: warn about performance pitfall ([#​4762]) - chore: fix spelling ([#​4769]) - sync: document spurious failures in oneshot ([#​4777]) - sync: add warning for watch in non-Send futures ([#​4741]) - chore: fix typo ([#​4798]) ##### Unstable - joinset: rename `join_one` to `join_next` ([#​4755]) - rt: unhandled panic config for current thread rt ([#​4770]) [#​4677]: tokio-rs/tokio#4677 [#​4741]: tokio-rs/tokio#4741 [#​4755]: tokio-rs/tokio#4755 [#​4758]: tokio-rs/tokio#4758 [#​4762]: tokio-rs/tokio#4762 [#​4764]: tokio-rs/tokio#4764 [#​4766]: tokio-rs/tokio#4766 [#​4767]: tokio-rs/tokio#4767 [#​4769]: tokio-rs/tokio#4769 [#​4770]: tokio-rs/tokio#4770 [#​4772]: tokio-rs/tokio#4772 [#​4777]: tokio-rs/tokio#4777 [#​4791]: tokio-rs/tokio#4791 [#​4793]: tokio-rs/tokio#4793 [#​4795]: tokio-rs/tokio#4795 [#​4798]: tokio-rs/tokio#4798 [#​4806]: tokio-rs/tokio#4806 [#​4808]: tokio-rs/tokio#4808 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTEuMSIsInVwZGF0ZWRJblZlciI6IjMyLjExMS4xIn0=--> Co-authored-by: cabr2-bot <[email protected]> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1458 Reviewed-by: crapStone <[email protected]> Co-authored-by: Calciumdibromid Bot <[email protected]> Co-committed-by: Calciumdibromid Bot <[email protected]>
Motivation
When a user of tokio calls a function that panics when misused (e.g. calling
Handle::current()
outside of a runtime), then the user currently sees the linenumber of the panic call inside tokio. It would be more informative for the user
to see the place where they called the panicking function.
It is still possible for the user to see the full stack trace by setting the
environment variable
RUST_BACKLOG=1
, so no useful information ishidden.
Hopefully this change fixes is the first step to closing #4413 (here it is done
for the main tokio crate, it still needs to be done for other crates).
Note that there was an earlier PR (#4445) to work on this issue for the
tokio-util
crate, but it hasn't seen much work recently and after looking into it, I found that
these change in the main crate are necessary prerequisites.
Solution
Functions that may panic can be annotated with
#[track_caller]
so thatin 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 thepanic 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. This requires adding the
lazy_static
crate as a dev dependency.The annotation of
block_on
was removed as it did not work. It appearsto 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.