Skip to content

Concurrency: Don't rely on future wait context in asyncLet_finish. #38911

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 1 commit into from
Aug 18, 2021

Conversation

jckarter
Copy link
Contributor

If a future task has already completed, then task_future_wait doesn't populate the future context,
but asyncLet_finish was inappropriately relying on it to recover the result buffer pointer to
destroy the value inside. There's room in asyncLet_finish's own context to put this pointer, so
do that instead. Fixes rdar://81481317.

@jckarter jckarter requested a review from mikeash August 17, 2021 20:50
@jckarter
Copy link
Contributor Author

@swift-ci Please test

If a future task has already completed, then task_future_wait doesn't populate the future context,
but asyncLet_finish was inappropriately relying on it to recover the result buffer pointer to
destroy the value inside. There's room in asyncLet_finish's own context to put this pointer, so
do that instead. Fixes rdar://81481317.
@jckarter jckarter force-pushed the async-let-throw-completion-order branch from 4f736e9 to 745467d Compare August 17, 2021 21:15
@jckarter
Copy link
Contributor Author

@swift-ci Please test

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

LGTM

@jckarter jckarter merged commit 1007cc9 into swiftlang:main Aug 18, 2021
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.

5 participants