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

JoinSet, clarify behaviour of blocking code under abort/drop #6802

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dvdsk
Copy link
Contributor

@dvdsk dvdsk commented Aug 27, 2024

I noticed some confusion when users interact with JoinSet. While task::spawn_blocking notes that blocking code can not be aborted that same warning is not present in the JoinSet.

While this makes perfect sense when you know how async works in Rust it is difficult for users less experienced with the inner workings of async.

Example: RustAudio/rodio#606 (comment)

I noticed some confusion when users interact with JoinSet. While
task::spawn_blocking notes that blocking code can not be aborted
that same warning is not present in the JoinSet.

While this makes perfect sense when you know how async works in Rust
it is difficult for users less experienced with the inner workings of
async.

Example: RustAudio/rodio#606 (comment)
Comment on lines 26 to 29
/// When the `JoinSet` is dropped, all tasks in the `JoinSet` are immediately aborted.
/// When the `JoinSet` is dropped, all non-blocking tasks in the `JoinSet` are
/// immediately aborted. Tasks spawned with
/// [`spawn_blocking`](Self::spawn_blocking) will abort when they reach an
/// `await` point
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't make sense to talk about .await points with spawn_blocking.

You may want to reword these things by referencing the general docs on cancellation of tasks, and by copying the note about spawn_blocking from there into the right spots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make sense to talk about .await points with spawn_blocking.

Ahh yeah ofc, I should have taken the time to look at the definition of spawn_blocking 😅.

by copying the note about spawn_blocking from there into the right spots.

Thats perfect!

@Darksonn Darksonn added T-docs Topic: documentation A-tokio Area: The main tokio crate M-task Module: tokio/task labels Aug 27, 2024
@Darksonn
Copy link
Contributor

Thank you for the PR!

@dvdsk
Copy link
Contributor Author

dvdsk commented Aug 27, 2024

Why does JoinSet::spawn_blocking return a AbortHandle? That seems misleading if the AbortHandle can not actually do anything.

@dvdsk
Copy link
Contributor Author

dvdsk commented Aug 27, 2024

Nevermind I just found it:

If you call abort on a spawn_blocking task, then this will not have any effect, and the task will continue running normally. The exception is if the task has not started running yet; in that case, calling abort may prevent the task from starting.

Would it be a good idea to call this out in the JoinSet docs, or is that getting too verbose?

Comment on lines 26 to 29
/// When the `JoinSet` is dropped, all tasks in the `JoinSet` are immediately aborted.
/// When the `JoinSet` is dropped, all *async* tasks in the `JoinSet` are
/// immediately aborted. Tasks spawned with [`spawn_blocking`] or
/// [`spawn_blocking_on`] can not be aborted as they are not *async*, see [task
/// cancellation].
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, they are aborted. It just has not effect unless it happens before the spawn_blocking task even starts running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the window between the task being spawned and it starting execution not extremely small? I fail to see the use case for abort. If I am to document it I would like to mention a case/example which helps the reader understand that window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nevermind just thought of it, the executor might have been set up to only allow a few simultaneous blocking tasks or rather threads. Ill work that into the docs

@dvdsk
Copy link
Contributor Author

dvdsk commented Sep 3, 2024

anything still blocking this?

Copy link
Member

@mox692 mox692 left a comment

Choose a reason for hiding this comment

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

Just one nit, otherwise looks good to me.

tokio/src/task/join_set.rs Outdated Show resolved Hide resolved
dvdsk and others added 2 commits September 3, 2024 14:07
Co-authored-by: Motoyuki Kimura <moymoymox@gmail.com>
/// When the `JoinSet` is dropped, all *async* tasks in the `JoinSet` are
/// immediately aborted. Tasks spawned with [`spawn_blocking`] or
/// [`spawn_blocking_on`] can only be aborted before they start running.
/// This is because they are not *async*, see [task cancellation].
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is "async" italic every time? We don't usually do that.

Copy link
Contributor Author

@dvdsk dvdsk Sep 3, 2024

Choose a reason for hiding this comment

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

I wanted to highlight the word to stress that it is the difference between async and blocking tasks that leads to the different behaviour. Its just a formatting thing, if it conflicts with the usual docs style it can be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-task Module: tokio/task T-docs Topic: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants