From 768f1cf19a895e1a0db0acc6652a57d37fff03fc Mon Sep 17 00:00:00 2001 From: Greg Cotten Date: Mon, 17 Feb 2025 09:47:14 -0800 Subject: [PATCH 1/5] store executed `redirectTask` in `RequestBag` and propagate cancellation --- Sources/AsyncHTTPClient/HTTPHandler.swift | 10 ++++++++-- Sources/AsyncHTTPClient/RequestBag+StateMachine.swift | 8 ++++++-- Sources/AsyncHTTPClient/RequestBag.swift | 11 ++++++++--- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 38b930638..68fe21bcc 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -956,7 +956,7 @@ internal struct RedirectHandler { status: HTTPResponseStatus, to redirectURL: URL, promise: EventLoopPromise - ) { + ) -> HTTPClient.Task? { do { var redirectState = self.redirectState try redirectState.redirect(to: redirectURL.absoluteString) @@ -976,13 +976,19 @@ internal struct RedirectHandler { 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 } } } diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index 37b2a42f0..15471b0d6 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -594,6 +594,7 @@ extension RequestBag.StateMachine { enum FailAction { case failTask(Error, HTTPRequestScheduler?, HTTPRequestExecutor?) case cancelExecutor(HTTPRequestExecutor) + case propagateCancellation case none } @@ -624,8 +625,11 @@ extension RequestBag.StateMachine { self.state = .finished(error: error) return .failTask(error, nil, nil) case .finished(.none): - // An error occurred after the request has finished. Ignore... - return .none + if (error as? HTTPClientError) == .cancelled { + return .propagateCancellation + } else { + return .none + } case .deadlineExceededWhileQueued: let realError: Error = { if (error as? HTTPClientError) == .cancelled { diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index f2720d9ef..72d1f6684 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -43,6 +43,9 @@ final class RequestBag { // 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? = nil + // MARK: HTTPClientTask properties var logger: Logger { @@ -234,7 +237,7 @@ final class RequestBag { 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 { 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 { } 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) } } @@ -368,6 +371,8 @@ final class RequestBag { self.failTask0(error) case .cancelExecutor(let executor): executor.cancelRequest(self) + case .propagateCancellation: + self.redirectTask?.cancel() case .none: break } From 9fc0ffadae1d873c1021354926fc4ff0b1283bd4 Mon Sep 17 00:00:00 2001 From: Greg Cotten Date: Mon, 17 Feb 2025 16:40:27 -0800 Subject: [PATCH 2/5] don't use FailAction, always cancel `redirectTask` on fail --- Sources/AsyncHTTPClient/RequestBag+StateMachine.swift | 8 ++------ Sources/AsyncHTTPClient/RequestBag.swift | 4 ++-- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index 15471b0d6..37b2a42f0 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -594,7 +594,6 @@ extension RequestBag.StateMachine { enum FailAction { case failTask(Error, HTTPRequestScheduler?, HTTPRequestExecutor?) case cancelExecutor(HTTPRequestExecutor) - case propagateCancellation case none } @@ -625,11 +624,8 @@ extension RequestBag.StateMachine { self.state = .finished(error: error) return .failTask(error, nil, nil) case .finished(.none): - if (error as? HTTPClientError) == .cancelled { - return .propagateCancellation - } else { - return .none - } + // An error occurred after the request has finished. Ignore... + return .none case .deadlineExceededWhileQueued: let realError: Error = { if (error as? HTTPClientError) == .cancelled { diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index 72d1f6684..af16d963a 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -361,6 +361,8 @@ final class RequestBag { let action = self.state.fail(error) self.executeFailAction0(action) + + self.redirectTask?.cancel() } private func executeFailAction0(_ action: RequestBag.StateMachine.FailAction) { @@ -371,8 +373,6 @@ final class RequestBag { self.failTask0(error) case .cancelExecutor(let executor): executor.cancelRequest(self) - case .propagateCancellation: - self.redirectTask?.cancel() case .none: break } From 5071aef575a860ff61e9677822a8390298fabbf3 Mon Sep 17 00:00:00 2001 From: Greg Cotten Date: Tue, 18 Feb 2025 13:30:49 -0800 Subject: [PATCH 3/5] add `testCancelingRequestAfterRedirect` --- .../HTTPClientTests.swift | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index a3c497761..ae9bd7d8c 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -4132,6 +4132,33 @@ 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) 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 testCancelingHTTP1RequestAfterHeaderSend() throws { var request = try HTTPClient.Request(url: self.defaultHTTPBin.baseURL + "/wait", method: .POST) // non-empty body is important From 867bdeb11f9be1baaece38c6a5faffcc69ce4988 Mon Sep 17 00:00:00 2001 From: Greg Cotten Date: Wed, 19 Feb 2025 09:59:56 -0800 Subject: [PATCH 4/5] propagate the error to `redirectTask` rather than simply `cancel()` --- Sources/AsyncHTTPClient/RequestBag.swift | 2 +- .../HTTPClientTests.swift | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index af16d963a..bc6325602 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -362,7 +362,7 @@ final class RequestBag { self.executeFailAction0(action) - self.redirectTask?.cancel() + self.redirectTask?.fail(reason: error) } private func executeFailAction0(_ action: RequestBag.StateMachine.FailAction) { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index ae9bd7d8c..0e0d42a93 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -4159,6 +4159,35 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { } } + 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) 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 From b724bea1c7deb771b0776ef283b1f352696f89c3 Mon Sep 17 00:00:00 2001 From: Greg Cotten Date: Wed, 19 Feb 2025 10:04:16 -0800 Subject: [PATCH 5/5] linter warnings --- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 0e0d42a93..0467e9fa9 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -4145,7 +4145,11 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws {} } - let task = defaultClient.execute(request: request, delegate: CancelAfterRedirect(), deadline: .now() + .seconds(1)) + 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) @@ -4172,7 +4176,11 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws {} } - let task = defaultClient.execute(request: request, delegate: FailAfterRedirect(), deadline: .now() + .seconds(1)) + 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)