-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Improve Task documentation. #85861
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
base: main
Are you sure you want to change the base?
Improve Task documentation. #85861
Conversation
Clarifies that Task instances do not inherit cancellation from an enclosing Task within which they are created. Clarifies that the term "child task" does not refer to such tasks. Clarifies some language which had previously made it seem like non-detached Tasks would inherit cancellation (they do not).
| /// Child tasks inherit the parent task's priority and task-local storage, | ||
| /// and canceling a parent task automatically cancels all of its child tasks. | ||
| /// You need to handle these considerations manually with a detached task. | ||
| /// In most cases, an "attached" unstructured task (created via `Task.init`) |
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.
lazy question since i did not yet search myself, but is this "attached" terminology used anywhere else? i've personally never seen it AFAIK and it seems a bit odd. to what is the Task 'attached'?
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 haven't seen it either. I have put it in quotes here ("attached") to connote the fact that it isn't an accepted term but is being used in a casual manner for rhetorical purposes. I do think, however, that the existence of Task.detached implies that an "attached" alternative must exist. I am also hoping here that by directly stating that Task.init is the alternative, it should help clarify for the reader that there isn't, say, a Task.attached someplace they've yet to stumble upon.
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 would be happy to change "attached" to non-detached if that would be preferable.
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 would be happy to change "attached" to non-detached if that would be preferable.
just my personal opinion, but both of those seem sort of 'overly jargony' and i'd be inclined to just avoid them if possible.
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 should not be introducing new terms like that. Please don’t call tasks “attached” that’s not a word we ever use anywhere.
these are called “unstructured tasks”.
ktoso
left a comment
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.
Sorry I don’t think I’d be comfortable accepting this as is. It is making up new ways to call tasks that are confusing and not used anywhere else. We’re open to docs improvements but we shouldn’t be inventing new names for things, it’ll only cause more confusion
| /// Child tasks inherit the parent task's priority and task-local storage, | ||
| /// and canceling a parent task automatically cancels all of its child tasks. | ||
| /// You need to handle these considerations manually with a detached task. | ||
| /// In most cases, an "attached" unstructured task (created via `Task.init`) |
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 should not be introducing new terms like that. Please don’t call tasks “attached” that’s not a word we ever use anywhere.
these are called “unstructured tasks”.
| /// and additional reasons can accrue during the cancellation process. | ||
| /// | ||
| /// A `Task` becomes cancelled when its `cancel()` method is called on it. There | ||
| /// is no other mechanism by which a `Task` can become cancelled. This is true |
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.
Thats very sneaky wording I’m not comfortable with… its true for specifically an unstructured task but the wording here isn’t clear enough, it may sound like you mean “any task”
| /// is no other mechanism by which a `Task` can become cancelled. This is true | ||
| /// even of a `Task` instance which is created inside the operation of another | ||
| /// `Task`. You can manually propagate cancellation from an enclosing `Task` to | ||
| /// another task created during its operation via `withTaskCancellationHandler(operation:onCancel:isolation:)`: |
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.
These should be double quotes to cause docc links
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 don’t think we should be giving complete examples here, we should link to the api which then has more examples
| /// - It cancels any child tasks and task groups of the task, including | ||
| /// those created in the future. If those tasks have cancellation handlers, | ||
| /// they also are triggered. | ||
| /// - It cancels any child tasks (lowercase-t tasks, like `async let` and |
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 should not be calling things “lowercase tasks” that’s wording that’s not used by swift documentation anywhere
Explanation
Clarifies that Task instances do not inherit cancellation from an enclosing Task within which they are created. Clarifies that the term "child task" does not refer to such tasks. Clarifies some language which had previously made it seem like non-detached Tasks would inherit cancellation (they do not).
Scope
The changes are limited exclusively to documentation. There is no impact on ABI.
Issues
N/A
Original PRs
N/A
Risk
Risk is minimal to zero, as these changes affect only the documentation.
Testing
N/A
Reviewers
Pending...