Skip to content

Commit e4b11eb

Browse files
dnadobadnadoba
authored
Fix HTTP1 to HTTP2 migration while shutdown is in progress (#530)
* Fix HTTP1 to HTTP2 migration while shutdown is in progress ### Motivation Calling `HTTPClient.shutdown()` may never return if connections are still starting and one new established connection results in a state migration (i.e. from HTTP1 to HTTP2 or vice versa). We forgot to migrate the shutdown state. This could result in a large dealy until `.shutdown()` returns because we wait until connections are closed because of idle timeout. Worse, it could also never return if more requests are queued because the connections would not be idle and therefore not close itself. ###Changes - Mirgrate shutdown state too - add tests for this specific case * simplify testMigrationFromHTTP1ToHTTP2WhileShuttingDown * add http2 to http1 migration test
1 parent d952776 commit e4b11eb

File tree

6 files changed

+199
-59
lines changed

6 files changed

+199
-59
lines changed

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

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,6 @@ import NIOCore
1616

1717
extension HTTPConnectionPool {
1818
struct HTTP1StateMachine {
19-
enum State: Equatable {
20-
case running
21-
case shuttingDown(unclean: Bool)
22-
case shutDown
23-
}
24-
2519
typealias Action = HTTPConnectionPool.StateMachine.Action
2620
typealias ConnectionMigrationAction = HTTPConnectionPool.StateMachine.ConnectionMigrationAction
2721
typealias EstablishedAction = HTTPConnectionPool.StateMachine.EstablishedAction
@@ -34,15 +28,20 @@ extension HTTPConnectionPool {
3428
private var lastConnectFailure: Error?
3529

3630
private(set) var requests: RequestQueue
37-
private var state: State = .running
31+
private(set) var lifecycleState: StateMachine.LifecycleState
3832

39-
init(idGenerator: Connection.ID.Generator, maximumConcurrentConnections: Int) {
33+
init(
34+
idGenerator: Connection.ID.Generator,
35+
maximumConcurrentConnections: Int,
36+
lifecycleState: StateMachine.LifecycleState
37+
) {
4038
self.connections = HTTP1Connections(
4139
maximumConcurrentConnections: maximumConcurrentConnections,
4240
generator: idGenerator
4341
)
4442

4543
self.requests = RequestQueue()
44+
self.lifecycleState = lifecycleState
4645
}
4746

4847
mutating func migrateFromHTTP2(
@@ -111,7 +110,7 @@ extension HTTPConnectionPool {
111110
// MARK: - Events -
112111

113112
mutating func executeRequest(_ request: Request) -> Action {
114-
switch self.state {
113+
switch self.lifecycleState {
115114
case .running:
116115
if let eventLoop = request.requiredEventLoop {
117116
return self.executeRequestOnRequiredEventLoop(request, eventLoop: eventLoop)
@@ -218,7 +217,7 @@ extension HTTPConnectionPool {
218217
self.failedConsecutiveConnectionAttempts += 1
219218
self.lastConnectFailure = error
220219

221-
switch self.state {
220+
switch self.lifecycleState {
222221
case .running:
223222
// We don't care how many waiting requests we have at this point, we will schedule a
224223
// retry. More tasks, may appear until the backoff has completed. The final
@@ -243,7 +242,7 @@ extension HTTPConnectionPool {
243242
}
244243

245244
mutating func connectionCreationBackoffDone(_ connectionID: Connection.ID) -> Action {
246-
switch self.state {
245+
switch self.lifecycleState {
247246
case .running:
248247
// The naming of `failConnection` is a little confusing here. All it does is moving the
249248
// connection state from `.backingOff` to `.closed` here. It also returns the
@@ -271,7 +270,7 @@ extension HTTPConnectionPool {
271270
return .none
272271
}
273272

274-
precondition(self.state == .running, "If we are shutting down, we must not have any idle connections")
273+
precondition(self.lifecycleState == .running, "If we are shutting down, we must not have any idle connections")
275274

276275
return .init(
277276
request: .none,
@@ -332,7 +331,7 @@ extension HTTPConnectionPool {
332331
}
333332

334333
mutating func shutdown() -> Action {
335-
precondition(self.state == .running, "Shutdown must only be called once")
334+
precondition(self.lifecycleState == .running, "Shutdown must only be called once")
336335

337336
// If we have remaining request queued, we should fail all of them with a cancelled
338337
// error.
@@ -350,10 +349,10 @@ extension HTTPConnectionPool {
350349
let isShutdown: StateMachine.ConnectionAction.IsShutdown
351350
let unclean = !(cleanupContext.cancel.isEmpty && waitingRequests.isEmpty)
352351
if self.connections.isEmpty && self.http2Connections == nil {
353-
self.state = .shutDown
352+
self.lifecycleState = .shutDown
354353
isShutdown = .yes(unclean: unclean)
355354
} else {
356-
self.state = .shuttingDown(unclean: unclean)
355+
self.lifecycleState = .shuttingDown(unclean: unclean)
357356
isShutdown = .no
358357
}
359358

@@ -371,7 +370,7 @@ extension HTTPConnectionPool {
371370
at index: Int,
372371
context: HTTP1Connections.IdleConnectionContext
373372
) -> EstablishedAction {
374-
switch self.state {
373+
switch self.lifecycleState {
375374
case .running:
376375
switch context.use {
377376
case .generalPurpose:
@@ -457,7 +456,7 @@ extension HTTPConnectionPool {
457456
at index: Int,
458457
context: HTTP1Connections.FailedConnectionContext
459458
) -> Action {
460-
switch self.state {
459+
switch self.lifecycleState {
461460
case .running:
462461
switch context.use {
463462
case .generalPurpose:
@@ -537,7 +536,7 @@ extension HTTPConnectionPool {
537536
}
538537

539538
mutating func http2ConnectionClosed(_ connectionID: Connection.ID) -> Action {
540-
switch self.state {
539+
switch self.lifecycleState {
541540
case .running:
542541
_ = self.http2Connections?.failConnection(connectionID)
543542
if self.http2Connections?.isEmpty == true {
@@ -570,7 +569,7 @@ extension HTTPConnectionPool {
570569
mutating func http2ConnectionStreamClosed(_ connectionID: Connection.ID) -> Action {
571570
// It is save to bang the http2Connections here. If we get this callback but we don't have
572571
// http2 connections something has gone terribly wrong.
573-
switch self.state {
572+
switch self.lifecycleState {
574573
case .running:
575574
let (index, context) = self.http2Connections!.releaseStream(connectionID)
576575
guard context.isIdle else {

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

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,6 @@ extension HTTPConnectionPool {
2222
typealias EstablishedAction = HTTPConnectionPool.StateMachine.EstablishedAction
2323
typealias EstablishedConnectionAction = HTTPConnectionPool.StateMachine.EstablishedConnectionAction
2424

25-
private enum State: Equatable {
26-
case running
27-
case shuttingDown(unclean: Bool)
28-
case shutDown
29-
}
30-
3125
private var lastConnectFailure: Error?
3226
private var failedConsecutiveConnectionAttempts = 0
3327

@@ -38,15 +32,17 @@ extension HTTPConnectionPool {
3832

3933
private let idGenerator: Connection.ID.Generator
4034

41-
private var state: State = .running
35+
private(set) var lifecycleState: StateMachine.LifecycleState
4236

4337
init(
44-
idGenerator: Connection.ID.Generator
38+
idGenerator: Connection.ID.Generator,
39+
lifecycleState: StateMachine.LifecycleState
4540
) {
4641
self.idGenerator = idGenerator
4742
self.requests = RequestQueue()
4843

4944
self.connections = HTTP2Connections(generator: idGenerator)
45+
self.lifecycleState = lifecycleState
5046
}
5147

5248
mutating func migrateFromHTTP1(
@@ -112,7 +108,7 @@ extension HTTPConnectionPool {
112108
}
113109

114110
mutating func executeRequest(_ request: Request) -> Action {
115-
switch self.state {
111+
switch self.lifecycleState {
116112
case .running:
117113
if let eventLoop = request.requiredEventLoop {
118114
return self.executeRequest(request, onRequired: eventLoop)
@@ -236,7 +232,7 @@ extension HTTPConnectionPool {
236232
at index: Int,
237233
context: HTTP2Connections.EstablishedConnectionContext
238234
) -> EstablishedAction {
239-
switch self.state {
235+
switch self.lifecycleState {
240236
case .running:
241237
// We prioritise requests with a required event loop over those without a requirement.
242238
// This can cause starvation for request without a required event loop.
@@ -323,7 +319,7 @@ extension HTTPConnectionPool {
323319
}
324320

325321
private mutating func nextActionForFailedConnection(at index: Int, on eventLoop: EventLoop) -> Action {
326-
switch self.state {
322+
switch self.lifecycleState {
327323
case .running:
328324
// we do not know if we have created this connection for a request with a required
329325
// event loop or not. However, we do not need this information and can infer
@@ -378,7 +374,7 @@ extension HTTPConnectionPool {
378374
}
379375

380376
private mutating func nextActionForClosingConnection(on eventLoop: EventLoop) -> Action {
381-
switch self.state {
377+
switch self.lifecycleState {
382378
case .running:
383379
let hasPendingRequest = !self.requests.isEmpty(for: eventLoop) || !self.requests.isEmpty(for: nil)
384380
guard hasPendingRequest else {
@@ -463,7 +459,7 @@ extension HTTPConnectionPool {
463459
return .none
464460
}
465461

466-
precondition(self.state == .running, "If we are shutting down, we must not have any idle connections")
462+
precondition(self.lifecycleState == .running, "If we are shutting down, we must not have any idle connections")
467463

468464
return .init(
469465
request: .none,
@@ -479,7 +475,7 @@ extension HTTPConnectionPool {
479475
if self.http1Connections!.isEmpty {
480476
self.http1Connections = nil
481477
}
482-
switch self.state {
478+
switch self.lifecycleState {
483479
case .running:
484480
return .none
485481
case .shuttingDown(let unclean):
@@ -510,7 +506,7 @@ extension HTTPConnectionPool {
510506
self.http1Connections = nil
511507

512508
// we must also check, if we are shutting down. Was this maybe out last connection?
513-
switch self.state {
509+
switch self.lifecycleState {
514510
case .running:
515511
return .init(request: .none, connection: .closeConnection(connection, isShutdown: .no))
516512
case .shuttingDown(let unclean):
@@ -543,10 +539,10 @@ extension HTTPConnectionPool {
543539
let unclean = !(cleanupContext.cancel.isEmpty && waitingRequests.isEmpty && self.http1Connections == nil)
544540
if self.connections.isEmpty && self.http1Connections == nil {
545541
isShutdown = .yes(unclean: unclean)
546-
self.state = .shutDown
542+
self.lifecycleState = .shutDown
547543
} else {
548544
isShutdown = .no
549-
self.state = .shuttingDown(unclean: unclean)
545+
self.lifecycleState = .shuttingDown(unclean: unclean)
550546
}
551547
return .init(
552548
request: requestAction,

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

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,12 @@ extension HTTPConnectionPool {
6666
case none
6767
}
6868

69+
enum LifecycleState: Equatable {
70+
case running
71+
case shuttingDown(unclean: Bool)
72+
case shutDown
73+
}
74+
6975
enum HTTPVersionState {
7076
case http1(HTTP1StateMachine)
7177
case http2(HTTP2StateMachine)
@@ -88,7 +94,6 @@ extension HTTPConnectionPool {
8894
}
8995

9096
var state: HTTPVersionState
91-
var isShuttingDown: Bool = false
9297

9398
let idGenerator: Connection.ID.Generator
9499
let maximumConcurrentHTTP1Connections: Int
@@ -98,7 +103,8 @@ extension HTTPConnectionPool {
98103
self.idGenerator = idGenerator
99104
let http1State = HTTP1StateMachine(
100105
idGenerator: idGenerator,
101-
maximumConcurrentConnections: maximumConcurrentHTTP1Connections
106+
maximumConcurrentConnections: maximumConcurrentHTTP1Connections,
107+
lifecycleState: .running
102108
)
103109
self.state = .http1(http1State)
104110
}
@@ -121,7 +127,8 @@ extension HTTPConnectionPool {
121127
case .http2(let http2StateMachine):
122128
var http1StateMachine = HTTP1StateMachine(
123129
idGenerator: self.idGenerator,
124-
maximumConcurrentConnections: self.maximumConcurrentHTTP1Connections
130+
maximumConcurrentConnections: self.maximumConcurrentHTTP1Connections,
131+
lifecycleState: http2StateMachine.lifecycleState
125132
)
126133

127134
let newConnectionAction = http1StateMachine.migrateFromHTTP2(
@@ -140,7 +147,8 @@ extension HTTPConnectionPool {
140147
case .http1(let http1StateMachine):
141148

142149
var http2StateMachine = HTTP2StateMachine(
143-
idGenerator: self.idGenerator
150+
idGenerator: self.idGenerator,
151+
lifecycleState: http1StateMachine.lifecycleState
144152
)
145153
let migrationAction = http2StateMachine.migrateFromHTTP1(
146154
http1Connections: http1StateMachine.connections,
@@ -263,10 +271,6 @@ extension HTTPConnectionPool {
263271
}
264272

265273
mutating func shutdown() -> Action {
266-
precondition(!self.isShuttingDown, "Shutdown must only be called once")
267-
268-
self.isShuttingDown = true
269-
270274
return self.state.modify(http1: { http1 in
271275
http1.shutdown()
272276
}, http2: { http2 in

‎Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
322322
#if compiler(>=5.5) && canImport(_Concurrency)
323323
guard #available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *) else { return }
324324
XCTAsyncTest(timeout: 5) {
325-
let bin = HTTPBin()
325+
let bin = HTTPBin(.http2(compress: false))
326326
defer { XCTAssertNoThrow(try bin.shutdown()) }
327327
let client = makeDefaultHTTPClient()
328328
defer { XCTAssertNoThrow(try client.syncShutdown()) }
@@ -368,7 +368,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
368368
#if compiler(>=5.5) && canImport(_Concurrency)
369369
guard #available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *) else { return }
370370
XCTAsyncTest(timeout: 5) {
371-
let bin = HTTPBin()
371+
let bin = HTTPBin(.http2(compress: false))
372372
defer { XCTAssertNoThrow(try bin.shutdown()) }
373373
let client = makeDefaultHTTPClient()
374374
defer { XCTAssertNoThrow(try client.syncShutdown()) }
@@ -381,7 +381,6 @@ final class AsyncAwaitEndToEndTests: XCTestCase {
381381
await XCTAssertThrowsError(try await task.value) {
382382
XCTAssertEqual($0 as? HTTPClientError, HTTPClientError.deadlineExceeded)
383383
}
384-
print("done")
385384
}
386385
#endif
387386
}

‎Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP2StateMachineTests+XCTest.swift‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,10 @@ extension HTTPConnectionPool_HTTP2StateMachineTests {
3737
("testGoAwayWithLeasedStream", testGoAwayWithLeasedStream),
3838
("testGoAwayWithPendingRequestsStartsNewConnection", testGoAwayWithPendingRequestsStartsNewConnection),
3939
("testMigrationFromHTTP1ToHTTP2", testMigrationFromHTTP1ToHTTP2),
40+
("testMigrationFromHTTP1ToHTTP2WhileShuttingDown", testMigrationFromHTTP1ToHTTP2WhileShuttingDown),
4041
("testMigrationFromHTTP1ToHTTP2WithAlreadyStartedHTTP1Connections", testMigrationFromHTTP1ToHTTP2WithAlreadyStartedHTTP1Connections),
4142
("testHTTP2toHTTP1Migration", testHTTP2toHTTP1Migration),
43+
("testHTTP2toHTTP1MigrationDuringShutdown", testHTTP2toHTTP1MigrationDuringShutdown),
4244
("testConnectionIsImmediatelyCreatedAfterBackoffTimerFires", testConnectionIsImmediatelyCreatedAfterBackoffTimerFires),
4345
("testMaxConcurrentStreamsIsRespected", testMaxConcurrentStreamsIsRespected),
4446
("testEventsAfterConnectionIsClosed", testEventsAfterConnectionIsClosed),

0 commit comments

Comments
 (0)