Skip to content

Support task cancellation in HTTP client fetch and download#755

Open
sysint64 wants to merge 1 commit intoreadium:developfrom
sysint64:cancel-http
Open

Support task cancellation in HTTP client fetch and download#755
sysint64 wants to merge 1 commit intoreadium:developfrom
sysint64:cancel-http

Conversation

@sysint64
Copy link
Copy Markdown

Added cooperative cancellation checks to the HTTPClient extension 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.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +35 to +37
if Task.isCancelled {
return .failure(.cancelled)
}
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
if Task.isCancelled {
return .failure(.cancelled)
}

Copilot uses AI. Check for mistakes.
Comment on lines +122 to 128
if Task.isCancelled {
return .failure(.cancelled)
}

do {
try fileHandle.seekToEnd()
try fileHandle.write(contentsOf: data)
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
}

do {
try fileHandle.seekToEnd()
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
try fileHandle.seekToEnd()

Copilot uses AI. Check for mistakes.
@mickael-menu
Copy link
Copy Markdown
Member

@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?

@sysint64
Copy link
Copy Markdown
Author

sysint64 commented Mar 27, 2026

@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.= nil

So 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.

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.

3 participants