Skip to content

Commit 3c45dbd

Browse files
fabianfettglbrntt
fabianfett
and
glbrntt
authored
Fix Connection Creation Crash (#873)
### Motivation When creating a connection, we wrongfully assumed that `failedToCreateNewConnection` will always be called before `http*ConnectionClosed` in the `HTTPConnectionPoolStateMachine`. However this is far from correct. In NIO Futures are fulfilled before `ChannelHandler` callbacks. Ordering in futures should not be assumed in such a complex project. ### Change We change the `http*ConnectionClosed` methods to be noops, if the connection is in the starting state. We instead wait for the `failedToCreateNewConnection` to create backoff timers and friends. rdar://164674912 --------- Co-authored-by: George Barnett <gbarnett@apple.com>
1 parent ce04df0 commit 3c45dbd

8 files changed

+165
-38
lines changed

‎Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift‎

Lines changed: 35 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -166,10 +166,22 @@ extension HTTPConnectionPool {
166166
}
167167
}
168168

169-
mutating func fail() {
169+
enum FailAction {
170+
case removeConnection
171+
case none
172+
}
173+
174+
mutating func fail() -> FailAction {
170175
switch self.state {
171-
case .starting, .backingOff, .idle, .leased:
176+
case .starting:
177+
// If the connection fails while we are starting it, the fail call raced with
178+
// `failedToConnect` (promises are succeeded or failed before channel handler
179+
// callbacks). let's keep the state in `starting`, so that `failedToConnect` can
180+
// create a backoff timer.
181+
return .none
182+
case .backingOff, .idle, .leased:
172183
self.state = .closed
184+
return .removeConnection
173185
case .closed:
174186
preconditionFailure("Invalid state: \(self.state)")
175187
}
@@ -559,23 +571,28 @@ extension HTTPConnectionPool {
559571
}
560572

561573
let use: ConnectionUse
562-
self.connections[index].fail()
563-
let eventLoop = self.connections[index].eventLoop
564-
let starting: Int
565-
if index < self.overflowIndex {
566-
use = .generalPurpose
567-
starting = self.startingGeneralPurposeConnections
568-
} else {
569-
use = .eventLoop(eventLoop)
570-
starting = self.startingEventLoopConnections(on: eventLoop)
571-
}
574+
switch self.connections[index].fail() {
575+
case .removeConnection:
576+
let eventLoop = self.connections[index].eventLoop
577+
let starting: Int
578+
if index < self.overflowIndex {
579+
use = .generalPurpose
580+
starting = self.startingGeneralPurposeConnections
581+
} else {
582+
use = .eventLoop(eventLoop)
583+
starting = self.startingEventLoopConnections(on: eventLoop)
584+
}
572585

573-
let context = FailedConnectionContext(
574-
eventLoop: eventLoop,
575-
use: use,
576-
connectionsStartingForUseCase: starting
577-
)
578-
return (index, context)
586+
let context = FailedConnectionContext(
587+
eventLoop: eventLoop,
588+
use: use,
589+
connectionsStartingForUseCase: starting
590+
)
591+
return (index, context)
592+
593+
case .none:
594+
return nil
595+
}
579596
}
580597

581598
// MARK: Migration

‎Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,11 @@ extension HTTPConnectionPool {
250250
self.failedConsecutiveConnectionAttempts += 1
251251
self.lastConnectFailure = error
252252

253+
// We don't care how many waiting requests we have at this point, we will schedule a
254+
// retry. More tasks, may appear until the backoff has completed. The final
255+
// decision about the retry will be made in `connectionCreationBackoffDone(_:)`
256+
let eventLoop = self.connections.backoffNextConnectionAttempt(connectionID)
257+
253258
switch self.lifecycleState {
254259
case .running:
255260
guard self.retryConnectionEstablishment else {
@@ -265,10 +270,6 @@ extension HTTPConnectionPool {
265270
connection: .none
266271
)
267272
}
268-
// We don't care how many waiting requests we have at this point, we will schedule a
269-
// retry. More tasks, may appear until the backoff has completed. The final
270-
// decision about the retry will be made in `connectionCreationBackoffDone(_:)`
271-
let eventLoop = self.connections.backoffNextConnectionAttempt(connectionID)
272273

273274
let backoff = calculateBackoff(failedAttempt: self.failedConsecutiveConnectionAttempts)
274275
return .init(

‎Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -187,10 +187,22 @@ extension HTTPConnectionPool {
187187
}
188188
}
189189

190-
mutating func fail() {
190+
enum FailAction {
191+
case removeConnection
192+
case none
193+
}
194+
195+
mutating func fail() -> FailAction {
191196
switch self.state {
192-
case .starting, .active, .backingOff, .draining:
197+
case .starting:
198+
// If the connection fails while we are starting it, the fail call raced with
199+
// `failedToConnect` (promises are succeeded or failed before channel handler
200+
// callbacks). let's keep the state in `starting`, so that `failedToConnect` can
201+
// create a backoff timer.
202+
return .none
203+
case .active, .backingOff, .draining:
193204
self.state = .closed
205+
return .removeConnection
194206
case .closed:
195207
preconditionFailure("Invalid state: \(self.state)")
196208
}
@@ -749,10 +761,16 @@ extension HTTPConnectionPool {
749761
// must ignore the event.
750762
return nil
751763
}
752-
self.connections[index].fail()
753-
let eventLoop = self.connections[index].eventLoop
754-
let context = FailedConnectionContext(eventLoop: eventLoop)
755-
return (index, context)
764+
765+
switch self.connections[index].fail() {
766+
case .none:
767+
return nil
768+
769+
case .removeConnection:
770+
let eventLoop = self.connections[index].eventLoop
771+
let context = FailedConnectionContext(eventLoop: eventLoop)
772+
return (index, context)
773+
}
756774
}
757775

758776
mutating func shutdown() -> CleanupContext {

‎Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -226,20 +226,18 @@ extension HTTPConnectionPool {
226226
) -> EstablishedAction {
227227
self.failedConsecutiveConnectionAttempts = 0
228228
self.lastConnectFailure = nil
229-
if self.connections.hasActiveConnection(for: connection.eventLoop) {
230-
guard let (index, _) = self.connections.failConnection(connection.id) else {
231-
preconditionFailure("we have established a new connection that we know nothing about?")
232-
}
233-
self.connections.removeConnection(at: index)
229+
let doesConnectionExistsForEL = self.connections.hasActiveConnection(for: connection.eventLoop)
230+
let (index, context) = self.connections.newHTTP2ConnectionEstablished(
231+
connection,
232+
maxConcurrentStreams: maxConcurrentStreams
233+
)
234+
if doesConnectionExistsForEL {
235+
let connection = self.connections.closeConnection(at: index)
234236
return .init(
235237
request: .none,
236238
connection: .closeConnection(connection, isShutdown: .no)
237239
)
238240
} else {
239-
let (index, context) = self.connections.newHTTP2ConnectionEstablished(
240-
connection,
241-
maxConcurrentStreams: maxConcurrentStreams
242-
)
243241
return self.nextActionForAvailableConnection(at: index, context: context)
244242
}
245243
}
@@ -424,6 +422,8 @@ extension HTTPConnectionPool {
424422
self.failedConsecutiveConnectionAttempts += 1
425423
self.lastConnectFailure = error
426424

425+
let eventLoop = self.connections.backoffNextConnectionAttempt(connectionID)
426+
427427
switch self.lifecycleState {
428428
case .running:
429429
guard self.retryConnectionEstablishment else {
@@ -440,7 +440,6 @@ extension HTTPConnectionPool {
440440
)
441441
}
442442

443-
let eventLoop = self.connections.backoffNextConnectionAttempt(connectionID)
444443
let backoff = calculateBackoff(failedAttempt: self.failedConsecutiveConnectionAttempts)
445444
return .init(
446445
request: .none,

‎Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1ConnectionsTest.swift‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,8 @@ class HTTPConnectionPool_HTTP1ConnectionsTests: XCTestCase {
384384
XCTAssertEqual(connections.closeConnection(at: releaseIndex), lease)
385385
XCTAssertFalse(connections.isEmpty)
386386

387+
let backoffEL = connections.backoffNextConnectionAttempt(startingID)
388+
XCTAssertIdentical(backoffEL, el2)
387389
guard let (failIndex, _) = connections.failConnection(startingID) else {
388390
return XCTFail("Expected that the connection is remembered")
389391
}

‎Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1493,4 +1493,48 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
14931493

14941494
// We won't bother doing it though, it's enough that it asked.
14951495
}
1496+
1497+
func testFailConnectionRacesAgainstConnectionCreationFailed() {
1498+
let elg = EmbeddedEventLoopGroup(loops: 4)
1499+
defer { XCTAssertNoThrow(try elg.syncShutdownGracefully()) }
1500+
1501+
var state = HTTPConnectionPool.StateMachine(
1502+
idGenerator: .init(),
1503+
maximumConcurrentHTTP1Connections: 2,
1504+
retryConnectionEstablishment: true,
1505+
preferHTTP1: true,
1506+
maximumConnectionUses: nil,
1507+
preWarmedHTTP1ConnectionCount: 0
1508+
)
1509+
1510+
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
1511+
let request = HTTPConnectionPool.Request(mockRequest)
1512+
1513+
let executeAction = state.executeRequest(request)
1514+
XCTAssertEqual(.scheduleRequestTimeout(for: request, on: mockRequest.eventLoop), executeAction.request)
1515+
1516+
// 1. connection attempt
1517+
guard case .createConnection(let connectionID, on: let connectionEL) = executeAction.connection else {
1518+
return XCTFail("Unexpected connection action: \(executeAction.connection)")
1519+
}
1520+
XCTAssert(connectionEL === mockRequest.eventLoop) // XCTAssertIdentical not available on Linux
1521+
1522+
// 2. connection fails – first with closed callback
1523+
1524+
XCTAssertEqual(state.http1ConnectionClosed(connectionID), .none)
1525+
1526+
// 3. connection fails – with make connection callback
1527+
1528+
let action = state.failedToCreateNewConnection(
1529+
IOError(errnoCode: -1, reason: "Test failure"),
1530+
connectionID: connectionID
1531+
)
1532+
XCTAssertEqual(action.request, .none)
1533+
guard case .scheduleBackoffTimer(connectionID, _, on: let backoffTimerEL) = action.connection else {
1534+
XCTFail("Unexpected connection action: \(action.connection)")
1535+
return
1536+
}
1537+
XCTAssertIdentical(connectionEL, backoffTimerEL)
1538+
1539+
}
14961540
}

‎Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2ConnectionsTest.swift‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,8 @@ class HTTPConnectionPool_HTTP2ConnectionsTests: XCTestCase {
331331
XCTAssertEqual(connections.closeConnection(at: releaseIndex), leasedConn)
332332
XCTAssertFalse(connections.isEmpty)
333333

334+
let backoffEL = connections.backoffNextConnectionAttempt(startingID)
335+
XCTAssertIdentical(el6, backoffEL)
334336
guard let (failIndex, _) = connections.failConnection(startingID) else {
335337
return XCTFail("Expected that the connection is remembered")
336338
}

‎Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests.swift‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1527,6 +1527,50 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase {
15271527

15281528
XCTAssertEqual(state.http2ConnectionClosed(connection.id), .none)
15291529
}
1530+
1531+
func testFailConnectionRacesAgainstConnectionCreationFailed() {
1532+
let elg = EmbeddedEventLoopGroup(loops: 4)
1533+
defer { XCTAssertNoThrow(try elg.syncShutdownGracefully()) }
1534+
1535+
var state = HTTPConnectionPool.StateMachine(
1536+
idGenerator: .init(),
1537+
maximumConcurrentHTTP1Connections: 2,
1538+
retryConnectionEstablishment: true,
1539+
preferHTTP1: false,
1540+
maximumConnectionUses: nil,
1541+
preWarmedHTTP1ConnectionCount: 0
1542+
)
1543+
1544+
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
1545+
let request = HTTPConnectionPool.Request(mockRequest)
1546+
1547+
let executeAction = state.executeRequest(request)
1548+
XCTAssertEqual(.scheduleRequestTimeout(for: request, on: mockRequest.eventLoop), executeAction.request)
1549+
1550+
// 1. connection attempt
1551+
guard case .createConnection(let connectionID, on: let connectionEL) = executeAction.connection else {
1552+
return XCTFail("Unexpected connection action: \(executeAction.connection)")
1553+
}
1554+
XCTAssert(connectionEL === mockRequest.eventLoop) // XCTAssertIdentical not available on Linux
1555+
1556+
// 2. connection fails – first with closed callback
1557+
1558+
XCTAssertEqual(state.http2ConnectionClosed(connectionID), .none)
1559+
1560+
// 3. connection fails – with make connection callback
1561+
1562+
let action = state.failedToCreateNewConnection(
1563+
IOError(errnoCode: -1, reason: "Test failure"),
1564+
connectionID: connectionID
1565+
)
1566+
XCTAssertEqual(action.request, .none)
1567+
guard case .scheduleBackoffTimer(connectionID, _, on: let backoffTimerEL) = action.connection else {
1568+
XCTFail("Unexpected connection action: \(action.connection)")
1569+
return
1570+
}
1571+
XCTAssertIdentical(connectionEL, backoffTimerEL)
1572+
}
1573+
15301574
}
15311575

15321576
/// Should be used if you have a value of statically unknown type and want to compare its value to another `Equatable` value.

0 commit comments

Comments
 (0)