net: add track_caller to public APIs #4805

hds · 2022-07-04T19:13:03Z

Motivation

When a user of tokio calls a function that panics when misused (e.g. creating
a tokio TcpListener from a std TcpListener without having an IO driver
enabled in the surrounding tokio runtime) then the user currently sees the
line number 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 is
hidden.

This change is the 5th in a series towards closing #4413 (starting with #4772),
this change is for the net functions in the main tokio crate.

Solution

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 net APIs
in the main tokio crate where the documentation describes how the
function may panic due to incorrect context or inputs. Not all panic
cases can have #[track_caller] applied fully as the callstack passes
through a closure which isn't yet supported by the annotations (e.g. net
functions called from outside a tokio runtime).

Additionally, the documentation was updated to indicate additional cases
in which the public net functions may panic (the same as the io
functions).

Tests are included to cover each potentially panicking function.

Refs: #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 net APIs in the main tokio crate where the documentation describes how the function may panic due to incorrect context or inputs. Not all panic cases can have #[track_caller] applied fully as the callstack passes through a closure which isn't yet supported by the annotation (e.g. net functions called from outside a tokio runtime). Additionally, the documentation was updated to indicate additional cases in which the public net functions may panic (the same as the io functions). Tests are included to cover each potentially panicking function. Refs: tokio-rs#4413

Without a windows machine to test on, I had to add the test before adding track_caller to ensure that it failed first. This commit adds the change that should make the test pass (and also fixes the test name).

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: tokio-rs#4413

Darksonn · 2022-07-06T10:55:00Z

tokio/src/net/tcp/listener.rs

-    /// This function panics if thread-local runtime is not set.
+    /// This function panics if there is no current reactor set, if the `rt`
+    /// feature flag is not enabled, or if thread-local runtime is not set.


Setting a current reactor or thread-local runtime is the same thing?

OK, and is my understanding correct that this also covers needing enable_io on the runtime?

Does it make sense to keep the description of the case that the rt feature flag is not enabled? Or would it be better to leave the comment as it was (here and in all the other places I changed it)?

Ah, I see what you meant. You can just say that it panics if you are not inside a runtime with IO enabled. You can't be inside such a runtime if rt is not enabled, so we don't need to mention that.

I've tried to write something that makes sense. Please let me know if it needs changing.

Modified the description of situations in which some functions can panic to align with reviewer suggestions (at least that's the intent).

Darksonn · 2022-07-09T11:51:57Z

tokio/src/net/tcp/listener.rs

+    /// This function panics if it is not called from within a runtime with
+    /// IO enabled or if the thread-local runtime is not set.


Suggested change

/// This function panics if it is not called from within a runtime with

/// IO enabled or if the thread-local runtime is not set.

/// This function panics if it is not called from within a runtime with

/// IO enabled.

@hds Did you have any thoughts on this doc change?

@Darksonn I agree with them, it fits in with the rest of the docs. I'll make them in all the relevant spots in the next few days - I've been on holiday and have extended that to not opening my computer at all for a week. (-;

Alright. The next release is probably a month from now, so there's no rush.

Changed to remove duplicate cause statements based on suggestions during the PR review.

Darksonn

Thanks!

tokio/tests/net_panic.rs

Co-authored-by: Alice Ryhl <aliceryhl@google.com>

Darksonn · 2022-07-18T20:19:33Z

tokio/tests/net_panic.rs

+#[test]
+#[cfg(not(target_os = "wasi"))]
+fn udp_socket_from_std_panic_caller() -> Result<(), Box<dyn Error>> {


This needs to go on the entire file. WASM doesn't support catch_unwind. Sorry about that.

Thank you, I was just digging into the Wasm32-wasi PR to work out what I was doing wrong. That saved me a bunch of time!

…ler-net

Catch unwind isn't supported, so these tests won't work.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tokio](https://tokio.rs) ([source](/~https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.20.1` -> `1.21.0` | | [tokio](https://tokio.rs) ([source](/~https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.20.1` -> `1.21.0` | --- ### Release Notes <details> <summary>tokio-rs/tokio</summary> ### [`v1.21.0`](/~https://github.com/tokio-rs/tokio/releases/tag/tokio-1.21.0) [Compare Source](tokio-rs/tokio@tokio-1.20.1...tokio-1.21.0) ##### 1.21.0 (September 2, 2022) This release is the first release of Tokio to intentionally support WASM. The `sync,macros,io-util,rt,time` features are stabilized on WASM. Additionally the wasm32-wasi target is given unstable support for the `net` feature. ##### Added - net: add `device` and `bind_device` methods to TCP/UDP sockets ([#4882]) - net: add `tos` and `set_tos` methods to TCP and UDP sockets ([#4877]) - net: add security flags to named pipe `ServerOptions` ([#4845]) - signal: add more windows signal handlers ([#4924]) - sync: add `mpsc::Sender::max_capacity` method ([#4904]) - sync: implement Weak version of `mpsc::Sender` ([#4595]) - task: add `LocalSet::enter` ([#4765]) - task: stabilize `JoinSet` and `AbortHandle` ([#4920]) - tokio: add `track_caller` to public APIs ([#4805], [#4848], [#4852]) - wasm: initial support for `wasm32-wasi` target ([#4716]) ##### Fixed - miri: improve miri compatibility by avoiding temporary references in `linked_list::Link` impls ([#4841]) - signal: don't register write interest on signal pipe ([#4898]) - sync: add `#[must_use]` to lock guards ([#4886]) - sync: fix hang when calling `recv` on closed and reopened broadcast channel ([#4867]) - task: propagate attributes on task-locals ([#4837]) ##### Changed - fs: change panic to error in `File::start_seek` ([#4897]) - io: reduce syscalls in `poll_read` ([#4840]) - process: use blocking threadpool for child stdio I/O ([#4824]) - signal: make `SignalKind` methods const ([#4956]) ##### Internal changes - rt: extract `basic_scheduler::Config` ([#4935]) - rt: move I/O driver into `runtime` module ([#4942]) - rt: rename internal scheduler types ([#4945]) ##### Documented - chore: fix typos and grammar ([#4858], [#4894], [#4928]) - io: fix typo in `AsyncSeekExt::rewind` docs ([#4893]) - net: add documentation to `try_read()` for zero-length buffers ([#4937]) - runtime: remove incorrect panic section for `Builder::worker_threads` ([#4849]) - sync: doc of `watch::Sender::send` improved ([#4959]) - task: add cancel safety docs to `JoinHandle` ([#4901]) - task: expand on cancellation of `spawn_blocking` ([#4811]) - time: clarify that the first tick of `Interval::tick` happens immediately ([#4951]) ##### Unstable - rt: add unstable option to disable the LIFO slot ([#4936]) - task: fix incorrect signature in `Builder::spawn_on` ([#4953]) - task: make `task::Builder::spawn*` methods fallible ([#4823]) [#4595]: tokio-rs/tokio#4595 [#4716]: tokio-rs/tokio#4716 [#4765]: tokio-rs/tokio#4765 [#4805]: tokio-rs/tokio#4805 [#4811]: tokio-rs/tokio#4811 [#4823]: tokio-rs/tokio#4823 [#4824]: tokio-rs/tokio#4824 [#4837]: tokio-rs/tokio#4837 [#4840]: tokio-rs/tokio#4840 [#4841]: tokio-rs/tokio#4841 [#4845]: tokio-rs/tokio#4845 [#4848]: tokio-rs/tokio#4848 [#4849]: tokio-rs/tokio#4849 [#4852]: tokio-rs/tokio#4852 [#4858]: tokio-rs/tokio#4858 [#4867]: tokio-rs/tokio#4867 [#4877]: tokio-rs/tokio#4877 [#4882]: tokio-rs/tokio#4882 [#4886]: tokio-rs/tokio#4886 [#4893]: tokio-rs/tokio#4893 [#4894]: tokio-rs/tokio#4894 [#4897]: tokio-rs/tokio#4897 [#4898]: tokio-rs/tokio#4898 [#4901]: tokio-rs/tokio#4901 [#4904]: tokio-rs/tokio#4904 [#4920]: tokio-rs/tokio#4920 [#4924]: tokio-rs/tokio#4924 [#4928]: tokio-rs/tokio#4928 [#4935]: tokio-rs/tokio#4935 [#4936]: tokio-rs/tokio#4936 [#4937]: tokio-rs/tokio#4937 [#4942]: tokio-rs/tokio#4942 [#4945]: tokio-rs/tokio#4945 [#4951]: tokio-rs/tokio#4951 [#4953]: tokio-rs/tokio#4953 [#4956]: tokio-rs/tokio#4956 [#4959]: tokio-rs/tokio#4959 </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. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](/~https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1532 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

github-actions bot added the R-loom Run loom tests on this PR label Jul 4, 2022

hds force-pushed the track-caller-net branch from 90b3a27 to 3eb71f2 Compare July 4, 2022 20:07

Added track_caller to max_instances

62a7455

Without a windows machine to test on, I had to add the test before adding track_caller to ensure that it failed first. This commit adds the change that should make the test pass (and also fixes the test name).

hds changed the title ~~Draft: net: add track_caller to public APIs~~ net: add track_caller to public APIs Jul 4, 2022

hds marked this pull request as ready for review July 4, 2022 23:25

Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Jul 6, 2022

Darksonn reviewed Jul 6, 2022

View reviewed changes

Modified changes to some panic descriptions

dedc60e

Modified the description of situations in which some functions can panic to align with reviewer suggestions (at least that's the intent).

Darksonn reviewed Jul 9, 2022

View reviewed changes

Updated panic description

990fb3a

Changed to remove duplicate cause statements based on suggestions during the PR review.

Darksonn approved these changes Jul 18, 2022

View reviewed changes

Darksonn reviewed Jul 18, 2022

View reviewed changes

tokio/tests/net_panic.rs Show resolved Hide resolved

Only test udp_socket_from_std_panic_caller when not on easi

73c5aea

Co-authored-by: Alice Ryhl <aliceryhl@google.com>

Darksonn reviewed Jul 18, 2022

View reviewed changes

hds added 2 commits July 18, 2022 22:32

Merge branch 'master', remote-tracking branch 'origin' into track-cal…

9ff7e4d

…ler-net

Ignore all net_panic tests when target is wasi

373c17a

Catch unwind isn't supported, so these tests won't work.

Darksonn enabled auto-merge (squash) July 18, 2022 20:46

Darksonn merged commit cbdba8b into tokio-rs:master Jul 18, 2022

hds mentioned this pull request Jul 21, 2022

polish: add #[track_caller] to functions that can panic #4413

Closed

hds deleted the track-caller-net branch July 21, 2022 12:46

Darksonn mentioned this pull request Sep 1, 2022

chore: prepare Tokio v1.21.0 #4967

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: add track_caller to public APIs #4805

net: add track_caller to public APIs #4805

hds commented Jul 4, 2022 •

edited

Loading

Darksonn Jul 6, 2022

hds Jul 6, 2022

Darksonn Jul 6, 2022

hds Jul 6, 2022

Darksonn Jul 9, 2022

Darksonn Jul 17, 2022

hds Jul 17, 2022

Darksonn Jul 17, 2022

hds Jul 18, 2022

Darksonn left a comment

Darksonn Jul 18, 2022

hds Jul 18, 2022

		/// This function panics if it is not called from within a runtime with
		/// IO enabled or if the thread-local runtime is not set.

net: add track_caller to public APIs #4805

net: add track_caller to public APIs #4805

Conversation

hds commented Jul 4, 2022 • edited Loading

Motivation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hds commented Jul 4, 2022 •

edited

Loading