From 7c885154e410b6615ecc9aa772659682a93c78be Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Wed, 30 Apr 2025 14:31:31 +0100 Subject: [PATCH 1/2] Clean up Task error handling. Motivation We have some Task error handling functions that are generic for no apparent reason. They're also typically called from contexts where they also report the error to the delegate, but one of the call sites doesn't do that. So add a test for that as well. Modifications - Rewrite Task.fail(with:delegate:) to be non-generic. - Add a call to the delegate error handler on the path that is missing it. - Add a test for that call Results Cleaner, easier to follow code --- Sources/AsyncHTTPClient/HTTPClient.swift | 3 +- Sources/AsyncHTTPClient/HTTPHandler.swift | 6 ++-- Sources/AsyncHTTPClient/RequestBag.swift | 4 +-- .../HTTPClientTests.swift | 31 +++++++++++++++++++ 4 files changed, 38 insertions(+), 6 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index e523a8f7d..02e450476 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -795,7 +795,8 @@ public class HTTPClient { self.poolManager.executeRequest(requestBag) } catch { - task.fail(with: error, delegateType: Delegate.self) + delegate.didReceiveError(task: task, error) + task.failInternal(with: error) } return task diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index fdca88982..8d92d8ef7 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -1024,9 +1024,9 @@ extension HTTPClient { taskDelegate?.fail(error) } - func fail( - with error: Error, - delegateType: Delegate.Type + /// Called internally only, used to fail a task from within the state machine functionality. + func failInternal( + with error: Error ) { self.promise.fail(error) } diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index d40e6ca04..f206325ee 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -120,7 +120,7 @@ final class RequestBag: Sendabl executor.cancelRequest(self) case .failTaskAndCancelExecutor(let error, let executor): self.delegate.didReceiveError(task: self.task, error) - self.task.fail(with: error, delegateType: Delegate.self) + self.task.failInternal(with: error) executor.cancelRequest(self) case .none: break @@ -181,7 +181,7 @@ final class RequestBag: Sendabl switch action { case .failTask(let error): self.delegate.didReceiveError(task: self.task, error) - self.task.fail(with: error, delegateType: Delegate.self) + self.task.failInternal(with: error) return self.task.eventLoop.makeFailedFuture(error) case .failFuture(let error): diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index b47cbe444..39f6e10a2 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3357,6 +3357,37 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertNoThrow(try future.wait()) } + func testDelegateGetsErrorsFromCreatingRequestBag() throws { + // We want to test that we propagate errors to the delegate from failures to construct the + // request bag. Those errors only come from invalid headers. + final class TestDelegate: HTTPClientResponseDelegate, Sendable { + typealias Response = Void + let error: NIOLockedValueBox = .init(nil) + func didFinishRequest(task: HTTPClient.Task) throws {} + func didReceiveError(task: HTTPClient.Task, _ error: Error) { + self.error.withLockedValue { $0 = error } + } + } + + let httpClient = HTTPClient( + eventLoopGroupProvider: .shared(self.clientGroup) + ) + + defer { + XCTAssertNoThrow(try httpClient.syncShutdown()) + } + + // 198.51.100.254 is reserved for documentation only + var request = try HTTPClient.Request(url: "http://198.51.100.254:65535/get") + request.headers.replaceOrAdd(name: "Not-ASCII", value: "not-fine\n") + let delegate = TestDelegate() + + XCTAssertThrowsError(try httpClient.execute(request: request, delegate: delegate).wait()) { + XCTAssertEqualTypeAndValue($0, HTTPClientError.invalidHeaderFieldValues(["not-fine\n"])) + XCTAssertEqualTypeAndValue(delegate.error.withLockedValue { $0 }, HTTPClientError.invalidHeaderFieldValues(["not-fine\n"])) + } + } + func testContentLengthTooLongFails() throws { let url = self.defaultHTTPBinURLPrefix + "post" XCTAssertThrowsError( From 2d05e455b21c9d49d16b2f5524ebaa0d057759e7 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Wed, 30 Apr 2025 14:51:10 +0100 Subject: [PATCH 2/2] Formatter --- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 39f6e10a2..307e56fd3 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3384,7 +3384,10 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertThrowsError(try httpClient.execute(request: request, delegate: delegate).wait()) { XCTAssertEqualTypeAndValue($0, HTTPClientError.invalidHeaderFieldValues(["not-fine\n"])) - XCTAssertEqualTypeAndValue(delegate.error.withLockedValue { $0 }, HTTPClientError.invalidHeaderFieldValues(["not-fine\n"])) + XCTAssertEqualTypeAndValue( + delegate.error.withLockedValue { $0 }, + HTTPClientError.invalidHeaderFieldValues(["not-fine\n"]) + ) } }