Skip to content

[HTTP2Connections] Return if connection was idle before lease #451

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 2 commits into from
Oct 2, 2021
Merged

[HTTP2Connections] Return if connection was idle before lease #451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5015bd3
return if connection was idle before lease
dnadoba Oct 1, 2021
b54f9fc
Fix compilation for Swift 5.2 and 5.3
dnadoba Oct 2, 2021
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
16 changes: 12 additions & 4 deletions ...es/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ extension HTTPConnectionPool {

// MARK: Leasing and releasing

mutating func leaseStream(onPreferred eventLoop: EventLoop) -> Connection? {
mutating func leaseStream(onPreferred eventLoop: EventLoop) -> (Connection, LeasedStreamContext)? {
guard let index = self.findAvailableConnection(onPreferred: eventLoop) else { return nil }
return self.leaseStreams(at: index, count: 1)
}
Expand All @@ -443,7 +443,7 @@ extension HTTPConnectionPool {
return availableConnectionIndex
}

mutating func leaseStream(onRequired eventLoop: EventLoop) -> Connection? {
mutating func leaseStream(onRequired eventLoop: EventLoop) -> (Connection, LeasedStreamContext)? {
guard let index = self.findAvailableConnection(onRequired: eventLoop) else { return nil }
return self.leaseStreams(at: index, count: 1)
}
Expand All @@ -459,8 +459,11 @@ extension HTTPConnectionPool {
/// - count: number of streams you want to lease. You get the current available streams from the `AvailableConnectionContext` which `newHTTP2ConnectionEstablished(_:maxConcurrentStreams:)` returns
/// - Returns: connection to execute `count` requests on
/// - precondition: `index` needs to be valid. `count` must be greater than or equal to *0* and not execeed the number of available streams.
mutating func leaseStreams(at index: Int, count: Int) -> Connection {
self.connections[index].lease(count)
mutating func leaseStreams(at index: Int, count: Int) -> (Connection, LeasedStreamContext) {
let isIdle = self.connections[index].isIdle
let connection = self.connections[index].lease(count)
let context = LeasedStreamContext(wasIdle: isIdle)
return (connection, context)
}

mutating func releaseStream(_ connectionID: Connection.ID) -> (Int, AvailableConnectionContext) {
Expand Down Expand Up @@ -559,6 +562,11 @@ extension HTTPConnectionPool {
var isIdle: Bool
}

struct LeasedStreamContext {
/// true if the connection was idle before leasing the stream
var wasIdle: Bool
}

struct GoAwayContext {
/// The eventLoop the connection is running on.
var eventLoop: EventLoop
Expand Down
81 changes: 62 additions & 19 deletions Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2ConnectionsTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
XCTAssertEqual(conn1CreatedContext.availableStreams, 100)
XCTAssertEqual(conn1CreatedContext.isIdle, true)
XCTAssert(conn1CreatedContext.eventLoop === el1)
XCTAssertEqual(connections.leaseStreams(at: conn1Index, count: 1), conn1)
let (leasedConn1, leasdConnContext1) = connections.leaseStreams(at: conn1Index, count: 1)
XCTAssertEqual(leasedConn1, conn1)
XCTAssertEqual(leasdConnContext1.wasIdle, true)

// eventLoop connection
XCTAssertTrue(connections.hasConnectionThatCanOrWillBeAbleToExecuteRequests)
Expand All @@ -48,7 +50,10 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
XCTAssertEqual(conn1CreatedContext.availableStreams, 100)
XCTAssertTrue(conn1CreatedContext.isIdle)
XCTAssert(conn2CreatedContext.eventLoop === el2)
XCTAssertEqual(connections.leaseStreams(at: conn2Index, count: 1), conn2)

let (leasedConn2, leasdConnContext2) = connections.leaseStreams(at: conn2Index, count: 1)
XCTAssertEqual(leasedConn2, conn2)
XCTAssertEqual(leasdConnContext2.wasIdle, true)
XCTAssertTrue(connections.hasConnectionThatCanOrWillBeAbleToExecuteRequests(for: el2))
}

Expand Down Expand Up @@ -184,7 +189,10 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
let conn2ID = connections.createNewConnection(on: el1)
let conn2: HTTPConnectionPool.Connection = .__testOnly_connection(id: conn2ID, eventLoop: el1)
let (conn2Index, _) = connections.newHTTP2ConnectionEstablished(conn2, maxConcurrentStreams: 100)
XCTAssertEqual(connections.leaseStreams(at: conn2Index, count: 1), conn2)

let (leasedConn1, leasdConnContext1) = connections.leaseStreams(at: conn2Index, count: 1)
XCTAssertEqual(leasedConn1, conn2)
XCTAssertEqual(leasdConnContext1.wasIdle, true)
XCTAssertNil(connections.closeConnectionIfIdle(conn2ID))
}

Expand All @@ -201,8 +209,11 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
_ = connections.newHTTP2ConnectionEstablished(conn1, maxConcurrentStreams: 100)

// connection is leased
let lease = connections.leaseStream(onPreferred: el1)
XCTAssertEqual(lease, conn1)
guard let (leasedConn, leaseContext) = connections.leaseStream(onPreferred: el1) else {
return XCTFail("lease unexpectedly failed")
}
XCTAssertEqual(leasedConn, conn1)
XCTAssertEqual(leaseContext.wasIdle, true)

// timeout arrives minimal to late
XCTAssertEqual(connections.closeConnectionIfIdle(conn1ID), nil)
Expand Down Expand Up @@ -238,7 +249,11 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
_ = connections.newHTTP2ConnectionEstablished(conn1, maxConcurrentStreams: 100)

// we lease it just before timeout
XCTAssertEqual(connections.leaseStream(onRequired: el1), conn1)
guard let (leasedConn, leaseContext) = connections.leaseStream(onRequired: el1) else {
return XCTFail("lease unexpectedly failed")
}
XCTAssertEqual(leasedConn, conn1)
XCTAssertEqual(leaseContext.wasIdle, true)

// timeout arrives minimal to late
XCTAssertEqual(connections.closeConnectionIfIdle(conn1ID), nil)
Expand Down Expand Up @@ -273,10 +288,11 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
XCTAssertEqual(connections.stats.idleConnections, 4)

// connection is leased
guard let lease = connections.leaseStream(onPreferred: el1) else {
guard let (leasedConn, leaseContext) = connections.leaseStream(onPreferred: el1) else {
return XCTFail("Expected to be able to lease a connection")
}
XCTAssertEqual(lease, .__testOnly_connection(id: 0, eventLoop: el1))
XCTAssertEqual(leasedConn, .__testOnly_connection(id: 0, eventLoop: el1))
XCTAssertEqual(leaseContext.wasIdle, true)

XCTAssertEqual(connections.stats.backingOffConnections, 0)
XCTAssertEqual(connections.stats.leasedStreams, 1)
Expand All @@ -292,7 +308,7 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {

let context = connections.shutdown()
XCTAssertEqual(context.close.count, 3)
XCTAssertEqual(context.cancel, [lease])
XCTAssertEqual(context.cancel, [leasedConn])
XCTAssertEqual(context.connectBackoff, [backingOffID])

XCTAssertEqual(connections.stats.idleConnections, 0)
Expand All @@ -302,8 +318,8 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
XCTAssertEqual(connections.stats.startingConnections, 1)
XCTAssertFalse(connections.isEmpty)

let (releaseIndex, _) = connections.releaseStream(lease.id)
XCTAssertEqual(connections.closeConnection(at: releaseIndex), lease)
let (releaseIndex, _) = connections.releaseStream(leasedConn.id)
XCTAssertEqual(connections.closeConnection(at: releaseIndex), leasedConn)
XCTAssertFalse(connections.isEmpty)

guard let (failIndex, _) = connections.failConnection(startingID) else {
Expand All @@ -322,15 +338,22 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
let conn1: HTTPConnectionPool.Connection = .__testOnly_connection(id: conn1ID, eventLoop: el1)
let (conn1Index, conn1CreatedContext) = connections.newHTTP2ConnectionEstablished(conn1, maxConcurrentStreams: 100)
XCTAssertEqual(conn1CreatedContext.availableStreams, 100)
XCTAssertEqual(connections.leaseStreams(at: conn1Index, count: 100), conn1)
let (leasedConn1, leasdConnContext1) = connections.leaseStreams(at: conn1Index, count: 100)
XCTAssertEqual(leasedConn1, conn1)
XCTAssertEqual(leasdConnContext1.wasIdle, true)

XCTAssertNil(connections.leaseStream(onRequired: el1), "should not be able to lease stream because they are all already leased")

let (_, releaseContext) = connections.releaseStream(conn1ID)
XCTAssertFalse(releaseContext.isIdle)
XCTAssertEqual(releaseContext.availableStreams, 1)

XCTAssertEqual(connections.leaseStream(onRequired: el1), conn1)
guard let (leasedConn, leaseContext) = connections.leaseStream(onRequired: el1) else {
return XCTFail("lease unexpectedly failed")
}
XCTAssertEqual(leasedConn, conn1)
XCTAssertEqual(leaseContext.wasIdle, false)

XCTAssertNil(connections.leaseStream(onRequired: el1), "should not be able to lease stream because they are all already leased")
}

Expand All @@ -343,7 +366,10 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
let conn1: HTTPConnectionPool.Connection = .__testOnly_connection(id: conn1ID, eventLoop: el1)
let (conn1Index, conn1CreatedContext) = connections.newHTTP2ConnectionEstablished(conn1, maxConcurrentStreams: 10)
XCTAssertEqual(conn1CreatedContext.availableStreams, 10)
XCTAssertEqual(connections.leaseStreams(at: conn1Index, count: 2), conn1)

let (leasedConn1, leasdConnContext1) = connections.leaseStreams(at: conn1Index, count: 2)
XCTAssertEqual(leasedConn1, conn1)
XCTAssertEqual(leasdConnContext1.wasIdle, true)

XCTAssertTrue(connections.goAwayReceived(conn1ID).eventLoop === el1)

Expand Down Expand Up @@ -421,7 +447,10 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
let conn1: HTTPConnectionPool.Connection = .__testOnly_connection(id: conn1ID, eventLoop: el1)
let (conn1Index, conn1CreatedContext) = connections.newHTTP2ConnectionEstablished(conn1, maxConcurrentStreams: 1)
XCTAssertEqual(conn1CreatedContext.availableStreams, 1)
XCTAssertEqual(connections.leaseStreams(at: conn1Index, count: 1), conn1)

let (leasedConn1, leasdConnContext1) = connections.leaseStreams(at: conn1Index, count: 1)
XCTAssertEqual(leasedConn1, conn1)
XCTAssertEqual(leasdConnContext1.wasIdle, true)

XCTAssertNil(connections.leaseStream(onRequired: el1), "all streams are in use")

Expand All @@ -430,7 +459,11 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
XCTAssertTrue(newSettingsContext1.eventLoop === el1)
XCTAssertFalse(newSettingsContext1.isIdle)

XCTAssertEqual(connections.leaseStream(onRequired: el1), conn1)
guard let (leasedConn2, leaseContext2) = connections.leaseStream(onRequired: el1) else {
return XCTFail("lease unexpectedly failed")
}
XCTAssertEqual(leasedConn2, conn1)
XCTAssertEqual(leaseContext2.wasIdle, false)

let (_, newSettingsContext2) = connections.newHTTP2MaxConcurrentStreamsReceived(conn1ID, newMaxStreams: 1)
XCTAssertEqual(newSettingsContext2.availableStreams, 0)
Expand All @@ -449,7 +482,11 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
XCTAssertTrue(release2Context.isIdle)
XCTAssertEqual(release2Context.availableStreams, 1)

XCTAssertEqual(connections.leaseStream(onRequired: el1), conn1)
guard let (leasedConn3, leaseContext3) = connections.leaseStream(onRequired: el1) else {
return XCTFail("lease unexpectedly failed")
}
XCTAssertEqual(leasedConn3, conn1)
XCTAssertEqual(leaseContext3.wasIdle, true)
}

func testLeaseOnPreferredEventLoopWithoutAnyAvailable() {
Expand All @@ -461,7 +498,9 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
let conn1: HTTPConnectionPool.Connection = .__testOnly_connection(id: conn1ID, eventLoop: el1)
let (conn1Index, conn1CreatedContext) = connections.newHTTP2ConnectionEstablished(conn1, maxConcurrentStreams: 1)
XCTAssertEqual(conn1CreatedContext.availableStreams, 1)
XCTAssertEqual(connections.leaseStreams(at: conn1Index, count: 1), conn1)
let (leasedConn1, leasdConnContext1) = connections.leaseStreams(at: conn1Index, count: 1)
XCTAssertEqual(leasedConn1, conn1)
XCTAssertEqual(leasdConnContext1.wasIdle, true)

XCTAssertNil(connections.leaseStream(onPreferred: el1), "all streams are in use")
}
Expand Down Expand Up @@ -494,7 +533,11 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
let conn1: HTTPConnectionPool.Connection = .__testOnly_connection(id: conn1ID, eventLoop: el1)
let (conn1Index, conn1CreatedContext) = connections.newHTTP2ConnectionEstablished(conn1, maxConcurrentStreams: 100)
XCTAssertEqual(conn1CreatedContext.availableStreams, 100)
XCTAssertEqual(connections.leaseStreams(at: conn1Index, count: 2), conn1)

let (leasedConn1, leasdConnContext1) = connections.leaseStreams(at: conn1Index, count: 2)
XCTAssertEqual(leasedConn1, conn1)
XCTAssertEqual(leasdConnContext1.wasIdle, true)

XCTAssertEqual(
connections.stats,
.init(
Expand Down