[Concurrency]: call AsyncStream's onCancel at most once #83190
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.
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.
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