-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
base: main
Are you sure you want to change the base?
Conversation
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.
@swift-ci please smoke test |
let cancelStorage: _AsyncStreamCriticalStorage<(() -> Void)?>? | ||
= if let onCancel { .create(onCancel) } else { nil } |
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'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 { |
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 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?
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