Propagate HTTPClient.Task<Response> failures to subsequent redirect tasks (#814)

Discussed in
#753

Author: gregcotten

Sources/AsyncHTTPClient/HTTPHandler.swift

@@ -956,7 +956,7 @@ internal struct RedirectHandler<ResponseType> {
         status: HTTPResponseStatus,
         to redirectURL: URL,
         promise: EventLoopPromise<ResponseType>
-    ) {
+    ) -> HTTPClient.Task<ResponseType>? {
         do {
             var redirectState = self.redirectState
             try redirectState.redirect(to: redirectURL.absoluteString)
@@ -976,13 +976,19 @@ internal struct RedirectHandler<ResponseType> {
                 headers: headers,
                 body: body
             )
-            self.execute(newRequest, redirectState).futureResult.whenComplete { result in
+
+            let newTask = self.execute(newRequest, redirectState)
+
+            newTask.futureResult.whenComplete { result in
                 promise.futureResult.eventLoop.execute {
                     promise.completeWith(result)
                 }
             }
+
+            return newTask
         } catch {
             promise.fail(error)
+            return nil
         }
     }
 }

Sources/AsyncHTTPClient/RequestBag.swift

@@ -43,6 +43,9 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
     // the consume body part stack depth is synchronized on the task event loop.
     private var consumeBodyPartStackDepth: Int
 
+    // if a redirect occurs, we store the task for it so we can propagate cancellation
+    private var redirectTask: HTTPClient.Task<Delegate.Response>? = nil
+
     // MARK: HTTPClientTask properties
 
     var logger: Logger {
@@ -234,7 +237,7 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
             executor.demandResponseBodyStream(self)
 
         case .redirect(let executor, let handler, let head, let newURL):
-            handler.redirect(status: head.status, to: newURL, promise: self.task.promise)
+            self.redirectTask = handler.redirect(status: head.status, to: newURL, promise: self.task.promise)
             executor.cancelRequest(self)
 
         case .forwardResponseHead(let head):
@@ -258,7 +261,7 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
             executor.demandResponseBodyStream(self)
 
         case .redirect(let executor, let handler, let head, let newURL):
-            handler.redirect(status: head.status, to: newURL, promise: self.task.promise)
+            self.redirectTask = handler.redirect(status: head.status, to: newURL, promise: self.task.promise)
             executor.cancelRequest(self)
 
         case .forwardResponsePart(let part):
@@ -294,7 +297,7 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
             }
 
         case .redirect(let handler, let head, let newURL):
-            handler.redirect(status: head.status, to: newURL, promise: self.task.promise)
+            self.redirectTask = handler.redirect(status: head.status, to: newURL, promise: self.task.promise)
         }
     }
 
@@ -358,6 +361,8 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
         let action = self.state.fail(error)
 
         self.executeFailAction0(action)
+
+        self.redirectTask?.fail(reason: error)
     }
 
     private func executeFailAction0(_ action: RequestBag<Delegate>.StateMachine.FailAction) {

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

@@ -4132,6 +4132,70 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass {
         XCTAssertNoThrow(try client.execute(request: request).wait())
     }
 
+    func testCancelingRequestAfterRedirect() throws {
+        let request = try Request(
+            url: self.defaultHTTPBinURLPrefix + "redirect/target",
+            method: .GET,
+            headers: ["X-Target-Redirect-URL": self.defaultHTTPBinURLPrefix + "wait"],
+            body: nil
+        )
+
+        class CancelAfterRedirect: HTTPClientResponseDelegate {
+            init() {}
+            func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task<Void>) throws {}
+        }
+
+        let task = defaultClient.execute(
+            request: request,
+            delegate: CancelAfterRedirect(),
+            deadline: .now() + .seconds(1)
+        )
+
+        // there is currently no HTTPClientResponseDelegate method to ensure the redirect occurs before we cancel, so we just sleep for 500ms
+        Thread.sleep(forTimeInterval: 0.5)
+
+        task.cancel()
+
+        XCTAssertThrowsError(try task.wait()) { error in
+            guard case let error = error as? HTTPClientError, error == .cancelled else {
+                return XCTFail("Should fail with cancelled")
+            }
+        }
+    }
+
+    func testFailingRequestAfterRedirect() throws {
+        let request = try Request(
+            url: self.defaultHTTPBinURLPrefix + "redirect/target",
+            method: .GET,
+            headers: ["X-Target-Redirect-URL": self.defaultHTTPBinURLPrefix + "wait"],
+            body: nil
+        )
+
+        class FailAfterRedirect: HTTPClientResponseDelegate {
+            init() {}
+            func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task<Void>) throws {}
+        }
+
+        let task = defaultClient.execute(
+            request: request,
+            delegate: FailAfterRedirect(),
+            deadline: .now() + .seconds(1)
+        )
+
+        // there is currently no HTTPClientResponseDelegate method to ensure the redirect occurs before we fail, so we just sleep for 500ms
+        Thread.sleep(forTimeInterval: 0.5)
+
+        struct TestError: Error {}
+
+        task.fail(reason: TestError())
+
+        XCTAssertThrowsError(try task.wait()) { error in
+            guard error is TestError else {
+                return XCTFail("Should fail with TestError")
+            }
+        }
+    }
+
     func testCancelingHTTP1RequestAfterHeaderSend() throws {
         var request = try HTTPClient.Request(url: self.defaultHTTPBin.baseURL + "/wait", method: .POST)
         // non-empty body is important