Skip to content

Commit

Permalink
Assume http2 connection by default, instead of http1 (#758)
Browse files Browse the repository at this point in the history 
Since most of the servers now conform to http2, the change here updates
the behaviour of assuming the connection to be http2 and not http1 by
default. It will migrate to http1 if the server only supports http1.
One can set the `httpVersion` in `ClientConfiguration` to `.http1Only`
which will start with http1 instead of http2.

Additional Changes:

- Fixed an off by one error in the maximum additional general purpose
connection check
- Updated tests

---------

Co-authored-by: Ayush Garg <[email protected]>
Co-authored-by: David Nadoba <[email protected]>
Co-authored-by: Fabian Fett <[email protected]>
  • Loading branch information
@ayush1794 @dnadoba @fabianfett
4 people authored Aug 15, 2024
1 parent 4b7a68e commit 1290119
Show file tree
Hide file tree
Showing 9 changed files with 149 additions and 40 deletions.
2 changes: 1 addition & 1 deletion Package.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ let package = Package(
],
dependencies: [
.package(url: "https://github.com/apple/swift-nio.git", from: "2.62.0"),
.package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.22.0"),
.package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.27.1"),
.package(url: "https://github.com/apple/swift-nio-http2.git", from: "1.19.0"),
.package(url: "https://github.com/apple/swift-nio-extras.git", from: "1.13.0"),
.package(url: "https://github.com/apple/swift-nio-transport-services.git", from: "1.19.0"),
Expand Down
1 change: 1 addition & 0 deletions Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ final class HTTPConnectionPool {
idGenerator: idGenerator,
maximumConcurrentHTTP1Connections: clientConfiguration.connectionPool.concurrentHTTP1ConnectionsPerHostSoftLimit,
retryConnectionEstablishment: clientConfiguration.connectionPool.retryConnectionEstablishment,
preferHTTP1: clientConfiguration.httpVersion == .http1Only,
maximumConnectionUses: clientConfiguration.maximumUsesPerConnection
)
}
Expand Down
6 changes: 4 additions & 2 deletions ...es/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ extension HTTPConnectionPool {
}

private var maximumAdditionalGeneralPurposeConnections: Int {
self.maximumConcurrentConnections - (self.overflowIndex - 1)
self.maximumConcurrentConnections - (self.overflowIndex)
}

/// Is there at least one connection that is able to run requests
Expand Down Expand Up @@ -594,6 +594,7 @@ extension HTTPConnectionPool {
eventLoop: eventLoop,
maximumUses: self.maximumConnectionUses
)

self.connections.insert(newConnection, at: self.overflowIndex)
/// If we can grow, we mark the connection as a general purpose connection.
/// Otherwise, it will be an overflow connection which is only used once for requests with a required event loop
Expand All @@ -610,6 +611,7 @@ extension HTTPConnectionPool {
)
// TODO: Maybe we want to add a static init for backing off connections to HTTP1ConnectionState
backingOffConnection.failedToConnect()

self.connections.insert(backingOffConnection, at: self.overflowIndex)
/// If we can grow, we mark the connection as a general purpose connection.
/// Otherwise, it will be an overflow connection which is only used once for requests with a required event loop
Expand Down Expand Up @@ -637,7 +639,7 @@ extension HTTPConnectionPool {
) -> [(Connection.ID, EventLoop)] {
// create new connections for requests with a required event loop

// we may already start connections for those requests and do not want to start to many
// we may already start connections for those requests and do not want to start too many
let startingRequiredEventLoopConnectionCount = Dictionary(
self.connections[self.overflowIndex..<self.connections.endIndex].lazy.map {
($0.eventLoop.id, 1)
Expand Down
28 changes: 20 additions & 8 deletions Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,20 +105,32 @@ extension HTTPConnectionPool {
idGenerator: Connection.ID.Generator,
maximumConcurrentHTTP1Connections: Int,
retryConnectionEstablishment: Bool,
preferHTTP1: Bool,
maximumConnectionUses: Int?
) {
self.maximumConcurrentHTTP1Connections = maximumConcurrentHTTP1Connections
self.retryConnectionEstablishment = retryConnectionEstablishment
self.idGenerator = idGenerator
self.maximumConnectionUses = maximumConnectionUses
let http1State = HTTP1StateMachine(
idGenerator: idGenerator,
maximumConcurrentConnections: maximumConcurrentHTTP1Connections,
retryConnectionEstablishment: retryConnectionEstablishment,
maximumConnectionUses: maximumConnectionUses,
lifecycleState: .running
)
self.state = .http1(http1State)

if preferHTTP1 {
let http1State = HTTP1StateMachine(
idGenerator: idGenerator,
maximumConcurrentConnections: maximumConcurrentHTTP1Connections,
retryConnectionEstablishment: retryConnectionEstablishment,
maximumConnectionUses: maximumConnectionUses,
lifecycleState: .running
)
self.state = .http1(http1State)
} else {
let http2State = HTTP2StateMachine(
idGenerator: idGenerator,
retryConnectionEstablishment: retryConnectionEstablishment,
lifecycleState: .running,
maximumConnectionUses: maximumConnectionUses
)
self.state = .http2(http2State)
}
}

mutating func executeRequest(_ request: Request) -> Action {
Expand Down
28 changes: 28 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1ConnectionsTest.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,34 @@ class HTTPConnectionPool_HTTP1ConnectionsTests: XCTestCase {
XCTAssertTrue(context.eventLoop === el3)
}

func testMigrationFromHTTP2WithPendingRequestsWithRequiredEventLoopSameAsStartingConnections() {
let elg = EmbeddedEventLoopGroup(loops: 4)
let generator = HTTPConnectionPool.Connection.ID.Generator()
var connections = HTTPConnectionPool.HTTP1Connections(maximumConcurrentConnections: 8, generator: generator, maximumConnectionUses: nil)

let el1 = elg.next()
let el2 = elg.next()

let conn1ID = generator.next()
let conn2ID = generator.next()

connections.migrateFromHTTP2(
starting: [(conn1ID, el1)],
backingOff: [(conn2ID, el2)]
)

let stats = connections.stats
XCTAssertEqual(stats.idle, 0)
XCTAssertEqual(stats.leased, 0)
XCTAssertEqual(stats.connecting, 1)
XCTAssertEqual(stats.backingOff, 1)

let conn1: HTTPConnectionPool.Connection = .__testOnly_connection(id: conn1ID, eventLoop: el1)
let (_, context) = connections.newHTTP1ConnectionEstablished(conn1)
XCTAssertEqual(context.use, .generalPurpose)
XCTAssertTrue(context.eventLoop === el1)
}

func testMigrationFromHTTP2WithPendingRequestsWithPreferredEventLoop() {
let elg = EmbeddedEventLoopGroup(loops: 4)
let generator = HTTPConnectionPool.Connection.ID.Generator()
Expand Down
9 changes: 9 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 8,
retryConnectionEstablishment: true,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand Down Expand Up @@ -113,6 +114,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 8,
retryConnectionEstablishment: false,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand Down Expand Up @@ -181,6 +183,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 2,
retryConnectionEstablishment: true,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand Down Expand Up @@ -240,6 +243,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 2,
retryConnectionEstablishment: true,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand Down Expand Up @@ -278,6 +282,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 2,
retryConnectionEstablishment: true,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand Down Expand Up @@ -670,6 +675,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 6,
retryConnectionEstablishment: true,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand Down Expand Up @@ -710,6 +716,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 6,
retryConnectionEstablishment: true,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand Down Expand Up @@ -743,6 +750,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 6,
retryConnectionEstablishment: true,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand All @@ -768,6 +776,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 6,
retryConnectionEstablishment: true,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand Down
44 changes: 20 additions & 24 deletions Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 8,
retryConnectionEstablishment: true,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand Down Expand Up @@ -811,6 +812,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 8,
retryConnectionEstablishment: true,
preferHTTP1: false,
maximumConnectionUses: nil
)

Expand Down Expand Up @@ -858,6 +860,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 8,
retryConnectionEstablishment: true,
preferHTTP1: true,
maximumConnectionUses: nil
)

Expand Down Expand Up @@ -998,6 +1001,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 8,
retryConnectionEstablishment: true,
preferHTTP1: false,
maximumConnectionUses: nil
)

Expand All @@ -1014,11 +1018,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
XCTAssertNoThrow(try queuer.queue(mockRequest, id: request1.id))
let http2Conn: HTTPConnectionPool.Connection = .__testOnly_connection(id: http2ConnID, eventLoop: el1)
XCTAssertNoThrow(try connections.succeedConnectionCreationHTTP2(http2ConnID, maxConcurrentStreams: 10))
let migrationAction1 = state.newHTTP2ConnectionCreated(http2Conn, maxConcurrentStreams: 10)
guard case .executeRequestsAndCancelTimeouts(let requests, http2Conn) = migrationAction1.request else {
return XCTFail("unexpected request action \(migrationAction1.request)")
let executeAction1 = state.newHTTP2ConnectionCreated(http2Conn, maxConcurrentStreams: 10)
guard case .executeRequestsAndCancelTimeouts(let requests, http2Conn) = executeAction1.request else {
return XCTFail("unexpected request action \(executeAction1.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()))
Expand Down Expand Up @@ -1069,6 +1073,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 8,
retryConnectionEstablishment: true,
preferHTTP1: false,
maximumConnectionUses: nil
)

Expand All @@ -1085,11 +1090,11 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
XCTAssertNoThrow(try queuer.queue(mockRequest, id: request1.id))
let http2Conn: HTTPConnectionPool.Connection = .__testOnly_connection(id: http2ConnID, eventLoop: el1)
XCTAssertNoThrow(try connections.succeedConnectionCreationHTTP2(http2ConnID, maxConcurrentStreams: 10))
let migrationAction1 = state.newHTTP2ConnectionCreated(http2Conn, maxConcurrentStreams: 10)
guard case .executeRequestsAndCancelTimeouts(let requests, http2Conn) = migrationAction1.request else {
return XCTFail("unexpected request action \(migrationAction1.request)")
let executeAction1 = state.newHTTP2ConnectionCreated(http2Conn, maxConcurrentStreams: 10)
guard case .executeRequestsAndCancelTimeouts(let requests, http2Conn) = executeAction1.request else {
return XCTFail("unexpected request action \(executeAction1.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()))
Expand Down Expand Up @@ -1120,7 +1125,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
}
XCTAssertTrue(queuer.isEmpty)

// if we established a new http/1 connection we should migrate back to http/1,
// if we established a new http/1 connection we should migrate to http/1,
// close the connection and shutdown the pool
let http1Conn: HTTPConnectionPool.Connection = .__testOnly_connection(id: http1ConnId, eventLoop: el2)
XCTAssertNoThrow(try connections.succeedConnectionCreationHTTP1(http1ConnId))
Expand All @@ -1146,11 +1151,12 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
idGenerator: .init(),
maximumConcurrentHTTP1Connections: 8,
retryConnectionEstablishment: true,
preferHTTP1: false,
maximumConnectionUses: nil
)

var connectionIDs: [HTTPConnectionPool.Connection.ID] = []
for el in [el1, el2, el2] {
for el in [el1, el2] {
let mockRequest = MockHTTPScheduableRequest(eventLoop: el, requiresEventLoopForChannel: true)
let request = HTTPConnectionPool.Request(mockRequest)
let action = state.executeRequest(request)
Expand All @@ -1164,7 +1170,7 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
XCTAssertNoThrow(try queuer.queue(mockRequest, id: request.id))
}

// fail the two connections for el2
// fail the connection for el2
for connectionID in connectionIDs.dropFirst() {
struct SomeError: Error {}
XCTAssertNoThrow(try connections.failConnectionCreation(connectionID))
Expand All @@ -1177,16 +1183,14 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
}
let http2ConnID1 = connectionIDs[0]
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)
guard case .executeRequestsAndCancelTimeouts(let requests, http2Conn1) = migrationAction1.request else {
return XCTFail("unexpected request action \(migrationAction1.request)")
let connectionAction = state.newHTTP2ConnectionCreated(http2Conn1, maxConcurrentStreams: 10)
guard case .executeRequestsAndCancelTimeouts(let requests, http2Conn1) = connectionAction.request else {
return XCTFail("unexpected request action \(connectionAction.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()))
Expand All @@ -1205,14 +1209,6 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
}
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)
}

func testMaxConcurrentStreamsIsRespected() {
Expand Down
Loading

0 comments on commit 1290119

Please sign in to comment.