Support task cancellation in HTTP client fetch and download#755
Support task cancellation in HTTP client fetch and download#755sysint64 wants to merge 1 commit intoreadium:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds cooperative cancellation checks to streamed-data convenience APIs on HTTPClient to avoid continuing to accumulate data (in-memory fetch) or write data (file download) after cancellation.
Changes:
- Add a cancellation check in
HTTPClient.fetch(_: )’s stream consumer. - Add a cancellation check in
HTTPClient.download(_:onProgress:)’s stream consumer (before file writes).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if Task.isCancelled { | ||
| return .failure(.cancelled) | ||
| } |
There was a problem hiding this comment.
Task.isCancelled inside this consume callback likely won't reflect cancellation of the original fetch task because consume is invoked from URLSessionDataDelegate.urlSession(_:dataTask:didReceive:) (synchronous delegate callback, not running inside the caller’s Swift concurrency Task). As a result, this check will almost always be false and won’t provide the intended cooperative cancellation. Consider handling cancellation at the stream/DefaultHTTPClient level (eg, cancel the underlying URLSessionTask when the caller task is cancelled or when consume returns .failure(.cancelled)), rather than checking Task.isCancelled here.
| if Task.isCancelled { | |
| return .failure(.cancelled) | |
| } |
| if Task.isCancelled { | ||
| return .failure(.cancelled) | ||
| } | ||
|
|
||
| do { | ||
| try fileHandle.seekToEnd() | ||
| try fileHandle.write(contentsOf: data) |
There was a problem hiding this comment.
The FileHandle is never closed. If consume returns .failure (including .cancelled), the failure path tries to removeItem(at:) while the handle is still open/in-scope, which can fail and leak a file descriptor; on success, the file can also remain open when returning HTTPDownload. Close the handle with a defer after opening it (and ensure it’s closed before deletion).
| } | ||
|
|
||
| do { | ||
| try fileHandle.seekToEnd() |
There was a problem hiding this comment.
seekToEnd() is called for every received chunk before writing. Since the file is created empty and FileHandle.write(contentsOf:) advances the offset, this repeated seek is unnecessary and can add overhead for large downloads. Consider removing the per-chunk seek (or seeking once up front if needed).
| try fileHandle.seekToEnd() |
|
@sysint64 Thank you for spotting that! Would you mind sharing with me how you tested this issue, and how you verified your fix? Ideally with an automated test, but I understand this is tricky to test. Maybe you modified the Test App to check it? |
|
@mickael-menu I tested only download method and not fetch. After review of my changes Claude AI suggested to add this to the fetch as well so I thought it won't harm adding it here. Regarding automatic testing - I agree it's better, the thing is that I'm not a Swift developer we use Flutter for our application and use Swift only to integrate with Readium. I have never written tests on Swift though I can explore this and see how it can be done for these changes. How I tested: in our application the code looked something like this: loadPublicationTask = Tast {
...
let acquisition = try await readium.lcpService.acquirePublication(
from: .file(url.fileURL!),
onProgress: { progress in
...
// Print for testing
print(progress)
}
).get()
...
}And if I want to cancel the task, I call this: loadPublicationTask?.cancel()
loadPublicationTask.= nilSo I tested that when I cancel the task successfully - I should stop receiving progress updation - i.e. check if the app keep outputting progress to the console. |
Added cooperative cancellation checks to the
HTTPClientextension methods that consume streamed data.Motivation:
Without these checks, once a stream starts, fetch and download will continue accumulating data even if the calling task has been cancelled. This could waste network bandwidth and memory for requests that are no longer needed (e.g., a user navigated away). Adding cooperative cancellation lets these operations bail out promptly between chunks.