Skip to content

Assume http2 connection by default, instead of http1 #758

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Aug 15, 2024
Merged

Assume http2 connection by default, instead of http1 #758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
950ea95
Assume http2 connection by default, instead of http1
Aug 7, 2024
53a481d
Fix/Add Tests
Aug 7, 2024
ab4ba40
Make param non optional
Aug 7, 2024
d733021
Fix formatiing
Aug 7, 2024
b96e7ba
Fix formatiing
Aug 7, 2024
8dbcd0f
Fix CI failures
Aug 7, 2024
ea7a7a5
Merge branch 'main' into http2
ayush1794 Aug 7, 2024
151692f
Merge branch 'main' into http2
dnadoba Aug 7, 2024
55ff47c
Merge branch 'main' into http2
fabianfett Aug 14, 2024
8397ec4
Prefer general purpose connections while migration h2->h1
Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Package.swift
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you remove el2 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because we now directly start with h2 rather than h1 and then migrating to h1.
h2 has checks to not allow creating connections for same EL. The second el2 just fails the test because the check fails.

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