From ab6142bed6812f3b95ba9bcecc99c4a97f02511b Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 6 Sep 2022 14:52:45 +0200 Subject: [PATCH 1/6] Allow immediate request failure on connection error --- .../ConnectionPool/HTTPConnectionPool.swift | 3 +- ...HTTPConnectionPool+HTTP1StateMachine.swift | 30 +++++ ...HTTPConnectionPool+HTTP2StateMachine.swift | 37 +++++ .../HTTPConnectionPool+StateMachine.swift | 11 +- Sources/AsyncHTTPClient/HTTPClient.swift | 7 + .../HTTPClientNIOTSTests.swift | 3 + .../HTTPConnectionPool+HTTP1StateTests.swift | 96 +++++++++++-- ...onnectionPool+HTTP2StateMachineTests.swift | 126 +++++++++++++++--- .../Mocks/MockConnectionPool.swift | 6 +- 9 files changed, 292 insertions(+), 27 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift index 5a0b2708e..315a2d204 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift @@ -70,7 +70,8 @@ final class HTTPConnectionPool { self._state = StateMachine( idGenerator: idGenerator, - maximumConcurrentHTTP1Connections: clientConfiguration.connectionPool.concurrentHTTP1ConnectionsPerHostSoftLimit + maximumConcurrentHTTP1Connections: clientConfiguration.connectionPool.concurrentHTTP1ConnectionsPerHostSoftLimit, + retryConnectionEstablishment: clientConfiguration.connectionPool.retryConnectionEstablishment ) } diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift index 2cd667bb3..35cf2021a 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift @@ -17,6 +17,7 @@ import NIOCore extension HTTPConnectionPool { struct HTTP1StateMachine { typealias Action = HTTPConnectionPool.StateMachine.Action + typealias RequestAction = HTTPConnectionPool.StateMachine.RequestAction typealias ConnectionMigrationAction = HTTPConnectionPool.StateMachine.ConnectionMigrationAction typealias EstablishedAction = HTTPConnectionPool.StateMachine.EstablishedAction typealias EstablishedConnectionAction = HTTPConnectionPool.StateMachine.EstablishedConnectionAction @@ -29,16 +30,19 @@ extension HTTPConnectionPool { private(set) var requests: RequestQueue private(set) var lifecycleState: StateMachine.LifecycleState + private let retryConnectionEstablishment: Bool init( idGenerator: Connection.ID.Generator, maximumConcurrentConnections: Int, + retryConnectionEstablishment: Bool, lifecycleState: StateMachine.LifecycleState ) { self.connections = HTTP1Connections( maximumConcurrentConnections: maximumConcurrentConnections, generator: idGenerator ) + self.retryConnectionEstablishment = retryConnectionEstablishment self.requests = RequestQueue() self.lifecycleState = lifecycleState @@ -219,6 +223,17 @@ extension HTTPConnectionPool { switch self.lifecycleState { case .running: + guard self.retryConnectionEstablishment else { + guard let (index, _) = self.connections.failConnection(connectionID) else { + preconditionFailure("Failed to create a connection that is unknown to us?") + } + self.connections.removeConnection(at: index) + + return .init( + request: self.failAllRequests(reason: error), + connection: .none + ) + } // We don't care how many waiting requests we have at this point, we will schedule a // retry. More tasks, may appear until the backoff has completed. The final // decision about the retry will be made in `connectionCreationBackoffDone(_:)` @@ -243,6 +258,13 @@ extension HTTPConnectionPool { mutating func waitingForConnectivity(_ error: Error, connectionID: Connection.ID) -> Action { self.lastConnectFailure = error + + guard self.retryConnectionEstablishment else { + return .init( + request: self.failAllRequests(reason: error), + connection: .none + ) + } return .init(request: .none, connection: .none) } @@ -522,6 +544,14 @@ extension HTTPConnectionPool { self.connections.removeConnection(at: index) return .none } + + private mutating func failAllRequests(reason error: Error) -> RequestAction { + let allRequests = requests.removeAll() + guard !allRequests.isEmpty else { + return .none + } + return .failRequestsAndCancelTimeouts(allRequests, error) + } // MARK: HTTP2 diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift index d517d82e6..6a6fadfc4 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift @@ -18,6 +18,7 @@ import NIOHTTP2 extension HTTPConnectionPool { struct HTTP2StateMachine { typealias Action = HTTPConnectionPool.StateMachine.Action + typealias RequestAction = HTTPConnectionPool.StateMachine.RequestAction typealias ConnectionMigrationAction = HTTPConnectionPool.StateMachine.ConnectionMigrationAction typealias EstablishedAction = HTTPConnectionPool.StateMachine.EstablishedAction typealias EstablishedConnectionAction = HTTPConnectionPool.StateMachine.EstablishedConnectionAction @@ -33,9 +34,11 @@ extension HTTPConnectionPool { private let idGenerator: Connection.ID.Generator private(set) var lifecycleState: StateMachine.LifecycleState + private let retryConnectionEstablishment: Bool init( idGenerator: Connection.ID.Generator, + retryConnectionEstablishment: Bool, lifecycleState: StateMachine.LifecycleState ) { self.idGenerator = idGenerator @@ -43,6 +46,7 @@ extension HTTPConnectionPool { self.connections = HTTP2Connections(generator: idGenerator) self.lifecycleState = lifecycleState + self.retryConnectionEstablishment = retryConnectionEstablishment } mutating func migrateFromHTTP1( @@ -400,6 +404,18 @@ extension HTTPConnectionPool { mutating func failedToCreateNewConnection(_ error: Error, connectionID: Connection.ID) -> Action { self.failedConsecutiveConnectionAttempts += 1 self.lastConnectFailure = error + + guard self.retryConnectionEstablishment else { + guard let (index, _) = self.connections.failConnection(connectionID) else { + preconditionFailure("Failed to create a connection that is unknown to us?") + } + self.connections.removeConnection(at: index) + + return .init( + request: self.failAllRequests(reason: error), + connection: .none + ) + } let eventLoop = self.connections.backoffNextConnectionAttempt(connectionID) let backoff = calculateBackoff(failedAttempt: self.failedConsecutiveConnectionAttempts) @@ -408,6 +424,19 @@ extension HTTPConnectionPool { mutating func waitingForConnectivity(_ error: Error, connectionID: Connection.ID) -> Action { self.lastConnectFailure = error + + guard self.retryConnectionEstablishment else { + guard let (index, _) = self.connections.failConnection(connectionID) else { + preconditionFailure("Failed to create a connection that is unknown to us?") + } + _ = self.connections.closeConnection(at: index) + + return .init( + request: self.failAllRequests(reason: error), + connection: .none + ) + } + return .init(request: .none, connection: .none) } @@ -420,6 +449,14 @@ extension HTTPConnectionPool { } return self.nextActionForFailedConnection(at: index, on: context.eventLoop) } + + private mutating func failAllRequests(reason error: Error) -> RequestAction { + let allRequests = requests.removeAll() + guard !allRequests.isEmpty else { + return .none + } + return .failRequestsAndCancelTimeouts(allRequests, error) + } mutating func timeoutRequest(_ requestID: Request.ID) -> Action { // 1. check requests in queue diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift index 63f3e5a9a..10579cbcb 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift @@ -96,13 +96,20 @@ extension HTTPConnectionPool { let idGenerator: Connection.ID.Generator let maximumConcurrentHTTP1Connections: Int + let retryConnectionEstablishment: Bool - init(idGenerator: Connection.ID.Generator, maximumConcurrentHTTP1Connections: Int) { + init( + idGenerator: Connection.ID.Generator, + maximumConcurrentHTTP1Connections: Int, + retryConnectionEstablishment: Bool + ) { self.maximumConcurrentHTTP1Connections = maximumConcurrentHTTP1Connections + self.retryConnectionEstablishment = retryConnectionEstablishment self.idGenerator = idGenerator let http1State = HTTP1StateMachine( idGenerator: idGenerator, maximumConcurrentConnections: maximumConcurrentHTTP1Connections, + retryConnectionEstablishment: retryConnectionEstablishment, lifecycleState: .running ) self.state = .http1(http1State) @@ -127,6 +134,7 @@ extension HTTPConnectionPool { var http1StateMachine = HTTP1StateMachine( idGenerator: self.idGenerator, maximumConcurrentConnections: self.maximumConcurrentHTTP1Connections, + retryConnectionEstablishment: self.retryConnectionEstablishment, lifecycleState: http2StateMachine.lifecycleState ) @@ -147,6 +155,7 @@ extension HTTPConnectionPool { var http2StateMachine = HTTP2StateMachine( idGenerator: self.idGenerator, + retryConnectionEstablishment: retryConnectionEstablishment, lifecycleState: http1StateMachine.lifecycleState ) let migrationAction = http2StateMachine.migrateFromHTTP1( diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 55d2573ba..51ca06a70 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -952,6 +952,12 @@ extension HTTPClient.Configuration { /// The maximum number of connections that are kept alive in the connection pool per host. If requests with /// an explicit eventLoopRequirement are sent, this number might be exceeded due to overflow connections. public var concurrentHTTP1ConnectionsPerHostSoftLimit: Int + + /// If true, ``HTTPClient`` will try to create new connections on connection failure with an exponential backoff. + /// Requests will only fail after the ``HTTPClient/Configuration/Timeout-swift.struct/connect`` timeout exceeded. + /// If false, requests will fail immediately after a connection could not be established. + /// Defaults to `true` + var retryConnectionEstablishment: Bool public init(idleTimeout: TimeAmount = .seconds(60)) { self.init(idleTimeout: idleTimeout, concurrentHTTP1ConnectionsPerHostSoftLimit: 8) @@ -960,6 +966,7 @@ extension HTTPClient.Configuration { public init(idleTimeout: TimeAmount, concurrentHTTP1ConnectionsPerHostSoftLimit: Int) { self.idleTimeout = idleTimeout self.concurrentHTTP1ConnectionsPerHostSoftLimit = concurrentHTTP1ConnectionsPerHostSoftLimit + self.retryConnectionEstablishment = true } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift index 3b659a14a..9727746cc 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientNIOTSTests.swift @@ -57,6 +57,7 @@ class HTTPClientNIOTSTests: XCTestCase { let httpBin = HTTPBin(.http1_1(ssl: true)) var config = HTTPClient.Configuration() config.networkFrameworkWaitForConnectivity = false + config.connectionPool.retryConnectionEstablishment = false let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: config) defer { @@ -85,6 +86,7 @@ class HTTPClientNIOTSTests: XCTestCase { let httpBin = HTTPBin(.http1_1(ssl: false)) var config = HTTPClient.Configuration() config.networkFrameworkWaitForConnectivity = false + config.connectionPool.retryConnectionEstablishment = false let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: config) @@ -140,6 +142,7 @@ class HTTPClientNIOTSTests: XCTestCase { var clientConfig = HTTPClient.Configuration(tlsConfiguration: tlsConfig) clientConfig.networkFrameworkWaitForConnectivity = false + clientConfig.connectionPool.retryConnectionEstablishment = false let httpClient = HTTPClient( eventLoopGroupProvider: .shared(self.clientGroup), configuration: clientConfig diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift index 7f59fd4e1..2d94d2bf5 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift @@ -27,7 +27,8 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { var state = HTTPConnectionPool.StateMachine( idGenerator: .init(), - maximumConcurrentHTTP1Connections: 8 + maximumConcurrentHTTP1Connections: 8, + retryConnectionEstablishment: true ) var connections = MockConnectionPool() @@ -101,6 +102,78 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { XCTAssert(queuer.isEmpty) XCTAssert(connections.isEmpty) } + + func testCreatingAndFailingConnectionsWithoutRetry() { + struct SomeError: Error, Equatable {} + let elg = EmbeddedEventLoopGroup(loops: 4) + defer { XCTAssertNoThrow(try elg.syncShutdownGracefully()) } + + var state = HTTPConnectionPool.StateMachine( + idGenerator: .init(), + maximumConcurrentHTTP1Connections: 8, + retryConnectionEstablishment: false + ) + + var connections = MockConnectionPool() + var queuer = MockRequestQueuer() + + // for the first eight requests, the pool should try to create new connections. + + for _ in 0..<8 { + let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let request = HTTPConnectionPool.Request(mockRequest) + let action = state.executeRequest(request) + guard case .createConnection(let connectionID, let connectionEL) = action.connection else { + return XCTFail("Unexpected connection action") + } + XCTAssertEqual(.scheduleRequestTimeout(for: request, on: mockRequest.eventLoop), action.request) + XCTAssert(connectionEL === mockRequest.eventLoop) + + XCTAssertNoThrow(try connections.createConnection(connectionID, on: connectionEL)) + XCTAssertNoThrow(try queuer.queue(mockRequest, id: request.id)) + } + + // the next eight requests should only be queued. + + for _ in 0..<8 { + let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let request = HTTPConnectionPool.Request(mockRequest) + let action = state.executeRequest(request) + guard case .none = action.connection else { + return XCTFail("Unexpected connection action") + } + XCTAssertEqual(.scheduleRequestTimeout(for: request, on: mockRequest.eventLoop), action.request) + XCTAssertNoThrow(try queuer.queue(mockRequest, id: request.id)) + } + + // timeout all queued requests except for two + + // fail all connection attempts + + // the first failure should cancel all requests because we have disabled connection establishtment retry + let randomConnectionID = connections.randomStartingConnection()! + XCTAssertNoThrow(try connections.failConnectionCreation(randomConnectionID)) + let action = state.failedToCreateNewConnection(SomeError(), connectionID: randomConnectionID) + XCTAssertEqual(action.connection, .none) + guard case .failRequestsAndCancelTimeouts(let requestsToFail, let requestError) = action.request else { + return XCTFail("Unexpected request action: \(action.request)") + } + XCTAssertEqualTypeAndValue(requestError, SomeError()) + for requestToFail in requestsToFail { + XCTAssertNoThrow(try queuer.fail(requestToFail.id, request: requestToFail.__testOnly_wrapped_request())) + } + + // all requests have been canceled and therefore nothing should happen if a connection fails + while let randomConnectionID = connections.randomStartingConnection() { + XCTAssertNoThrow(try connections.failConnectionCreation(randomConnectionID)) + let action = state.failedToCreateNewConnection(SomeError(), connectionID: randomConnectionID) + + XCTAssertEqual(action, .none) + } + + XCTAssert(queuer.isEmpty) + XCTAssert(connections.isEmpty) + } func testConnectionFailureBackoff() { let elg = EmbeddedEventLoopGroup(loops: 4) @@ -108,7 +181,8 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { var state = HTTPConnectionPool.StateMachine( idGenerator: .init(), - maximumConcurrentHTTP1Connections: 2 + maximumConcurrentHTTP1Connections: 2, + retryConnectionEstablishment: true ) let mockRequest = MockHTTPRequest(eventLoop: elg.next()) @@ -165,7 +239,8 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { var state = HTTPConnectionPool.StateMachine( idGenerator: .init(), - maximumConcurrentHTTP1Connections: 2 + maximumConcurrentHTTP1Connections: 2, + retryConnectionEstablishment: true ) let mockRequest = MockHTTPRequest(eventLoop: elg.next()) @@ -201,7 +276,8 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { var state = HTTPConnectionPool.StateMachine( idGenerator: .init(), - maximumConcurrentHTTP1Connections: 2 + maximumConcurrentHTTP1Connections: 2, + retryConnectionEstablishment: true ) let mockRequest = MockHTTPRequest(eventLoop: elg.next()) @@ -591,7 +667,8 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { var state = HTTPConnectionPool.StateMachine( idGenerator: .init(), - maximumConcurrentHTTP1Connections: 6 + maximumConcurrentHTTP1Connections: 6, + retryConnectionEstablishment: true ) let mockRequest = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) @@ -629,7 +706,8 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { var state = HTTPConnectionPool.StateMachine( idGenerator: .init(), - maximumConcurrentHTTP1Connections: 6 + maximumConcurrentHTTP1Connections: 6, + retryConnectionEstablishment: true ) let mockRequest = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) @@ -660,7 +738,8 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { var state = HTTPConnectionPool.StateMachine( idGenerator: .init(), - maximumConcurrentHTTP1Connections: 6 + maximumConcurrentHTTP1Connections: 6, + retryConnectionEstablishment: true ) let mockRequest = MockHTTPRequest(eventLoop: eventLoop.next(), requiresEventLoopForChannel: false) @@ -683,7 +762,8 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { var state = HTTPConnectionPool.StateMachine( idGenerator: .init(), - maximumConcurrentHTTP1Connections: 6 + maximumConcurrentHTTP1Connections: 6, + retryConnectionEstablishment: true ) let mockRequest1 = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false) diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift index e42a98ac7..6e5e7e331 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift @@ -29,7 +29,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let el1 = elg.next() var connections = MockConnectionPool() var queuer = MockRequestQueuer() - var state = HTTPConnectionPool.HTTP2StateMachine(idGenerator: .init(), lifecycleState: .running) + var state = HTTPConnectionPool.HTTP2StateMachine( + idGenerator: .init(), + retryConnectionEstablishment: true, + lifecycleState: .running + ) /// first request should create a new connection let mockRequest = MockHTTPRequest(eventLoop: el1) @@ -138,6 +142,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { var state = HTTPConnectionPool.HTTP2StateMachine( idGenerator: .init(), + retryConnectionEstablishment: true, lifecycleState: .running ) @@ -188,6 +193,38 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { // 4. retry connection, but no more queued requests. XCTAssertEqual(state.connectionCreationBackoffDone(newConnectionID), .none) } + + func testConnectionFailureWithoutRetry() { + struct SomeError: Error, Equatable {} + let elg = EmbeddedEventLoopGroup(loops: 4) + defer { XCTAssertNoThrow(try elg.syncShutdownGracefully()) } + + var state = HTTPConnectionPool.HTTP2StateMachine( + idGenerator: .init(), + retryConnectionEstablishment: false, + lifecycleState: .running + ) + + let mockRequest = MockHTTPRequest(eventLoop: elg.next()) + let request = HTTPConnectionPool.Request(mockRequest) + + let action = state.executeRequest(request) + XCTAssertEqual(.scheduleRequestTimeout(for: request, on: mockRequest.eventLoop), action.request) + + // 1. connection attempt + guard case .createConnection(let connectionID, on: let connectionEL) = action.connection else { + return XCTFail("Unexpected connection action: \(action.connection)") + } + XCTAssert(connectionEL === mockRequest.eventLoop) // XCTAssertIdentical not available on Linux + + let failedConnectAction = state.failedToCreateNewConnection(SomeError(), connectionID: connectionID) + XCTAssertEqual(failedConnectAction.connection, .none) + guard case .failRequestsAndCancelTimeouts(let requestsToFail, let requestError) = failedConnectAction.request else { + return XCTFail("Unexpected request action: \(action.request)") + } + XCTAssertEqualTypeAndValue(requestError, SomeError()) + XCTAssertEqualTypeAndValue(requestsToFail, [request]) + } func testCancelRequestWorks() { let elg = EmbeddedEventLoopGroup(loops: 4) @@ -195,6 +232,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { var state = HTTPConnectionPool.HTTP2StateMachine( idGenerator: .init(), + retryConnectionEstablishment: true, lifecycleState: .running ) @@ -233,6 +271,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { var state = HTTPConnectionPool.HTTP2StateMachine( idGenerator: .init(), + retryConnectionEstablishment: true, lifecycleState: .running ) @@ -287,7 +326,12 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let el1 = elg.next() let idGenerator = HTTPConnectionPool.Connection.ID.Generator() - var http1State = HTTPConnectionPool.HTTP1StateMachine(idGenerator: idGenerator, maximumConcurrentConnections: 8, lifecycleState: .running) + var http1State = HTTPConnectionPool.HTTP1StateMachine( + idGenerator: idGenerator, + maximumConcurrentConnections: 8, + retryConnectionEstablishment: true, + lifecycleState: .running + ) let mockRequest1 = MockHTTPRequest(eventLoop: el1) let request1 = HTTPConnectionPool.Request(mockRequest1) @@ -313,7 +357,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { // second connection is a HTTP2 connection and we need to migrate let conn2: HTTPConnectionPool.Connection = .__testOnly_connection(id: conn2ID, eventLoop: el1) - var http2State = HTTPConnectionPool.HTTP2StateMachine(idGenerator: idGenerator, lifecycleState: .running) + var http2State = HTTPConnectionPool.HTTP2StateMachine( + idGenerator: idGenerator, + retryConnectionEstablishment: true, + lifecycleState: .running + ) let http2ConnectAction = http2State.migrateFromHTTP1( http1Connections: http1State.connections, @@ -353,7 +401,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let idGenerator = HTTPConnectionPool.Connection.ID.Generator() var http1Conns = HTTPConnectionPool.HTTP1Connections(maximumConcurrentConnections: 8, generator: idGenerator) let conn1ID = http1Conns.createNewConnection(on: el1) - var state = HTTPConnectionPool.HTTP2StateMachine(idGenerator: idGenerator, lifecycleState: .running) + var state = HTTPConnectionPool.HTTP2StateMachine( + idGenerator: idGenerator, + retryConnectionEstablishment: true, + lifecycleState: .running + ) let conn1 = HTTPConnectionPool.Connection.__testOnly_connection(id: conn1ID, eventLoop: el1) let connectAction = state.migrateFromHTTP1(http1Connections: http1Conns, requests: .init(), newHTTP2Connection: conn1, maxConcurrentStreams: 100) @@ -398,7 +450,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let idGenerator = HTTPConnectionPool.Connection.ID.Generator() var http1Conns = HTTPConnectionPool.HTTP1Connections(maximumConcurrentConnections: 8, generator: idGenerator) let conn1ID = http1Conns.createNewConnection(on: el1) - var state = HTTPConnectionPool.HTTP2StateMachine(idGenerator: idGenerator, lifecycleState: .running) + var state = HTTPConnectionPool.HTTP2StateMachine( + idGenerator: idGenerator, + retryConnectionEstablishment: true, + lifecycleState: .running + ) let conn1 = HTTPConnectionPool.Connection.__testOnly_connection(id: conn1ID, eventLoop: el1) let connectAction = state.migrateFromHTTP1(http1Connections: http1Conns, requests: .init(), newHTTP2Connection: conn1, maxConcurrentStreams: 100) @@ -426,7 +482,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let idGenerator = HTTPConnectionPool.Connection.ID.Generator() var http1Conns = HTTPConnectionPool.HTTP1Connections(maximumConcurrentConnections: 8, generator: idGenerator) let conn1ID = http1Conns.createNewConnection(on: el1) - var state = HTTPConnectionPool.HTTP2StateMachine(idGenerator: idGenerator, lifecycleState: .running) + var state = HTTPConnectionPool.HTTP2StateMachine( + idGenerator: idGenerator, + retryConnectionEstablishment: true, + lifecycleState: .running + ) let conn1 = HTTPConnectionPool.Connection.__testOnly_connection(id: conn1ID, eventLoop: el1) let connectAction = state.migrateFromHTTP1(http1Connections: http1Conns, requests: .init(), newHTTP2Connection: conn1, maxConcurrentStreams: 100) XCTAssertEqual(connectAction.request, .none) @@ -461,7 +521,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let idGenerator = HTTPConnectionPool.Connection.ID.Generator() var http1Conns = HTTPConnectionPool.HTTP1Connections(maximumConcurrentConnections: 8, generator: idGenerator) let conn1ID = http1Conns.createNewConnection(on: el1) - var state = HTTPConnectionPool.HTTP2StateMachine(idGenerator: idGenerator, lifecycleState: .running) + var state = HTTPConnectionPool.HTTP2StateMachine( + idGenerator: idGenerator, + retryConnectionEstablishment: true, + lifecycleState: .running + ) let conn1 = HTTPConnectionPool.Connection.__testOnly_connection(id: conn1ID, eventLoop: el1) @@ -491,7 +555,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let idGenerator = HTTPConnectionPool.Connection.ID.Generator() var http1Conns = HTTPConnectionPool.HTTP1Connections(maximumConcurrentConnections: 8, generator: idGenerator) let conn1ID = http1Conns.createNewConnection(on: el1) - var state = HTTPConnectionPool.HTTP2StateMachine(idGenerator: idGenerator, lifecycleState: .running) + var state = HTTPConnectionPool.HTTP2StateMachine( + idGenerator: idGenerator, + retryConnectionEstablishment: true, + lifecycleState: .running + ) let conn1 = HTTPConnectionPool.Connection.__testOnly_connection(id: conn1ID, eventLoop: el1) let connectAction = state.migrateFromHTTP1( @@ -532,7 +600,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let idGenerator = HTTPConnectionPool.Connection.ID.Generator() var http1Conns = HTTPConnectionPool.HTTP1Connections(maximumConcurrentConnections: 8, generator: idGenerator) let conn1ID = http1Conns.createNewConnection(on: el1) - var state = HTTPConnectionPool.HTTP2StateMachine(idGenerator: idGenerator, lifecycleState: .running) + var state = HTTPConnectionPool.HTTP2StateMachine( + idGenerator: idGenerator, + retryConnectionEstablishment: true, + lifecycleState: .running + ) let conn1 = HTTPConnectionPool.Connection.__testOnly_connection(id: conn1ID, eventLoop: el1) let connectAction1 = state.migrateFromHTTP1( @@ -592,7 +664,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let el1 = elg.next() var connections = MockConnectionPool() var queuer = MockRequestQueuer() - var state = HTTPConnectionPool.StateMachine(idGenerator: .init(), maximumConcurrentHTTP1Connections: 8) + var state = HTTPConnectionPool.StateMachine( + idGenerator: .init(), + maximumConcurrentHTTP1Connections: 8, + retryConnectionEstablishment: true + ) /// first 8 request should create a new connection var connectionIDs: [HTTPConnectionPool.Connection.ID] = [] @@ -678,7 +754,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let el1 = elg.next() var connections = MockConnectionPool() var queuer = MockRequestQueuer() - var state = HTTPConnectionPool.StateMachine(idGenerator: .init(), maximumConcurrentHTTP1Connections: 8) + var state = HTTPConnectionPool.StateMachine( + idGenerator: .init(), + maximumConcurrentHTTP1Connections: 8, + retryConnectionEstablishment: true + ) /// create a new connection let mockRequest = MockHTTPRequest(eventLoop: el1) @@ -720,7 +800,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let el1 = elg.next() var connections = MockConnectionPool() var queuer = MockRequestQueuer() - var state = HTTPConnectionPool.StateMachine(idGenerator: .init(), maximumConcurrentHTTP1Connections: 8) + var state = HTTPConnectionPool.StateMachine( + idGenerator: .init(), + maximumConcurrentHTTP1Connections: 8, + retryConnectionEstablishment: true + ) /// first 8 request should create a new connection var connectionIDs: [HTTPConnectionPool.Connection.ID] = [] @@ -855,7 +939,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let el2 = elg.next() var connections = MockConnectionPool() var queuer = MockRequestQueuer() - var state = HTTPConnectionPool.StateMachine(idGenerator: .init(), maximumConcurrentHTTP1Connections: 8) + var state = HTTPConnectionPool.StateMachine( + idGenerator: .init(), + maximumConcurrentHTTP1Connections: 8, + retryConnectionEstablishment: true + ) // create http2 connection let mockRequest = MockHTTPRequest(eventLoop: el1) @@ -921,7 +1009,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let el2 = elg.next() var connections = MockConnectionPool() var queuer = MockRequestQueuer() - var state = HTTPConnectionPool.StateMachine(idGenerator: .init(), maximumConcurrentHTTP1Connections: 8) + var state = HTTPConnectionPool.StateMachine( + idGenerator: .init(), + maximumConcurrentHTTP1Connections: 8, + retryConnectionEstablishment: true + ) // create http2 connection let mockRequest = MockHTTPRequest(eventLoop: el1) @@ -993,7 +1085,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let el2 = elg.next() var connections = MockConnectionPool() var queuer = MockRequestQueuer() - var state = HTTPConnectionPool.StateMachine(idGenerator: .init(), maximumConcurrentHTTP1Connections: 8) + var state = HTTPConnectionPool.StateMachine( + idGenerator: .init(), + maximumConcurrentHTTP1Connections: 8, + retryConnectionEstablishment: true + ) var connectionIDs: [HTTPConnectionPool.Connection.ID] = [] for el in [el1, el2, el2] { diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift index eedc499ad..1b2c27b68 100644 --- a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift +++ b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift @@ -541,7 +541,8 @@ extension MockConnectionPool { ) throws -> (Self, HTTPConnectionPool.StateMachine) { var state = HTTPConnectionPool.StateMachine( idGenerator: .init(), - maximumConcurrentHTTP1Connections: maxNumberOfConnections + maximumConcurrentHTTP1Connections: maxNumberOfConnections, + retryConnectionEstablishment: true ) var connections = MockConnectionPool() var queuer = MockRequestQueuer() @@ -604,7 +605,8 @@ extension MockConnectionPool { ) throws -> (Self, HTTPConnectionPool.StateMachine) { var state = HTTPConnectionPool.StateMachine( idGenerator: .init(), - maximumConcurrentHTTP1Connections: 8 + maximumConcurrentHTTP1Connections: 8, + retryConnectionEstablishment: true ) var connections = MockConnectionPool() var queuer = MockRequestQueuer() From cfce59ee8459c038806cc3981e27bba209ebc980 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 6 Sep 2022 17:22:55 +0200 Subject: [PATCH 2/6] generate linux tests and run SwiftFormat --- .../HTTPConnectionPool+HTTP1StateMachine.swift | 8 ++++---- .../HTTPConnectionPool+HTTP2StateMachine.swift | 14 +++++++------- .../HTTPConnectionPool+StateMachine.swift | 2 +- Sources/AsyncHTTPClient/HTTPClient.swift | 2 +- ...HTTPConnectionPool+HTTP1StateTests+XCTest.swift | 1 + .../HTTPConnectionPool+HTTP1StateTests.swift | 8 ++++---- ...nectionPool+HTTP2StateMachineTests+XCTest.swift | 1 + ...HTTPConnectionPool+HTTP2StateMachineTests.swift | 2 +- 8 files changed, 20 insertions(+), 18 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift index 35cf2021a..aea773f35 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift @@ -228,7 +228,7 @@ extension HTTPConnectionPool { preconditionFailure("Failed to create a connection that is unknown to us?") } self.connections.removeConnection(at: index) - + return .init( request: self.failAllRequests(reason: error), connection: .none @@ -258,7 +258,7 @@ extension HTTPConnectionPool { mutating func waitingForConnectivity(_ error: Error, connectionID: Connection.ID) -> Action { self.lastConnectFailure = error - + guard self.retryConnectionEstablishment else { return .init( request: self.failAllRequests(reason: error), @@ -544,9 +544,9 @@ extension HTTPConnectionPool { self.connections.removeConnection(at: index) return .none } - + private mutating func failAllRequests(reason error: Error) -> RequestAction { - let allRequests = requests.removeAll() + let allRequests = self.requests.removeAll() guard !allRequests.isEmpty else { return .none } diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift index 6a6fadfc4..e7a4e3340 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift @@ -404,13 +404,13 @@ extension HTTPConnectionPool { mutating func failedToCreateNewConnection(_ error: Error, connectionID: Connection.ID) -> Action { self.failedConsecutiveConnectionAttempts += 1 self.lastConnectFailure = error - + guard self.retryConnectionEstablishment else { guard let (index, _) = self.connections.failConnection(connectionID) else { preconditionFailure("Failed to create a connection that is unknown to us?") } self.connections.removeConnection(at: index) - + return .init( request: self.failAllRequests(reason: error), connection: .none @@ -424,19 +424,19 @@ extension HTTPConnectionPool { mutating func waitingForConnectivity(_ error: Error, connectionID: Connection.ID) -> Action { self.lastConnectFailure = error - + guard self.retryConnectionEstablishment else { guard let (index, _) = self.connections.failConnection(connectionID) else { preconditionFailure("Failed to create a connection that is unknown to us?") } _ = self.connections.closeConnection(at: index) - + return .init( request: self.failAllRequests(reason: error), connection: .none ) } - + return .init(request: .none, connection: .none) } @@ -449,9 +449,9 @@ extension HTTPConnectionPool { } return self.nextActionForFailedConnection(at: index, on: context.eventLoop) } - + private mutating func failAllRequests(reason error: Error) -> RequestAction { - let allRequests = requests.removeAll() + let allRequests = self.requests.removeAll() guard !allRequests.isEmpty else { return .none } diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift index 10579cbcb..4412e265d 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift @@ -155,7 +155,7 @@ extension HTTPConnectionPool { var http2StateMachine = HTTP2StateMachine( idGenerator: self.idGenerator, - retryConnectionEstablishment: retryConnectionEstablishment, + retryConnectionEstablishment: self.retryConnectionEstablishment, lifecycleState: http1StateMachine.lifecycleState ) let migrationAction = http2StateMachine.migrateFromHTTP1( diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 51ca06a70..3fa620cb4 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -952,7 +952,7 @@ extension HTTPClient.Configuration { /// The maximum number of connections that are kept alive in the connection pool per host. If requests with /// an explicit eventLoopRequirement are sent, this number might be exceeded due to overflow connections. public var concurrentHTTP1ConnectionsPerHostSoftLimit: Int - + /// If true, ``HTTPClient`` will try to create new connections on connection failure with an exponential backoff. /// Requests will only fail after the ``HTTPClient/Configuration/Timeout-swift.struct/connect`` timeout exceeded. /// If false, requests will fail immediately after a connection could not be established. diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests+XCTest.swift index 16377d07f..d50ab9893 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests+XCTest.swift @@ -26,6 +26,7 @@ extension HTTPConnectionPool_HTTP1StateMachineTests { static var allTests: [(String, (HTTPConnectionPool_HTTP1StateMachineTests) -> () throws -> Void)] { return [ ("testCreatingAndFailingConnections", testCreatingAndFailingConnections), + ("testCreatingAndFailingConnectionsWithoutRetry", testCreatingAndFailingConnectionsWithoutRetry), ("testConnectionFailureBackoff", testConnectionFailureBackoff), ("testCancelRequestWorks", testCancelRequestWorks), ("testExecuteOnShuttingDownPool", testExecuteOnShuttingDownPool), diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift index 2d94d2bf5..9d045e21a 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift @@ -102,7 +102,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { XCTAssert(queuer.isEmpty) XCTAssert(connections.isEmpty) } - + func testCreatingAndFailingConnectionsWithoutRetry() { struct SomeError: Error, Equatable {} let elg = EmbeddedEventLoopGroup(loops: 4) @@ -149,7 +149,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { // timeout all queued requests except for two // fail all connection attempts - + // the first failure should cancel all requests because we have disabled connection establishtment retry let randomConnectionID = connections.randomStartingConnection()! XCTAssertNoThrow(try connections.failConnectionCreation(randomConnectionID)) @@ -162,7 +162,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { for requestToFail in requestsToFail { XCTAssertNoThrow(try queuer.fail(requestToFail.id, request: requestToFail.__testOnly_wrapped_request())) } - + // all requests have been canceled and therefore nothing should happen if a connection fails while let randomConnectionID = connections.randomStartingConnection() { XCTAssertNoThrow(try connections.failConnectionCreation(randomConnectionID)) @@ -170,7 +170,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { XCTAssertEqual(action, .none) } - + XCTAssert(queuer.isEmpty) XCTAssert(connections.isEmpty) } diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests+XCTest.swift index 9dca0c934..95ea8c580 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests+XCTest.swift @@ -27,6 +27,7 @@ extension HTTPConnectionPool_HTTP2StateMachineTests { return [ ("testCreatingOfConnection", testCreatingOfConnection), ("testConnectionFailureBackoff", testConnectionFailureBackoff), + ("testConnectionFailureWithoutRetry", testConnectionFailureWithoutRetry), ("testCancelRequestWorks", testCancelRequestWorks), ("testExecuteOnShuttingDownPool", testExecuteOnShuttingDownPool), ("testHTTP1ToHTTP2MigrationAndShutdownIfFirstConnectionIsHTTP1", testHTTP1ToHTTP2MigrationAndShutdownIfFirstConnectionIsHTTP1), diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift index 6e5e7e331..699909c09 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift @@ -193,7 +193,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { // 4. retry connection, but no more queued requests. XCTAssertEqual(state.connectionCreationBackoffDone(newConnectionID), .none) } - + func testConnectionFailureWithoutRetry() { struct SomeError: Error, Equatable {} let elg = EmbeddedEventLoopGroup(loops: 4) From 2cff156b95db93d9bc9bedb1b0c7890a827ad138 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 20 Sep 2022 15:26:18 +0200 Subject: [PATCH 3/6] remove copy & past comments --- .../HTTPConnectionPool+HTTP1StateTests.swift | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift index 9d045e21a..125ba1a74 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift @@ -146,10 +146,6 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase { XCTAssertNoThrow(try queuer.queue(mockRequest, id: request.id)) } - // timeout all queued requests except for two - - // fail all connection attempts - // the first failure should cancel all requests because we have disabled connection establishtment retry let randomConnectionID = connections.randomStartingConnection()! XCTAssertNoThrow(try connections.failConnectionCreation(randomConnectionID)) From 8a1f97d3ab8074dcfce77187885e40095869e6d8 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 20 Sep 2022 15:44:09 +0200 Subject: [PATCH 4/6] print unepxected error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AsyncAwaitEndToEndTests.testImmediateDeadline` failed in CI. Hard to debug without more information. Doesn’t fail locally with 1000 repetitions. --- Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index 1420f187e..92b01dd02 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -409,7 +409,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase { return XCTFail("unexpected error \(error)") } // a race between deadline and connect timer can result in either error - XCTAssertTrue([.deadlineExceeded, .connectTimeout].contains(error)) + XCTAssertTrue([.deadlineExceeded, .connectTimeout].contains(error), "unexpected error \(error)") } } #endif From 67a7c6da0ad001d1a6f203effd3b031d7e04cc45 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Fri, 7 Oct 2022 18:42:02 +0100 Subject: [PATCH 5/6] Fix review comments --- .../HTTPConnectionPool+HTTP1StateMachine.swift | 4 +++- .../HTTPConnectionPool+HTTP2StateMachine.swift | 4 +++- .../State Machine/HTTPConnectionPool+StateMachine.swift | 4 +++- Sources/AsyncHTTPClient/HTTPClient.swift | 7 +++++-- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift index aea773f35..3a6d2534b 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift @@ -30,6 +30,8 @@ extension HTTPConnectionPool { private(set) var requests: RequestQueue private(set) var lifecycleState: StateMachine.LifecycleState + /// The property was introduced to fail fast during testing. + /// Otherwise this should always be true and not turned off. private let retryConnectionEstablishment: Bool init( @@ -225,7 +227,7 @@ extension HTTPConnectionPool { case .running: guard self.retryConnectionEstablishment else { guard let (index, _) = self.connections.failConnection(connectionID) else { - preconditionFailure("Failed to create a connection that is unknown to us?") + preconditionFailure("A connection attempt failed, that the state machine knows nothing about. Somewhere state was lost.") } self.connections.removeConnection(at: index) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift index e7a4e3340..1fef7cf26 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift @@ -34,6 +34,8 @@ extension HTTPConnectionPool { private let idGenerator: Connection.ID.Generator private(set) var lifecycleState: StateMachine.LifecycleState + /// The property was introduced to fail fast during testing. + /// Otherwise this should always be true and not turned off. private let retryConnectionEstablishment: Bool init( @@ -407,7 +409,7 @@ extension HTTPConnectionPool { guard self.retryConnectionEstablishment else { guard let (index, _) = self.connections.failConnection(connectionID) else { - preconditionFailure("Failed to create a connection that is unknown to us?") + preconditionFailure("A connection attempt failed, that the state machine knows nothing about. Somewhere state was lost.") } self.connections.removeConnection(at: index) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift index 4412e265d..0460849cc 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift @@ -96,7 +96,9 @@ extension HTTPConnectionPool { let idGenerator: Connection.ID.Generator let maximumConcurrentHTTP1Connections: Int - let retryConnectionEstablishment: Bool + /// The property was introduced to fail fast during testing. + /// Otherwise this should always be true and not turned off. + private let retryConnectionEstablishment: Bool init( idGenerator: Connection.ID.Generator, diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 1d7f6d5f3..b4d9e8241 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -977,8 +977,11 @@ extension HTTPClient.Configuration { /// If true, ``HTTPClient`` will try to create new connections on connection failure with an exponential backoff. /// Requests will only fail after the ``HTTPClient/Configuration/Timeout-swift.struct/connect`` timeout exceeded. - /// If false, requests will fail immediately after a connection could not be established. - /// Defaults to `true` + /// If false, all requests that have no assigned connection will fail immediately after a connection could not be established. + /// Defaults to `true`. + /// - warning: We highly recommend leaving this on. + /// It is very common that connections establishment is flaky at scale. + /// ``HTTPClient`` will automatically mitigate these kind of issues if this flag is turned on. var retryConnectionEstablishment: Bool public init(idleTimeout: TimeAmount = .seconds(60)) { From 8046ab3fa40590e5d1cfa68dd8e9e06cb96ccfe8 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Mon, 10 Oct 2022 11:50:04 +0100 Subject: [PATCH 6/6] Address review comments --- .../HTTPConnectionPool+HTTP1StateMachine.swift | 7 ------- .../HTTPConnectionPool+HTTP2StateMachine.swift | 13 +------------ 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift index 3a6d2534b..669e43f13 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift @@ -261,13 +261,6 @@ extension HTTPConnectionPool { mutating func waitingForConnectivity(_ error: Error, connectionID: Connection.ID) -> Action { self.lastConnectFailure = error - guard self.retryConnectionEstablishment else { - return .init( - request: self.failAllRequests(reason: error), - connection: .none - ) - } - return .init(request: .none, connection: .none) } diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift index 1fef7cf26..003de4223 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift @@ -404,6 +404,7 @@ extension HTTPConnectionPool { } mutating func failedToCreateNewConnection(_ error: Error, connectionID: Connection.ID) -> Action { + // TODO: switch over state https://github.com/swift-server/async-http-client/issues/638 self.failedConsecutiveConnectionAttempts += 1 self.lastConnectFailure = error @@ -427,18 +428,6 @@ extension HTTPConnectionPool { mutating func waitingForConnectivity(_ error: Error, connectionID: Connection.ID) -> Action { self.lastConnectFailure = error - guard self.retryConnectionEstablishment else { - guard let (index, _) = self.connections.failConnection(connectionID) else { - preconditionFailure("Failed to create a connection that is unknown to us?") - } - _ = self.connections.closeConnection(at: index) - - return .init( - request: self.failAllRequests(reason: error), - connection: .none - ) - } - return .init(request: .none, connection: .none) }