HTTPConnectionPool timeout requests: preserve connection errors

If a request times out because no connection could be attained, we should fail the request with the last connection creation error. If no connection creation error is present and there is no active connection, we fail the request with a connectTimeout error.

Author: fabianfett

Diff for: Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift

@@ -257,6 +257,11 @@ extension HTTPConnectionPool {
             return connecting
         }
 
+        /// Is there at least one connection that is able to run requests
+        var hasActiveConnections: Bool {
+            self.connections.contains(where: { $0.isIdle || $0.isLeased })
+        }
+
         func startingEventLoopConnections(on eventLoop: EventLoop) -> Int {
             return self.connections[self.overflowIndex..<self.connections.endIndex].reduce(into: 0) { count, connection in
                 guard connection.eventLoop === eventLoop else { return }

Diff for: Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

@@ -26,6 +26,8 @@ extension HTTPConnectionPool {
 
         private var connections: HTTP1Connections
         private var failedConsecutiveConnectionAttempts: Int = 0
+        /// the error from the last connection creation
+        private var lastConnectFailure: Error?
 
         private var requests: RequestQueue
         private var state: State = .running
@@ -136,12 +138,14 @@ extension HTTPConnectionPool {
 
         mutating func newHTTP1ConnectionEstablished(_ connection: Connection) -> Action {
             self.failedConsecutiveConnectionAttempts = 0
+            self.lastConnectFailure = nil
             let (index, context) = self.connections.newHTTP1ConnectionEstablished(connection)
             return self.nextActionForIdleConnection(at: index, context: context)
         }
 
         mutating func failedToCreateNewConnection(_ error: Error, connectionID: Connection.ID) -> Action {
             self.failedConsecutiveConnectionAttempts += 1
+            self.lastConnectFailure = error
 
             switch self.state {
             case .running:
@@ -223,8 +227,14 @@ extension HTTPConnectionPool {
         mutating func timeoutRequest(_ requestID: Request.ID) -> Action {
             // 1. check requests in queue
             if let request = self.requests.remove(requestID) {
+                var error: Error = HTTPClientError.getConnectionFromPoolTimeout
+                if let lastError = self.lastConnectFailure {
+                    error = lastError
+                } else if !self.connections.hasActiveConnections {
+                    error = HTTPClientError.connectTimeout
+                }
                 return .init(
-                    request: .failRequest(request, HTTPClientError.getConnectionFromPoolTimeout, cancelTimeout: false),
+                    request: .failRequest(request, error, cancelTimeout: false),
                     connection: .none
                 )
             }

Diff for: Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests+XCTest.swift

@@ -38,6 +38,9 @@ extension HTTPConnectionPool_HTTP1StateMachineTests {
             ("testConnectionPoolFullOfParkedConnectionsIsShutdownImmediately", testConnectionPoolFullOfParkedConnectionsIsShutdownImmediately),
             ("testParkedConnectionTimesOutButIsAlsoClosedByRemote", testParkedConnectionTimesOutButIsAlsoClosedByRemote),
             ("testConnectionBackoffVsShutdownRace", testConnectionBackoffVsShutdownRace),
+            ("testRequestThatTimesOutIsFailedWithLastConnectionCreationError", testRequestThatTimesOutIsFailedWithLastConnectionCreationError),
+            ("testRequestThatTimesOutBeforeAConnectionIsEstablishedIsFailedWithConnectTimeoutError", testRequestThatTimesOutBeforeAConnectionIsEstablishedIsFailedWithConnectTimeoutError),
+            ("testRequestThatTimesOutAfterAConnectionWasEstablishedSuccessfullyTimesOutWithGenericError", testRequestThatTimesOutAfterAConnectionWasEstablishedSuccessfullyTimesOutWithGenericError),
         ]
     }
 }

Diff for: Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift

@@ -153,7 +153,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
             return XCTFail("Unexpected request action: \(action.request)")
         }
         XCTAssert(requestToFail.__testOnly_wrapped_request() === mockRequest) // XCTAssertIdentical not available on Linux
-        XCTAssertEqual(requestError as? HTTPClientError, .getConnectionFromPoolTimeout)
+        XCTAssertEqual(requestError as? HTTPClientError, .connectTimeout)
         XCTAssertEqual(failRequest.connection, .none)
 
         // 4. retry connection, but no more queued requests.
@@ -626,4 +626,110 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
 
         XCTAssertEqual(state.connectionCreationBackoffDone(connectionID), .none)
     }
+
+    func testRequestThatTimesOutIsFailedWithLastConnectionCreationError() {
+        let elg = EmbeddedEventLoopGroup(loops: 1)
+        defer { XCTAssertNoThrow(try elg.syncShutdownGracefully()) }
+
+        var state = HTTPConnectionPool.StateMachine(
+            eventLoopGroup: elg,
+            idGenerator: .init(),
+            maximumConcurrentHTTP1Connections: 6
+        )
+
+        let mockRequest = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
+        let request = HTTPConnectionPool.Request(mockRequest)
+
+        let executeAction = state.executeRequest(request)
+        guard case .createConnection(let connectionID, on: let connEL) = executeAction.connection else {
+            return XCTFail("Expected to create a connection")
+        }
+
+        XCTAssertEqual(executeAction.request, .scheduleRequestTimeout(for: request, on: mockRequest.eventLoop))
+
+        let failAction = state.failedToCreateNewConnection(HTTPClientError.httpProxyHandshakeTimeout, connectionID: connectionID)
+        guard case .scheduleBackoffTimer(connectionID, backoff: _, on: let timerEL) = failAction.connection else {
+            return XCTFail("Expected to create a backoff timer")
+        }
+        XCTAssert(timerEL === connEL)
+        XCTAssertEqual(failAction.request, .none)
+
+        let timeoutAction = state.timeoutRequest(request.id)
+        XCTAssertEqual(timeoutAction.request, .failRequest(request, HTTPClientError.httpProxyHandshakeTimeout, cancelTimeout: false))
+        XCTAssertEqual(timeoutAction.connection, .none)
+    }
+
+    func testRequestThatTimesOutBeforeAConnectionIsEstablishedIsFailedWithConnectTimeoutError() {
+        let eventLoop = EmbeddedEventLoop()
+        defer { XCTAssertNoThrow(try eventLoop.syncShutdownGracefully()) }
+
+        var state = HTTPConnectionPool.StateMachine(
+            eventLoopGroup: eventLoop,
+            idGenerator: .init(),
+            maximumConcurrentHTTP1Connections: 6
+        )
+
+        let mockRequest = MockHTTPRequest(eventLoop: eventLoop.next(), requiresEventLoopForChannel: false)
+        let request = HTTPConnectionPool.Request(mockRequest)
+
+        let executeAction = state.executeRequest(request)
+        guard case .createConnection(_, on: _) = executeAction.connection else {
+            return XCTFail("Expected to create a connection")
+        }
+        XCTAssertEqual(executeAction.request, .scheduleRequestTimeout(for: request, on: mockRequest.eventLoop))
+
+        let timeoutAction = state.timeoutRequest(request.id)
+        XCTAssertEqual(timeoutAction.request, .failRequest(request, HTTPClientError.connectTimeout, cancelTimeout: false))
+        XCTAssertEqual(timeoutAction.connection, .none)
+    }
+
+    func testRequestThatTimesOutAfterAConnectionWasEstablishedSuccessfullyTimesOutWithGenericError() {
+        let elg = EmbeddedEventLoopGroup(loops: 1)
+        defer { XCTAssertNoThrow(try elg.syncShutdownGracefully()) }
+
+        var state = HTTPConnectionPool.StateMachine(
+            eventLoopGroup: elg,
+            idGenerator: .init(),
+            maximumConcurrentHTTP1Connections: 6
+        )
+
+        let mockRequest1 = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
+        let request1 = HTTPConnectionPool.Request(mockRequest1)
+
+        let executeAction1 = state.executeRequest(request1)
+        guard case .createConnection(let connectionID1, on: let connEL1) = executeAction1.connection else {
+            return XCTFail("Expected to create a connection")
+        }
+        XCTAssert(mockRequest1.eventLoop === connEL1)
+
+        XCTAssertEqual(executeAction1.request, .scheduleRequestTimeout(for: request1, on: mockRequest1.eventLoop))
+
+        let mockRequest2 = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
+        let request2 = HTTPConnectionPool.Request(mockRequest2)
+
+        let executeAction2 = state.executeRequest(request2)
+        guard case .createConnection(let connectionID2, on: let connEL2) = executeAction2.connection else {
+            return XCTFail("Expected to create a connection")
+        }
+        XCTAssert(mockRequest2.eventLoop === connEL2)
+
+        XCTAssertEqual(executeAction2.request, .scheduleRequestTimeout(for: request2, on: connEL1))
+
+        let failAction = state.failedToCreateNewConnection(HTTPClientError.httpProxyHandshakeTimeout, connectionID: connectionID1)
+        guard case .scheduleBackoffTimer(connectionID1, backoff: _, on: let timerEL) = failAction.connection else {
+            return XCTFail("Expected to create a backoff timer")
+        }
+        XCTAssert(timerEL === connEL2)
+        XCTAssertEqual(failAction.request, .none)
+
+        let conn2 = HTTPConnectionPool.Connection.__testOnly_connection(id: connectionID2, eventLoop: connEL2)
+        let createdAction = state.newHTTP1ConnectionCreated(conn2)
+
+        XCTAssertEqual(createdAction.request, .executeRequest(request1, conn2, cancelTimeout: true))
+        XCTAssertEqual(createdAction.connection, .none)
+
+        let timeoutAction = state.timeoutRequest(request2.id)
+        XCTAssertEqual(timeoutAction.request, .failRequest(request2, HTTPClientError.getConnectionFromPoolTimeout, cancelTimeout: false))
+        XCTAssertEqual(timeoutAction.connection, .none)
+    }
 }

Diff for: Tests/AsyncHTTPClientTests/HTTPConnectionPoolTests.swift

@@ -229,7 +229,7 @@ class HTTPConnectionPoolTests: XCTestCase {
 
         pool.executeRequest(requestBag)
         XCTAssertThrowsError(try requestBag.task.futureResult.wait()) {
-            XCTAssertEqual($0 as? HTTPClientError, .getConnectionFromPoolTimeout)
+            XCTAssertEqual($0 as? HTTPClientError, .proxyAuthenticationRequired)
         }
         XCTAssertGreaterThanOrEqual(httpBin.createdConnections, 8)
         XCTAssertEqual(httpBin.activeConnections, 0)