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

task: consider changing JoinSet to not return cancelled JoinErrors #4534

Closed
hawkw opened this issue Feb 24, 2022 · 5 comments
Closed

task: consider changing JoinSet to not return cancelled JoinErrors #4534

hawkw opened this issue Feb 24, 2022 · 5 comments
Assignees
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-task Module: tokio/task

Comments

@hawkw
Copy link
Member

hawkw commented Feb 24, 2022

Is your feature request related to a problem? Please describe.

Currently, if a task on a JoinSet' is cancelled, the JoinSet's join_one and poll_join_one methods will return a JoinError with is_canceled = true. Right now, this only occurs when the JoinSet::abort_all method is called, or if the runtime the tasks are running on is shut down, so this is relatively infrequent --- typically, it will only once, unless the JoinSet contains tasks spawned on multiple runtimes.

However, PR #4530 introduces the ability to cancel individual tasks that are part of a JoinSet, without canceling the entire JoinSet or shutting down a runtime. This means that single tasks will fail with canceled JoinErrors more frequently. I'm not really sure what the value of returning a canceled JoinError is in this case.

Describe the solution you'd like

In a "future work" comment on #4530, I suggested that we might want to change the poll_join_one methods to skip JoinErrors if JoinError::is_canceled() is true, and continue polling. This way, canceled tasks are just ignored, unless the last task in the JoinSet was canceled (in which case, it will be empty). IMO, this makes the API somewhat simpler to use, as the user doesn't have to ignore canceled join errors while waiting for tasks on the JoinSet to complete.

Describe alternatives you've considered

Alternatively, we could...not do this. Ignoring canceled JoinErrors does technically remove some information from the API --- the user no longer gets a confirmation that a task on the join set was canceled. However, I'm not sure if there's any strong motivation for preserving those JoinErrors; off the top of my head, I can't really think of a situation in which they'd be useful.

@hawkw hawkw added A-tokio Area: The main tokio crate M-task Module: tokio/task C-feature-request Category: A feature request. labels Feb 24, 2022
@hawkw hawkw self-assigned this Feb 24, 2022
@hawkw
Copy link
Member Author

hawkw commented Feb 24, 2022

cc @Darksonn --- was there a reason you opted not to skip canceled JoinErrors that I might have overlooked?

@Darksonn
Copy link
Contributor

It did not occur to me that skipping them was an option at all.

@Noah-Kennedy
Copy link
Contributor

I would argue that there could be value in knowing when an aborted task terminates.

@hawkw
Copy link
Member Author

hawkw commented Feb 25, 2022

Yeah...after writing a JoinMap implementation (#4538) I'm thinking this is the correct behavior, at least in the case of JoinMap. Since the JoinMap API returns the JoinError along with the key associated with the task, the user actually knows which task was aborted or panicked, which seems useful to know. In JoinSet, it's a little bit less obvious, since there's no key to say "it was this task", but I think we're still better off keeping those APIs as consistent as possible...

@Darksonn
Copy link
Contributor

Darksonn commented Mar 2, 2022

I do not think that we should skip these errors.

@Darksonn Darksonn closed this as completed Mar 2, 2022
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 C-feature-request Category: A feature request. M-task Module: tokio/task
Projects
None yet
Development

No branches or pull requests

3 participants