Skip to content

Update Task.cancel() with more details #76508

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

Merged
merged 4 commits into from
Sep 18, 2024
Merged

Update Task.cancel() with more details #76508

merged 4 commits into from
Sep 18, 2024

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Sep 17, 2024

Add more details into cancel() docs, some developers were worried if they are allowed to call it concurrently and multiple times etc.

Add more details into cancel() docs, some developers were worried if they are allowed to call it concurrently and multiple times etc.
@ktoso ktoso requested review from amartini51 and hborla September 17, 2024 00:38
Copy link

@smumriak smumriak left a comment

Choose a reason for hiding this comment

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

Thank you!

@ktoso
Copy link
Contributor Author

ktoso commented Sep 17, 2024

@swift-ci please smoke test

Copy link
Member

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

Maybe it's worth adding that any taskCancellationHandler that's setup is run once task.cancel() is called from the thread that calls it

@ktoso
Copy link
Contributor Author

ktoso commented Sep 17, 2024

I'm not sure we want to document the threading guarantee hmmm... I did include that a note that cancellation handlers run just once though, good enough?

@ktoso ktoso added concurrency Feature: umbrella label for concurrency language features documentation labels Sep 17, 2024
Copy link
Member

@amartini51 amartini51 left a comment

Choose a reason for hiding this comment

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

Fixed spelling of "canceled" per APSG and left a suggested rewording.

/// checks whether it has been canceled at various points during its work.
/// checks whether it has been canceled at various points during its work,
/// and may either return early or throw an error in order to
/// finish running earlier than it would if it wasn't cancelled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

Cancels this task.

Cancelling a task has three primary effects:

  • It flags the task as cancelled.
  • It causes any active cancellation handlers on the task to run.
  • It cancels any child tasks and task groups of the task, including
    those created in the future.

Task cancellation is cooperative in the sense that cancelling a
task does not automatically cause arbitrary functions on the task
to stop running or throw errors. A function may choose to react
to cancellation by ending its work early, and it is conventional to
signal that to callers by throwing CancellationError. However,
a function that doesn't specifically check for cancellation will
run to completion normally even if the task it is running on is
cancelled. (Of course, it may still end early if it calls something
else that handles cancellation by throwing and then doesn't
handle the error.)

It is safe to cancel a task from any task or thread. It is safe for
multiple tasks or threads to cancel the same task at the same
time. Cancelling a task that has already been cancelled has no
additional effect.

cancel may need to acquire locks and synchronously run
arbitrary cancellation-handler code associated with the
cancelled task. To reduce the risk of deadlock, it is
recommended that callers release any locks they might be
holding before they call cancel.

  • SeeAlso: Task.checkCancellation()
  • SeeAlso: withTaskCancellationHandler()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's pretty comprehensive, lemme take that -- thanks @rjmccall

@ktoso
Copy link
Contributor Author

ktoso commented Sep 18, 2024

@swift-ci please smoke test

@ktoso ktoso enabled auto-merge September 18, 2024 00:38
@ktoso ktoso merged commit fc03688 into main Sep 18, 2024
3 checks passed
@ktoso ktoso deleted the ktoso-patch-17 branch September 18, 2024 04:17
@rjmccall
Copy link
Contributor

@amartini51 Please do a style check for the revised wording — we effectively have a sort of "merge conflict" here.

@amartini51
Copy link
Member

@rjmccall Thanks — opened #76560 with minor documentation style fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concurrency Feature: umbrella label for concurrency language features documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants