notify delegate about connect errors (swift-server#245)

Author: artemredkin

Sources/AsyncHTTPClient/ConnectionPool.swift

@@ -513,13 +513,13 @@ class HTTP1ConnectionProvider {
     }
 
     private func makeChannel(preference: HTTPClient.EventLoopPreference) -> EventLoopFuture<Channel> {
-        let eventLoop = preference.bestEventLoop ?? self.eventLoop
+        let channelEventLoop = preference.bestEventLoop ?? self.eventLoop
         let requiresTLS = self.key.scheme.requiresTLS
         let bootstrap: NIOClientTCPBootstrap
         do {
-            bootstrap = try NIOClientTCPBootstrap.makeHTTPClientBootstrapBase(on: eventLoop, host: self.key.host, port: self.key.port, requiresTLS: requiresTLS, configuration: self.configuration)
+            bootstrap = try NIOClientTCPBootstrap.makeHTTPClientBootstrapBase(on: channelEventLoop, host: self.key.host, port: self.key.port, requiresTLS: requiresTLS, configuration: self.configuration)
         } catch {
-            return eventLoop.makeFailedFuture(error)
+            return channelEventLoop.makeFailedFuture(error)
         }
 
         let channel: EventLoopFuture<Channel>
@@ -564,7 +564,7 @@ class HTTP1ConnectionProvider {
                     error = HTTPClient.NWErrorHandler.translateError(error)
                 }
             #endif
-            return self.eventLoop.makeFailedFuture(error)
+            return channelEventLoop.makeFailedFuture(error)
         }
     }
 

Sources/AsyncHTTPClient/HTTPClient.swift

@@ -545,6 +545,14 @@ public class HTTPClient {
                                                  deadline: deadline,
                                                  setupComplete: setupComplete.futureResult,
                                                  logger: logger)
+
+        let taskHandler = TaskHandler(task: task,
+                                      kind: request.kind,
+                                      delegate: delegate,
+                                      redirectHandler: redirectHandler,
+                                      ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown,
+                                      logger: logger)
+
         connection.flatMap { connection -> EventLoopFuture<Void> in
             logger.debug("got connection for request",
                          metadata: ["ahc-connection": "\(connection)",
@@ -561,12 +569,6 @@ public class HTTPClient {
             }
 
             return future.flatMap {
-                let taskHandler = TaskHandler(task: task,
-                                              kind: request.kind,
-                                              delegate: delegate,
-                                              redirectHandler: redirectHandler,
-                                              ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown,
-                                              logger: logger)
                 return channel.pipeline.addHandler(taskHandler)
             }.flatMap {
                 task.setConnection(connection)
@@ -590,7 +592,12 @@ public class HTTPClient {
             }
         }.always { _ in
             setupComplete.succeed(())
-        }.cascadeFailure(to: task.promise)
+        }.whenFailure { error in
+            taskHandler.callOutToDelegateFireAndForget { task in
+                delegate.didReceiveError(task: task, error)
+            }
+            task.promise.fail(error)
+        }
 
         return task
     }

Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift

@@ -43,6 +43,7 @@ extension HTTPClientInternalTests {
             ("testUploadStreamingIsCalledOnTaskEL", testUploadStreamingIsCalledOnTaskEL),
             ("testWeCanActuallyExactlySetTheEventLoops", testWeCanActuallyExactlySetTheEventLoops),
             ("testTaskPromiseBoundToEL", testTaskPromiseBoundToEL),
+            ("testConnectErrorCalloutOnCorrectEL", testConnectErrorCalloutOnCorrectEL),
             ("testInternalRequestURI", testInternalRequestURI),
         ]
     }

Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift

@@ -960,6 +960,45 @@ class HTTPClientInternalTests: XCTestCase {
         XCTAssertNoThrow(try task.wait())
     }
 
+    func testConnectErrorCalloutOnCorrectEL() throws {
+        class TestDelegate: HTTPClientResponseDelegate {
+            typealias Response = Void
+
+            let expectedEL: EventLoop
+            var receivedError: Bool = false
+
+            init(expectedEL: EventLoop) {
+                self.expectedEL = expectedEL
+            }
+
+            func didFinishRequest(task: HTTPClient.Task<Void>) throws {}
+
+            func didReceiveError(task: HTTPClient.Task<Void>, _ error: Error) {
+                self.receivedError = true
+                XCTAssertTrue(self.expectedEL.inEventLoop)
+            }
+        }
+
+        let elg = getDefaultEventLoopGroup(numberOfThreads: 2)
+        let el1 = elg.next()
+        let el2 = elg.next()
+
+        let httpBin = HTTPBin(refusesConnections: true)
+        let client = HTTPClient(eventLoopGroupProvider: .shared(elg))
+
+        defer {
+            XCTAssertNoThrow(try client.syncShutdown())
+            XCTAssertNoThrow(try elg.syncShutdownGracefully())
+        }
+
+        let request = try HTTPClient.Request(url: "http://localhost:\(httpBin.port)/get")
+        let delegate = TestDelegate(expectedEL: el1)
+        XCTAssertNoThrow(try httpBin.shutdown())
+        let task = client.execute(request: request, delegate: delegate, eventLoop: .init(.testOnly_exact(channelOn: el2, delegateOn: el1)))
+        XCTAssertThrowsError(try task.wait())
+        XCTAssertTrue(delegate.receivedError)
+    }
+
     func testInternalRequestURI() throws {
         let request1 = try Request(url: "https://someserver.com:8888/some/path?foo=bar")
         XCTAssertEqual(request1.kind, .host)

Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift

@@ -115,6 +115,7 @@ extension HTTPClientTests {
             ("testAllMethodsLog", testAllMethodsLog),
             ("testClosingIdleConnectionsInPoolLogsInTheBackground", testClosingIdleConnectionsInPoolLogsInTheBackground),
             ("testUploadStreamingNoLength", testUploadStreamingNoLength),
+            ("testConnectErrorPropagatedToDelegate", testConnectErrorPropagatedToDelegate),
             ("testDelegateCallinsTolerateRandomEL", testDelegateCallinsTolerateRandomEL),
             ("testContentLengthTooLongFails", testContentLengthTooLongFails),
             ("testContentLengthTooShortFails", testContentLengthTooShortFails),

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

@@ -2405,6 +2405,33 @@ class HTTPClientTests: XCTestCase {
         XCTAssertNoThrow(try future.wait())
     }
 
+    func testConnectErrorPropagatedToDelegate() throws {
+        class TestDelegate: HTTPClientResponseDelegate {
+            typealias Response = Void
+            var error: Error?
+            func didFinishRequest(task: HTTPClient.Task<Void>) throws {}
+            func didReceiveError(task: HTTPClient.Task<Response>, _ error: Error) {
+                self.error = error
+            }
+        }
+
+        let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup),
+                                    configuration: .init(timeout: .init(connect: .milliseconds(10))))
+
+        defer {
+            XCTAssertNoThrow(try httpClient.syncShutdown())
+        }
+
+        // This must throw as 198.51.100.254 is reserved for documentation only
+        let request = try HTTPClient.Request(url: "http://198.51.100.254:65535/get")
+        let delegate = TestDelegate()
+
+        XCTAssertThrowsError(try httpClient.execute(request: request, delegate: delegate).wait()) { error in
+            XCTAssertEqual(.connectTimeout(.milliseconds(10)), error as? ChannelError)
+            XCTAssertEqual(.connectTimeout(.milliseconds(10)), delegate.error as? ChannelError)
+        }
+    }
+
     func testDelegateCallinsTolerateRandomEL() throws {
         class TestDelegate: HTTPClientResponseDelegate {
             typealias Response = Void

Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift

@@ -32,6 +32,14 @@ extension RequestValidationTests {
             ("testGET_HEAD_DELETE_CONNECTRequestCanHaveBody", testGET_HEAD_DELETE_CONNECTRequestCanHaveBody),
             ("testInvalidHeaderFieldNames", testInvalidHeaderFieldNames),
             ("testValidHeaderFieldNames", testValidHeaderFieldNames),
+            ("testNoHeadersNoBody", testNoHeadersNoBody),
+            ("testNoHeadersHasBody", testNoHeadersHasBody),
+            ("testContentLengthHeaderNoBody", testContentLengthHeaderNoBody),
+            ("testContentLengthHeaderHasBody", testContentLengthHeaderHasBody),
+            ("testTransferEncodingHeaderNoBody", testTransferEncodingHeaderNoBody),
+            ("testTransferEncodingHeaderHasBody", testTransferEncodingHeaderHasBody),
+            ("testBothHeadersNoBody", testBothHeadersNoBody),
+            ("testBothHeadersHasBody", testBothHeadersHasBody),
         ]
     }
 }