Make URLSession async-compatibility shims cancellation-aware - #136
Merged
alexeichhorn merged 3 commits intoAug 6, 2026
Merged
Conversation
The data(from:)/data(for:) continuation wrappers ignored Swift task cancellation entirely — cancelling a Task running an extraction let the underlying dataTask download to completion anyway. Wrap both in withTaskCancellationHandler and cancel the URLSessionTask on task cancellation (guarding the register/cancel race with a locked box), so cancelled extractions abort their network work immediately with URLError(.cancelled).
Contributor
📝 WalkthroughWalkthroughAdds a thread-safe cancellation box for in-flight 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/YouTubeKit/Extensions/AsyncCompatibility.swift (2)
44-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared continuation/cancellation logic to remove duplication.
data(from:)anddata(for:)are structurally identical (continuation setup, box registration, resume/cancel handling); only thedataTask(with:...)call differs. Extracting a shared private helper would remove this duplication and keep both call sites in sync going forward.♻️ Proposed refactor
+ private func performDataTask( + _ makeTask: (`@escaping` (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask + ) async throws -> (Data, URLResponse) { + let box = URLSessionTaskCancellationBox() + return try await withTaskCancellationHandler { + try await withCheckedThrowingContinuation { continuation in + let task = makeTask { data, response, error in + guard let data = data, let response = response else { + let error = error ?? URLError(.unknown) + return continuation.resume(throwing: error) + } + continuation.resume(returning: (data, response)) + } + box.register(task) + task.resume() + } + } onCancel: { + box.cancel() + } + } + func data(from url: URL) async throws -> (Data, URLResponse) { - let box = URLSessionTaskCancellationBox() - return try await withTaskCancellationHandler { - try await withCheckedThrowingContinuation { continuation in - let task = dataTask(with: url) { data, response, error in - guard let data = data, let response = response else { - let error = error ?? URLError(.unknown) - return continuation.resume(throwing: error) - } - - continuation.resume(returning: (data, response)) - } - box.register(task) - task.resume() - } - } onCancel: { - box.cancel() - } + try await performDataTask { dataTask(with: url, completionHandler: $0) } } func data(for request: URLRequest) async throws -> (Data, URLResponse) { - let box = URLSessionTaskCancellationBox() - return try await withTaskCancellationHandler { - try await withCheckedThrowingContinuation { continuation in - let task = dataTask(with: request) { data, response, error in - guard let data = data, let response = response else { - let error = error ?? URLError(.unknown) - return continuation.resume(throwing: error) - } - - continuation.resume(returning: (data, response)) - } - box.register(task) - task.resume() - } - } onCancel: { - box.cancel() - } + try await performDataTask { dataTask(with: request, completionHandler: $0) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/YouTubeKit/Extensions/AsyncCompatibility.swift` around lines 44 - 83, Extract the duplicated continuation and cancellation flow from data(from:) and data(for:) into a shared private helper that accepts the differing dataTask creation operation, while preserving the existing error handling, task registration, resume, and cancellation behavior. Update both public overloads to delegate to this helper.
44-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cancellation-path tests for
AsyncCompatibility.swiftThe currentTests/YouTubeKitTestssuite doesn’t cover immediate cancel, mid-flight cancel, or cancel-before-register behavior inURLSessionTaskCancellationBox. A few targeted async tests would protect this race-prone path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/YouTubeKit/Extensions/AsyncCompatibility.swift` around lines 44 - 83, Add targeted async tests in the YouTubeKit test suite for URLSessionTaskCancellationBox covering cancellation before task registration, immediate cancellation, and cancellation during an in-flight request. Exercise the AsyncCompatibility data(from:) and data(for:) paths as needed, and assert requests are cancelled without leaving continuations unresolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Sources/YouTubeKit/Extensions/AsyncCompatibility.swift`:
- Around line 44-83: Extract the duplicated continuation and cancellation flow
from data(from:) and data(for:) into a shared private helper that accepts the
differing dataTask creation operation, while preserving the existing error
handling, task registration, resume, and cancellation behavior. Update both
public overloads to delegate to this helper.
- Around line 44-83: Add targeted async tests in the YouTubeKit test suite for
URLSessionTaskCancellationBox covering cancellation before task registration,
immediate cancellation, and cancellation during an in-flight request. Exercise
the AsyncCompatibility data(from:) and data(for:) paths as needed, and assert
requests are cancelled without leaving continuations unresolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 08afae64-7a0a-4cc6-9630-f460168e5585
📒 Files selected for processing (1)
Sources/YouTubeKit/Extensions/AsyncCompatibility.swift
Owner
|
Nice. Valid problem. FYI: Will probably in near future anyways get rid of this backport extension as it is only really necessary for a very small subset of setups, if anyone even uses that anymore. But have to check. But nice fix for the time being. Thanks for your contribution ❤️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
URLSession.data(from:)/data(for:)back-compat shims inAsyncCompatibility.swiftwrapdataTaskin a plainwithCheckedThrowingContinuationwith no cancellation handling. When the surroundingTaskis cancelled, the underlyingURLSessionTaskkeeps running and downloads to completion — cancellation is silently ignored.In an app that starts extractions on scroll/focus changes and cancels superseded ones, this leaves "zombie" downloads competing for bandwidth. On a constrained device link they starved foreground work for 20–40s in our testing (an extraction for a title the user had already scrolled past kept running to completion).
Fix
Wrap both shims in
withTaskCancellationHandlerand cancel theURLSessionTaskwhen the Swift task is cancelled. A small locked box ties the two together and handles the race where cancellation arrives before thedataTaskis registered (cancel-on-register). Cancelled requests now fail promptly withURLError(.cancelled).No public API change; behaviour is identical for non-cancelled calls. The
@availablebounds already match the shims' existing platform floor (iOS 13 / tvOS 13 / watchOS 6 / macOS 10.15).Verification
Builds clean across the CI matrix locally, including explicit Swift 6 language mode.
Note
Overview Makes the backward-compatible
URLSessionasync shims cancellation-aware, so cancelling the surrounding Swift task also cancels the underlying network request.Cancellation handling. Both
data(from:)anddata(for:)now shareperformCancellableDataTask, backed by a thread-safe cancellation box that handles cancellation before or after theURLSessionTaskis registered. Cancelled requests surfaceURLError.cancelledinstead of continuing to completion.