Clean up Task error handling. (#839)

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

Author: Lukasa

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

Sources/AsyncHTTPClient/HTTPHandler.swift

@@ -1024,9 +1024,9 @@ extension HTTPClient {
             taskDelegate?.fail(error)
         }
 
-        func fail<Delegate: HTTPClientResponseDelegate>(
-            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)
         }

Sources/AsyncHTTPClient/RequestBag.swift

@@ -120,7 +120,7 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate & Sendable>: 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<Delegate: HTTPClientResponseDelegate & Sendable>: 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):

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

@@ -3357,6 +3357,40 @@ 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<Error?> = .init(nil)
+            func didFinishRequest(task: HTTPClient.Task<Void>) throws {}
+            func didReceiveError(task: HTTPClient.Task<Response>, _ 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(