From efc86bf903d331334763b7c0d1b983156f5f45c9 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 2 Nov 2021 16:54:40 +0000 Subject: [PATCH 1/4] add new single iteration method to check if we need to start a new connection --- .../HTTPConnectionPool+HTTP2Connections.swift | 39 ++++++++++++------- ...HTTPConnectionPool+HTTP2StateMachine.swift | 7 +++- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift index 560d5ff0f..08de45f15 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift @@ -48,6 +48,15 @@ extension HTTPConnectionPool { return false } } + + var isStartingOrActive: Bool { + switch self.state { + case .starting, .active: + return true + case .draining, .backingOff, .closed: + return false + } + } /// A connection is established and can potentially execute requests if not all streams are leased var isActive: Bool { @@ -439,24 +448,28 @@ extension HTTPConnectionPool { } } - /// used after backoff is done to determine if we need to create a new connection - func hasStartingOrActiveConnection() -> Bool { - self.connections.contains { connection in - connection.canOrWillBeAbleToExecuteRequests - } - } - /// used after backoff is done to determine if we need to create a new connection /// - Parameters: /// - eventLoop: connection `EventLoop` to search for - /// - Returns: true if at least one connection is starting or active for the given `eventLoop` - func hasStartingOrActiveConnection( + /// - Returns: + /// `onAnyEventLoop`: true if at least one connection is starting or active regardless of the event loop. + /// `onSpecifiedEventLoop`: true if at least one connection is starting or active for the given `eventLoop`. + func hasAnyStartingOrActiveConnectionAnd( for eventLoop: EventLoop - ) -> Bool { - self.connections.contains { connection in - connection.eventLoop === eventLoop && - connection.canOrWillBeAbleToExecuteRequests + ) -> (onAnyEventLoop: Bool, onSpecifiedEventLoop: Bool) { + var onAnyEventLoop: Bool = false + var onSpecifiedEventLoop: Bool = false + for connection in connections { + guard connection.isStartingOrActive else { continue } + onAnyEventLoop = true + guard connection.eventLoop === eventLoop else { continue } + onSpecifiedEventLoop = true + break } + return ( + onAnyEventLoop, + onSpecifiedEventLoop + ) } mutating func createNewConnection(on eventLoop: EventLoop) -> Connection.ID { diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift index 94ae3edda..5575d95f8 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift @@ -320,19 +320,22 @@ extension HTTPConnectionPool { // if we need to create a new connection because we will only ever create one connection // per event loop for required event loop requests and only need one connection for // general purpose requests. + + // precompute if we have starting or active connections to only iterate once over `self.connections` + let startingOrActiveConnection = self.connections.hasAnyStartingOrActiveConnectionAnd(for: eventLoop) // we need to start a new on connection in two cases: let needGeneralPurposeConnection = // 1. if we have general purpose requests !self.requests.isEmpty(for: nil) && // and no connection starting or active - !self.connections.hasStartingOrActiveConnection() + !startingOrActiveConnection.onAnyEventLoop let needRequiredEventLoopConnection = // 2. or if we have requests for a required event loop !self.requests.isEmpty(for: eventLoop) && // and no connection starting or active for the given event loop - !self.connections.hasStartingOrActiveConnection(for: eventLoop) + !startingOrActiveConnection.onSpecifiedEventLoop guard needGeneralPurposeConnection || needRequiredEventLoopConnection else { // otherwise we can remove the connection From d606ddab9cfac0068bf128f4efa32aa4a8c74b98 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 3 Nov 2021 12:11:14 +0000 Subject: [PATCH 2/4] add test for bugfix --- .../HTTPConnectionPool+HTTP2Connections.swift | 4 +- ...HTTPConnectionPool+HTTP2StateMachine.swift | 2 +- ...onPool+HTTP2StateMachineTests+XCTest.swift | 1 + ...onnectionPool+HTTP2StateMachineTests.swift | 72 +++++++++++++++++++ 4 files changed, 76 insertions(+), 3 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift index 08de45f15..526c77d99 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift @@ -48,7 +48,7 @@ extension HTTPConnectionPool { return false } } - + var isStartingOrActive: Bool { switch self.state { case .starting, .active: @@ -459,7 +459,7 @@ extension HTTPConnectionPool { ) -> (onAnyEventLoop: Bool, onSpecifiedEventLoop: Bool) { var onAnyEventLoop: Bool = false var onSpecifiedEventLoop: Bool = false - for connection in connections { + for connection in self.connections { guard connection.isStartingOrActive else { continue } onAnyEventLoop = true guard connection.eventLoop === eventLoop else { continue } diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift index 5575d95f8..24ac94ec8 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift @@ -320,7 +320,7 @@ extension HTTPConnectionPool { // if we need to create a new connection because we will only ever create one connection // per event loop for required event loop requests and only need one connection for // general purpose requests. - + // precompute if we have starting or active connections to only iterate once over `self.connections` let startingOrActiveConnection = self.connections.hasAnyStartingOrActiveConnectionAnd(for: eventLoop) diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests+XCTest.swift index dbb489503..2c335779e 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests+XCTest.swift @@ -39,6 +39,7 @@ extension HTTPConnectionPool_HTTP2StateMachineTests { ("testMigrationFromHTTP1ToHTTP2", testMigrationFromHTTP1ToHTTP2), ("testMigrationFromHTTP1ToHTTP2WithAlreadyStartedHTTP1Connections", testMigrationFromHTTP1ToHTTP2WithAlreadyStartedHTTP1Connections), ("testHTTP2toHTTP1Migration", testHTTP2toHTTP1Migration), + ("testConnectionIsImmediatelyCreatedAfterBackoffTimerFires", testConnectionIsImmediatelyCreatedAfterBackoffTimerFires), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift index 767f2f032..b45a40326 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift @@ -869,4 +869,76 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { XCTAssertEqual(releaseAction.request, .none) XCTAssertNoThrow(try connections.closeConnection(http2Conn)) } + + func testConnectionIsImmediatelyCreatedAfterBackoffTimerFires() { + let elg = EmbeddedEventLoopGroup(loops: 2) + let el1 = elg.next() + let el2 = elg.next() + var connections = MockConnectionPool() + var queuer = MockRequestQueuer() + var state = HTTPConnectionPool.StateMachine(idGenerator: .init(), maximumConcurrentHTTP1Connections: 8) + + var connectionIDs: [HTTPConnectionPool.Connection.ID] = [] + for el in [el1, el2, el2] { + let mockRequest = MockHTTPRequest(eventLoop: el, requiresEventLoopForChannel: true) + let request = HTTPConnectionPool.Request(mockRequest) + let action = state.executeRequest(request) + guard case .createConnection(let connID, let eventLoop) = action.connection else { + return XCTFail("Unexpected connection action \(action.connection)") + } + connectionIDs.append(connID) + XCTAssertTrue(eventLoop === el) + XCTAssertEqual(action.request, .scheduleRequestTimeout(for: request, on: mockRequest.eventLoop)) + XCTAssertNoThrow(try connections.createConnection(connID, on: el)) + XCTAssertNoThrow(try queuer.queue(mockRequest, id: request.id)) + } + + for connectionID in connectionIDs.dropFirst() { + struct SomeError: Error {} + XCTAssertNoThrow(try connections.failConnectionCreation(connectionID)) + let action = state.failedToCreateNewConnection(SomeError(), connectionID: connectionID) + XCTAssertEqual(action.request, .none) + guard case .scheduleBackoffTimer(connectionID, backoff: _, on: _) = action.connection else { + return XCTFail("unexpected connection action \(connectionID)") + } + XCTAssertNoThrow(try connections.startConnectionBackoffTimer(connectionID)) + } + let http2ConnID1 = connectionIDs[0] + let http2ConnID2 = connectionIDs[1] + let http2ConnID3 = connectionIDs[2] + + let http2Conn1: HTTPConnectionPool.Connection = .__testOnly_connection(id: http2ConnID1, eventLoop: el1) + XCTAssertNoThrow(try connections.succeedConnectionCreationHTTP2(http2ConnID1, maxConcurrentStreams: 10)) + let migrationAction1 = state.newHTTP2ConnectionCreated(http2Conn1, maxConcurrentStreams: 10) + guard case .executeRequestsAndCancelTimeouts(let requests, http2Conn1) = migrationAction1.request else { + return XCTFail("unexpected request action \(migrationAction1.request)") + } + XCTAssertEqual(migrationAction1.connection, .migration(createConnections: [], closeConnections: [], scheduleTimeout: nil)) + XCTAssertEqual(requests.count, 1) + for request in requests { + XCTAssertNoThrow(try queuer.get(request.id, request: request.__testOnly_wrapped_request())) + XCTAssertNoThrow(try connections.execute(request.__testOnly_wrapped_request(), on: http2Conn1)) + } + + // we now have 1 active connection on el1 and 2 backing off connections on el2 + // with 2 queued requests with a requirement to be executed on el2 + + // if the backoff time fires for a connection on el2, we should immediately start a new connection + XCTAssertNoThrow(try connections.connectionBackoffTimerDone(http2ConnID2)) + let action2 = state.connectionCreationBackoffDone(http2ConnID2) + XCTAssertEqual(action2.request, .none) + guard case .createConnection(let newHttp2ConnID2, let eventLoop2) = action2.connection else { + return XCTFail("Unexpected connection action \(action2.connection)") + } + XCTAssertTrue(eventLoop2 === el2) + XCTAssertNoThrow(try connections.createConnection(newHttp2ConnID2, on: el2)) + + // we now have a starting connection for el2 and another one backing off + + // if the backoff timer fires now for a connection on el2, we should *not* start a new connection + XCTAssertNoThrow(try connections.connectionBackoffTimerDone(http2ConnID3)) + let action3 = state.connectionCreationBackoffDone(http2ConnID3) + XCTAssertEqual(action3.request, .none) + XCTAssertEqual(action3.connection, .none) + } } From 0c7a822b6018403ecf3d0226bcd331f0d31cb35d Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 3 Nov 2021 13:59:55 +0000 Subject: [PATCH 3/4] add more documentaiton to new test case --- .../HTTPConnectionPool+HTTP2StateMachineTests.swift | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift index b45a40326..eb236813e 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift @@ -892,7 +892,8 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { XCTAssertNoThrow(try connections.createConnection(connID, on: el)) XCTAssertNoThrow(try queuer.queue(mockRequest, id: request.id)) } - + + // fail the two connections for el2 for connectionID in connectionIDs.dropFirst() { struct SomeError: Error {} XCTAssertNoThrow(try connections.failConnectionCreation(connectionID)) @@ -907,6 +908,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { let http2ConnID2 = connectionIDs[1] let http2ConnID3 = connectionIDs[2] + // let the first connection on el1 succeed as a http2 connection let http2Conn1: HTTPConnectionPool.Connection = .__testOnly_connection(id: http2ConnID1, eventLoop: el1) XCTAssertNoThrow(try connections.succeedConnectionCreationHTTP2(http2ConnID1, maxConcurrentStreams: 10)) let migrationAction1 = state.newHTTP2ConnectionCreated(http2Conn1, maxConcurrentStreams: 10) @@ -923,7 +925,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { // we now have 1 active connection on el1 and 2 backing off connections on el2 // with 2 queued requests with a requirement to be executed on el2 - // if the backoff time fires for a connection on el2, we should immediately start a new connection + // if the backoff timer fires for a connection on el2, we should immediately start a new connection XCTAssertNoThrow(try connections.connectionBackoffTimerDone(http2ConnID2)) let action2 = state.connectionCreationBackoffDone(http2ConnID2) XCTAssertEqual(action2.request, .none) From 51ae32715d27e89cef7c08445b03eba328ed9b37 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Wed, 3 Nov 2021 14:18:56 +0000 Subject: [PATCH 4/4] rename method as Fabian suggested --- .../HTTPConnectionPool+HTTP2Connections.swift | 30 +++++++++++-------- ...HTTPConnectionPool+HTTP2StateMachine.swift | 6 ++-- ...onnectionPool+HTTP2StateMachineTests.swift | 2 +- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift index 526c77d99..e2ef07065 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift @@ -451,24 +451,22 @@ extension HTTPConnectionPool { /// used after backoff is done to determine if we need to create a new connection /// - Parameters: /// - eventLoop: connection `EventLoop` to search for - /// - Returns: - /// `onAnyEventLoop`: true if at least one connection is starting or active regardless of the event loop. - /// `onSpecifiedEventLoop`: true if at least one connection is starting or active for the given `eventLoop`. - func hasAnyStartingOrActiveConnectionAnd( + /// - Returns: if we have a starting or active general purpose connection and if we have also one for the given `eventLoop` + func backingOffTimerDone( for eventLoop: EventLoop - ) -> (onAnyEventLoop: Bool, onSpecifiedEventLoop: Bool) { - var onAnyEventLoop: Bool = false - var onSpecifiedEventLoop: Bool = false + ) -> RetryConnectionCreationContext { + var hasGeneralPurposeConnection: Bool = false + var hasConnectionOnSpecifiedEventLoop: Bool = false for connection in self.connections { guard connection.isStartingOrActive else { continue } - onAnyEventLoop = true + hasGeneralPurposeConnection = true guard connection.eventLoop === eventLoop else { continue } - onSpecifiedEventLoop = true + hasConnectionOnSpecifiedEventLoop = true break } - return ( - onAnyEventLoop, - onSpecifiedEventLoop + return RetryConnectionCreationContext( + hasGeneralPurposeConnection: hasGeneralPurposeConnection, + hasConnectionOnSpecifiedEventLoop: hasConnectionOnSpecifiedEventLoop ) } @@ -688,6 +686,14 @@ extension HTTPConnectionPool { // MARK: Result structs + struct RetryConnectionCreationContext { + /// true if at least one connection is starting or active regardless of the event loop. + let hasGeneralPurposeConnection: Bool + + /// true if at least one connection is starting or active for the given `eventLoop` + let hasConnectionOnSpecifiedEventLoop: Bool + } + /// Information around a connection which is either in the .active or .draining state. struct EstablishedConnectionContext { /// number of streams which can be leased diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift index 24ac94ec8..7a9f4dc7c 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift @@ -322,20 +322,20 @@ extension HTTPConnectionPool { // general purpose requests. // precompute if we have starting or active connections to only iterate once over `self.connections` - let startingOrActiveConnection = self.connections.hasAnyStartingOrActiveConnectionAnd(for: eventLoop) + let context = self.connections.backingOffTimerDone(for: eventLoop) // we need to start a new on connection in two cases: let needGeneralPurposeConnection = // 1. if we have general purpose requests !self.requests.isEmpty(for: nil) && // and no connection starting or active - !startingOrActiveConnection.onAnyEventLoop + !context.hasGeneralPurposeConnection let needRequiredEventLoopConnection = // 2. or if we have requests for a required event loop !self.requests.isEmpty(for: eventLoop) && // and no connection starting or active for the given event loop - !startingOrActiveConnection.onSpecifiedEventLoop + !context.hasConnectionOnSpecifiedEventLoop guard needGeneralPurposeConnection || needRequiredEventLoopConnection else { // otherwise we can remove the connection diff --git a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift index eb236813e..e8a4edbbe 100644 --- a/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift @@ -892,7 +892,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { XCTAssertNoThrow(try connections.createConnection(connID, on: el)) XCTAssertNoThrow(try queuer.queue(mockRequest, id: request.id)) } - + // fail the two connections for el2 for connectionID in connectionIDs.dropFirst() { struct SomeError: Error {}