Skip to content

[Concurrency][docs] More docs about task groups being structured concurrency #82558

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 3 commits into from
Jul 8, 2025

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Jun 27, 2025

And correct an incorrect statement specifically in the withTaskGroup API, while other APIs already had correct versions of the statement.

refs #82396
resolves rdar://154000871

Would you mind having an editorial look @amartini51 ?

@ktoso
Copy link
Contributor Author

ktoso commented Jun 27, 2025

@swift-ci please smoke test

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This is a great improvement, clearly spelling out the cancellation and structured-concurrency behavior here.

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.

Thanks for writing this up! Fixed up some style issues and noted some structural issues. Maybe we can take another look at this early next week together.

Comment on lines 303 to 304
/// You may use `group.cancelAll()` to signal cancellation to all the remaining in-progress tasks,
/// however this will not interrupt their execution automatically, as the child tasks will need to cooperatively
/// react to the cancellation, and potentially return early (if able to).
Copy link
Member

Choose a reason for hiding this comment

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

Avoiding "may" which can be ambiguous between what's allowed and what's possible. Breaking up a long sentence.

Suggested change
/// You may use `group.cancelAll()` to signal cancellation to all the remaining in-progress tasks,
/// however this will not interrupt their execution automatically, as the child tasks will need to cooperatively
/// react to the cancellation, and potentially return early (if able to).
/// You can use `group.cancelAll()` to signal cancellation to the remaining in-progress tasks,
/// however this doesn't interrupt their execution automatically.
/// Rather, the child tasks need to cooperatively react to the cancellation,
/// and return early if that's possible.

Copy link
Member

Choose a reason for hiding this comment

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

After marking several editorial fixes back to "unresolved" because they weren't fixed in the latest diff, I wonder if something went wrong with your commit-squash-force-push workflow? I see the previous commit this PR used to point to included a bunch of these fixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, thanks, let me double check -- maybe I pushed over the GH applied changes 😟

@ktoso
Copy link
Contributor Author

ktoso commented Jun 28, 2025

Thanks Alex, I’ll apply those!

@ktoso
Copy link
Contributor Author

ktoso commented Jul 3, 2025

Applied the changes and following up with a few more, thanks for the review @amartini51 !

…urrency

And correct an incorrect statement specifically in the withTaskGroup
API, while other APIs already had correct versions of the statement.

refs swiftlang#82396
@ktoso ktoso force-pushed the wip-cleanup-group-docs branch from 744ef5a to 55b4418 Compare July 3, 2025 04:03
@ktoso ktoso requested a review from amartini51 July 3, 2025 04:03
@ktoso
Copy link
Contributor Author

ktoso commented Jul 3, 2025

@swift-ci please smoke test

@ktoso ktoso enabled auto-merge (squash) July 3, 2025 04:42
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.

I'll stop re-reviewing for now, since it looks like some of the editorial corrections you applied in 744ef5a were lost when you rebased or force-pushed. Will take another look after you restore those.

@ktoso
Copy link
Contributor Author

ktoso commented Jul 7, 2025

Reapplied the lost changes, sorry about that @amartini51

@ktoso ktoso requested a review from amartini51 July 7, 2025 09:50
@ktoso
Copy link
Contributor Author

ktoso commented Jul 7, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Jul 8, 2025

@swift-ci please smoke test macOS

@ktoso
Copy link
Contributor Author

ktoso commented Jul 8, 2025

@swift-ci please smoke test Windows

@ktoso ktoso merged commit fe86091 into swiftlang:main Jul 8, 2025
3 checks passed
@ktoso ktoso deleted the wip-cleanup-group-docs branch July 8, 2025 09:35
amartini51 added a commit that referenced this pull request Jul 9, 2025
Revise task group docs for style, fixing issues in the content changed in #82558 and some nearby content.
ktoso added a commit to ktoso/swift that referenced this pull request Jul 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants