Task doesn't correctly reference follow-on RequestBag objects so it cannot cancel them after redirect.

[gregcotten]

Update

Reproducible test case:
https://github.com/gregcotten/ahc-download-cancel-bug

Just run swift test

It turns out that a redirect causes a new task to be created, so you can't count on execute(...)'s Task to be the only Task you'll need to cancel. I'll put a workaround on the above repo at some point.

---

Original

Stubbing this for now with hope to provide a reproducible test case when I have time...

I'm hoping to use structured concurrency Task cancellation to actually cancel a download, so I did something like this:

let download = Task {
    let delegate = try FileDownloadDelegate(path: tempDownloadPath)

    let downloadTask = HTTPClient.shared.execute(request: request, delegate: delegate, reportProgress: {
        print("progress: \($0.receivedBytes)")
    })

    _ = try await withTaskCancellationHandler {
        try await downloadTask.get()
    } onCancel: {
        print("cancelling download...")
        downloadTask.cancel()
        print("cancelled download")
    }
}

await Task.sleep(for: .seconds(5))
download.cancel()
try await download.value

The actual download progresses until it's done downloading the file, no matter what, and then downloadTask.get() returns without throwing an error.

What am I doing wrong here?

[gregcotten]

Follow up: canceling the task passed as a parameter in reportProgress can actually stop the download and fail the downloadTask.get(). Why can't I cancel the download outside of that scope? Obviously that would be something you'd want to do!

[Lukasa]

Sorry for this sitting unanswered over the weekend @gregcotten, let me see if I can reproduce this.

[Lukasa]

Ok, so I don't reproduce this locally with a simple alternative. Here's the complete code I'm running:

import NIOCore
import NIOPosix
import NIOHTTP1
import AsyncHTTPClient

private final class HTTPServer: ChannelInboundHandler {
    typealias InboundIn = HTTPServerRequestPart
    typealias OutboundOut = HTTPServerResponsePart

    func channelRead(context: ChannelHandlerContext, data: NIOAny) {
        switch self.unwrapInboundIn(data) {
        case .head, .body:
            ()
        case .end:
            self.sendResponse(context: context)
        }
    }

    private func sendResponse(context: ChannelHandlerContext) {
        let head = HTTPResponseHead(version: .http1_1, status: .ok, headers: ["Content-Length": "12"])
        context.write(self.wrapOutboundOut(.head(head)), promise: nil)
        context.writeAndFlush(self.wrapOutboundOut(.body(.byteBuffer(ByteBuffer(string: "hello")))), promise: nil)
        context.eventLoop.scheduleTask(in: .seconds(15)) {
            context.writeAndFlush(self.wrapOutboundOut(.body(.byteBuffer(ByteBuffer(string: " ")))), promise: nil)
        }
        context.eventLoop.scheduleTask(in: .seconds(30)) {
            context.write(self.wrapOutboundOut(.body(.byteBuffer(ByteBuffer(string: "world!")))), promise: nil)
            context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)
        }
    }
}

@main
struct Main {
    static func main() async throws {
        let server = try await Self.runServer()
        let client = HTTPClient(eventLoopGroup: .singletonMultiThreadedEventLoopGroup)
        let request = try HTTPClient.Request(url: "http://localhost:\(server.localAddress!.port!)/")

        let download = Task {
            let delegate = try FileDownloadDelegate(path: "/tmp/test.txt", reportProgress: {
                print("progress: \($0.receivedBytes)")
            })

            let downloadTask = client.execute(request: request, delegate: delegate)

            _ = try await withTaskCancellationHandler {
                try await downloadTask.get()
            } onCancel: {
                print("cancelling download...")
                downloadTask.cancel()
                print("cancelled download")
            }
        }

        try await Task.sleep(for: .seconds(5))
        download.cancel()
        try await download.value
    }

    private static func runServer() async throws -> Channel {
        return try await ServerBootstrap(group: .singletonMultiThreadedEventLoopGroup)
            .childChannelInitializer { channel in
                channel.pipeline.configureHTTPServerPipeline().flatMap {
                    channel.pipeline.addHandler(HTTPServer())
                }
            }
            .bind(host: "localhost", port: 0)
            .get()
    }
}

This code correctly throws at the top level, showing the following output in the console:

progress: 5
cancelling download...
cancelled download
Swift/ErrorType.swift:253: Fatal error: Error raised at top level: HTTPClientError.cancelled

You appear to be using slightly different APIs though, I can't find the APIs you're using on AHC. Do you know where they're coming from? If you run my sample code, do you see the same output as me?

[gregcotten]

Downloading from a real-world location and trying to cancel from the vended Task from HTTPClient.shared.execute(..., delegate: someFileDownloadDelegate) doesn't seem to work. If I cancel the Task I get from reportProgress, it seems to actually do something but also triggers an assertion:

NIOCore/FileHandle.swift:198: Assertion failed: leaked open NIOFileHandle(descriptor: UnsafeAtomic<UInt64>(_ptr: 0x0000600002f9c050)). Call `close()` to close or `takeDescriptorOwnership()` to take ownership and close by some other means.

Essentially after reportProgress starts getting called, I can no longer use the original task returned from execute(...) to cancel the download progress. In my case the download is a large, long-running task that definitely needs to be able to cancel if requested...

If you need a link to a semi-large file, this might suffice:

http://vault.centos.org/7.9.2009/isos/x86_64/CentOS-7-x86_64-Minimal-2009.iso

[gregcotten]

@Lukasa here's a reproducible test case:
https://github.com/gregcotten/ahc-download-cancel-bug

Just run swift test

[Lukasa]

Huh, so this is interesting. The problem isn't the download, it's the redirect.

The test as-written gets a 301, which can be seen in the log output:

2025-02-17T10:32:37+0000 trace download-task : ahc-el-preference=indifferent ahc-eventloop=NIOTransportServices.NIOTSEventLoop ahc-request-id=1 [AsyncHTTPClient] selected EventLoop for task given the preference
2025-02-17T10:32:37+0000 debug download-task : ahc-request-id=1 [AsyncHTTPClient] Request was queued (waiting for a connection to become available)
2025-02-17T10:32:38+0000 debug download-task : ahc-connection-id=0 ahc-el=NIOTransportServices.NIOTSEventLoop ahc-request-id=1 [AsyncHTTPClient] Request was scheduled on connection
2025-02-17T10:32:38+0000 trace download-task : ahc-channel-writable=true ahc-connection-id=0 ahc-el=NIOTransportServices.NIOTSEventLoop ahc-request-id=1 [AsyncHTTPClient] Channel active
2025-02-17T10:32:38+0000 trace download-task : ahc-connection-id=0 ahc-el=NIOTransportServices.NIOTSEventLoop ahc-request-id=1 [AsyncHTTPClient] Read event caught
2025-02-17T10:32:38+0000 trace download-task : ahc-connection-id=0 ahc-el=NIOTransportServices.NIOTSEventLoop ahc-http-part=head(HTTPResponseHead { version: HTTP/1.1, status: 301 Moved Permanently, headers: Server: CloudFront; Date: Mon, 17 Feb 2025 10:32:38 GMT; Content-Type: text/html; Content-Length: 167; Connection: keep-alive; Location: https://vault.centos.org/7.9.2009/isos/x86_64/CentOS-7-x86_64-Minimal-2009.iso; X-Cache: Redirect from cloudfront; Via: 1.1 4cafceb008e6fb971d9321d02b918f8e.cloudfront.net (CloudFront); X-Amz-Cf-Pop: LHR61-P4; X-Amz-Cf-Id: jNqHcMOMEoBvVDF5Fl-IHFF59ITrYzHjjSLBK_gIEKh_4ACVKRNQ_Q== }) ahc-request-id=1 [AsyncHTTPClient] HTTP response part received
2025-02-17T10:32:38+0000 trace download-task : ahc-connection-id=0 ahc-el=NIOTransportServices.NIOTSEventLoop ahc-request-id=1 [AsyncHTTPClient] Downstream requests more response body data
2025-02-17T10:32:38+0000 trace download-task : ahc-connection-id=0 ahc-el=NIOTransportServices.NIOTSEventLoop ahc-http-part=body([3c68746d6c3e0d0a3c686561643e3c7469746c653e333031204d6f7665642050...6f6e743c2f63656e7465723e0d0a3c2f626f64793e0d0a3c2f68746d6c3e0d0a](167 bytes)) ahc-request-id=1 [AsyncHTTPClient] HTTP response part received
2025-02-17T10:32:38+0000 trace download-task : ahc-connection-id=0 ahc-el=NIOTransportServices.NIOTSEventLoop ahc-http-part=end(nil) ahc-request-id=1 [AsyncHTTPClient] HTTP response part received
2025-02-17T10:32:38+0000 trace download-task : ahc-el-preference=indifferent ahc-eventloop=NIOTransportServices.NIOTSEventLoop ahc-request-id=2 [AsyncHTTPClient] selected EventLoop for task given the preference
2025-02-17T10:32:38+0000 debug download-task : ahc-request-id=2 [AsyncHTTPClient] Request was queued (waiting for a connection to become available)
cancelling...
cancelled

If we rewrite the test to avoid the 301 (by changing the URL to https) then the test passes:

downloading to /var/folders/n2/8dgq0zmd6zq24w06p7dplqxm0000gn/T/centos7_download_test.tmp
2025-02-17T10:33:29+0000 trace download-task : ahc-el-preference=indifferent ahc-eventloop=NIOTransportServices.NIOTSEventLoop ahc-request-id=1 [AsyncHTTPClient] selected EventLoop for task given the preference
2025-02-17T10:33:29+0000 debug download-task : ahc-request-id=1 [AsyncHTTPClient] Request was queued (waiting for a connection to become available)
cancelling...
cancelled
✔ Test ensureDownloadIsCancellable() passed after 5.068 seconds.

So I think the actual problem we have here is the task cancellation not working after a redirect is followed.

[Lukasa]

I've updated the title here, because I think this is the crux of the issue. On a HTTP redirect, a new Task object is produced (because we essentially make a new call to HTTPClient._execute). The original Task object doesn't know about this other Task, and so it cannot forward its logic on to the next one upon redirect, causing the Task to be basically dead.

This turns out not to matter beyond cancellation. The underlying response promise is forwarded on, so only cancellation fails to work.

There are a number of possible fixes, but the easiest one is to have the RequestBag keep track of the next Task in the redirect chain and, on being told to fail a request should be willing to forward that failure on to the next Task in the event of having redirected.

[Lukasa]

Resolved by #814.