Skip to content

[Concurrency]: call AsyncStream's onCancel at most once #83190

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jamieQ
Copy link
Contributor

@jamieQ jamieQ commented Jul 19, 2025

This change adds logic to prevent cases in which an AsyncStream created with the init(unfolding:onCancel:) initializer could cause the onCancel callback to be invoked more than one time.

resolves: #83137

This change adds logic to prevent cases in which an AsyncStream created
with the init(unfolding:onCancel:) initializer could cause the onCancel
callback to be invoked more than one time.
@jamieQ
Copy link
Contributor Author

jamieQ commented Jul 19, 2025

@swift-ci please smoke test

@jamieQ jamieQ marked this pull request as ready for review July 20, 2025 14:37
@jamieQ jamieQ requested a review from ktoso as a code owner July 20, 2025 14:37
@jamieQ
Copy link
Contributor Author

jamieQ commented Jul 20, 2025

cc @ktoso @phausler – here's a PoC of a solution. lmk what you think when you have a chance please!

Comment on lines +342 to +343
let cancelStorage: _AsyncStreamCriticalStorage<(() -> Void)?>?
= if let onCancel { .create(onCancel) } else { nil }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not sure introducing another lock is the ideal approach, but it seems at least serviceable. it wasn't immediately obvious to me if _AsyncStreamCriticalStorage supports a fully generic payload, or if it's supposed to contain only a closure (or closure-sized generic parameter?) per this comment. also i tried changing to use Mutex directly, but could not get the tests to successfully build, so unsure if that approach would be an option.


let cancelStorage: _AsyncStreamCriticalStorage<(() -> Void)?>?
= if let onCancel { .create(onCancel) } else { nil }

context = _Context {
return await withTaskCancellationHandler {
guard let result = await storage.value?() else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think there might also be a 'philosophical' question here about what the appropriate behavior is if we know that the cancel handler has been invoked during a suspension of the produce closure. should we return a non-nil value in such a case, or wait for a subsequent call to next() to do so? i suppose clients could achieve whatever behavior they prefer under the current implementation, but could not 'return the last value produced during a concurrent cancellation' if we changed things, so maybe it's better to leave it as-is?

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.

AsyncStream(unfolding:onCancel:) onCancel block is called twice
1 participant