From 3b3b5d0f073d40a559b8471b0b8941c872bb5584 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Mon, 7 Nov 2022 22:03:23 +0100 Subject: [PATCH 1/6] Tolerate shutdown message after channel is closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Motivation A channel can close unexpectedly if something goes wrong. We may in the meantime have scheduled the connection for graceful shutdown but the connection has not yet seen the message. We need to still accept the shutdown message and just ignore it if we are already closed. ### Modification - ignore calls to shutdown if the channel is already closed - add a test which would previously crash because we have transition from the closed state to the closing state and we hit the deinit precondition - include the current state in preconditions if we are in the wrong state ### Result We don’t hit the precondition in the deinit in the scenario described above and have more descriptive crashes if something still goes wrong. --- .../HTTP2/HTTP2Connection.swift | 41 +++++++++++++------ .../HTTP2ConnectionTests.swift | 22 ++++++++++ 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift index 701e630c2..218752b4b 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift @@ -111,7 +111,7 @@ final class HTTP2Connection { deinit { guard case .closed = self.state else { - preconditionFailure("Connection must be closed, before we can deinit it") + preconditionFailure("Connection must be closed, before we can deinit it. Current state: \(self.state)") } } @@ -164,15 +164,23 @@ final class HTTP2Connection { return promise.futureResult } - private func start() -> EventLoopFuture { + func start() -> EventLoopFuture { self.channel.eventLoop.assertInEventLoop() let readyToAcceptConnectionsPromise = self.channel.eventLoop.makePromise(of: Int.self) self.state = .starting(readyToAcceptConnectionsPromise) self.channel.closeFuture.whenComplete { _ in - self.state = .closed - self.delegate.http2ConnectionClosed(self) + switch self.state { + case .initialized, .closed: + preconditionFailure("invalid state \(self.state)") + case .starting(let readyToAcceptConnectionsPromise): + self.state = .closed + readyToAcceptConnectionsPromise.fail(HTTPClientError.remoteConnectionClosed) + case .active, .closing: + self.state = .closed + self.delegate.http2ConnectionClosed(self) + } } do { @@ -258,16 +266,25 @@ final class HTTP2Connection { private func shutdown0() { self.channel.eventLoop.assertInEventLoop() - self.state = .closing + switch self.state { + case .active, .starting: + self.state = .closing - // inform all open streams, that the currently running request should be cancelled. - self.openStreams.forEach { box in - box.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil) - } + // inform all open streams, that the currently running request should be cancelled. + self.openStreams.forEach { box in + box.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil) + } - // inform the idle connection handler, that connection should be closed, once all streams - // are closed. - self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil) + // inform the idle connection handler, that connection should be closed, once all streams + // are closed. + self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil) + + case .closed: + // we are already closed and we need to tolerate this + break + case .initialized, .closing: + preconditionFailure("invalid state \(self.state)") + } } } diff --git a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift index a9ff14f49..6398612af 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift @@ -41,6 +41,28 @@ class HTTP2ConnectionTests: XCTestCase { logger: logger ).wait()) } + + func testConnectionToleratesShutdownEventsAfterAlreadyClosed() { + let embedded = EmbeddedChannel() + XCTAssertNoThrow(try embedded.connect(to: SocketAddress(ipAddress: "127.0.0.1", port: 3000)).wait()) + + let logger = Logger(label: "test.http2.connection") + let connection = HTTP2Connection( + channel: embedded, + connectionID: 0, + decompression: .disabled, + delegate: TestHTTP2ConnectionDelegate(), + logger: logger + ) + _ = connection.start() + + XCTAssertNoThrow(try embedded.close().wait()) + // to really destroy the channel we need to tick once + embedded.embeddedEventLoop.run() + + // should not crash + connection.shutdown() + } func testSimpleGetRequest() { let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1) From 1a512292a6f24f45826dc74ee6000b41561ffcd1 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 8 Nov 2022 10:17:56 +0100 Subject: [PATCH 2/6] Tolerate multiple calls to shutdown() --- .../ConnectionPool/HTTP2/HTTP2Connection.swift | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift index 218752b4b..8def88d19 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift @@ -267,7 +267,7 @@ final class HTTP2Connection { self.channel.eventLoop.assertInEventLoop() switch self.state { - case .active, .starting: + case .active: self.state = .closing // inform all open streams, that the currently running request should be cancelled. @@ -279,10 +279,11 @@ final class HTTP2Connection { // are closed. self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil) - case .closed: - // we are already closed and we need to tolerate this + case .closed, .closing: + // we are already closing/closed and we need to tolerate this break - case .initialized, .closing: + + case .initialized, .starting: preconditionFailure("invalid state \(self.state)") } } From 4d8b475238f0157283cef7bcff17734c44e40440 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 8 Nov 2022 13:15:46 +0100 Subject: [PATCH 3/6] Soundness --- .../ConnectionPool/HTTP2/HTTP2Connection.swift | 4 ++-- .../HTTP2ConnectionTests+XCTest.swift | 1 + Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift | 8 ++++---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift index 8def88d19..08fe5879f 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift @@ -278,11 +278,11 @@ final class HTTP2Connection { // inform the idle connection handler, that connection should be closed, once all streams // are closed. self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil) - + case .closed, .closing: // we are already closing/closed and we need to tolerate this break - + case .initialized, .starting: preconditionFailure("invalid state \(self.state)") } diff --git a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests+XCTest.swift index 9f9582d9f..06b60f757 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests+XCTest.swift @@ -26,6 +26,7 @@ extension HTTP2ConnectionTests { static var allTests: [(String, (HTTP2ConnectionTests) -> () throws -> Void)] { return [ ("testCreateNewConnectionFailureClosedIO", testCreateNewConnectionFailureClosedIO), + ("testConnectionToleratesShutdownEventsAfterAlreadyClosed", testConnectionToleratesShutdownEventsAfterAlreadyClosed), ("testSimpleGetRequest", testSimpleGetRequest), ("testEveryDoneRequestLeadsToAStreamAvailableCall", testEveryDoneRequestLeadsToAStreamAvailableCall), ("testCancelAllRunningRequests", testCancelAllRunningRequests), diff --git a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift index 6398612af..15486728f 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift @@ -41,11 +41,11 @@ class HTTP2ConnectionTests: XCTestCase { logger: logger ).wait()) } - + func testConnectionToleratesShutdownEventsAfterAlreadyClosed() { let embedded = EmbeddedChannel() XCTAssertNoThrow(try embedded.connect(to: SocketAddress(ipAddress: "127.0.0.1", port: 3000)).wait()) - + let logger = Logger(label: "test.http2.connection") let connection = HTTP2Connection( channel: embedded, @@ -55,11 +55,11 @@ class HTTP2ConnectionTests: XCTestCase { logger: logger ) _ = connection.start() - + XCTAssertNoThrow(try embedded.close().wait()) // to really destroy the channel we need to tick once embedded.embeddedEventLoop.run() - + // should not crash connection.shutdown() } From ba76fe445fcd25559a564364229c742db8c63316 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 8 Nov 2022 13:57:05 +0100 Subject: [PATCH 4/6] Rename `start` to `start0` And wait for connection start to fail --- .../ConnectionPool/HTTP2/HTTP2Connection.swift | 4 ++-- Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift index 08fe5879f..64f05a9d0 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift @@ -129,7 +129,7 @@ final class HTTP2Connection { delegate: delegate, logger: logger ) - return connection.start().map { maxStreams in (connection, maxStreams) } + return connection.start0().map { maxStreams in (connection, maxStreams) } } func executeRequest(_ request: HTTPExecutableRequest) { @@ -164,7 +164,7 @@ final class HTTP2Connection { return promise.futureResult } - func start() -> EventLoopFuture { + func start0() -> EventLoopFuture { self.channel.eventLoop.assertInEventLoop() let readyToAcceptConnectionsPromise = self.channel.eventLoop.makePromise(of: Int.self) diff --git a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift index 15486728f..12cb75e04 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift @@ -54,11 +54,13 @@ class HTTP2ConnectionTests: XCTestCase { delegate: TestHTTP2ConnectionDelegate(), logger: logger ) - _ = connection.start() + let startFuture = connection.start0() XCTAssertNoThrow(try embedded.close().wait()) // to really destroy the channel we need to tick once embedded.embeddedEventLoop.run() + + XCTAssertThrowsError(try startFuture.wait()) // should not crash connection.shutdown() From b17d92e1acf23e031669148bd741220ec8fed04a Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 8 Nov 2022 13:58:57 +0100 Subject: [PATCH 5/6] Soundness --- Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift index 12cb75e04..ca7d750a1 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift @@ -59,7 +59,7 @@ class HTTP2ConnectionTests: XCTestCase { XCTAssertNoThrow(try embedded.close().wait()) // to really destroy the channel we need to tick once embedded.embeddedEventLoop.run() - + XCTAssertThrowsError(try startFuture.wait()) // should not crash From 510d6355ea9820d5509e93e428f42e3918479693 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Tue, 8 Nov 2022 17:38:14 +0100 Subject: [PATCH 6/6] Add underscore prefix to `start0` --- .../ConnectionPool/HTTP2/HTTP2Connection.swift | 4 ++-- Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift index 64f05a9d0..5859e619a 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift @@ -129,7 +129,7 @@ final class HTTP2Connection { delegate: delegate, logger: logger ) - return connection.start0().map { maxStreams in (connection, maxStreams) } + return connection._start0().map { maxStreams in (connection, maxStreams) } } func executeRequest(_ request: HTTPExecutableRequest) { @@ -164,7 +164,7 @@ final class HTTP2Connection { return promise.futureResult } - func start0() -> EventLoopFuture { + func _start0() -> EventLoopFuture { self.channel.eventLoop.assertInEventLoop() let readyToAcceptConnectionsPromise = self.channel.eventLoop.makePromise(of: Int.self) diff --git a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift index ca7d750a1..652884a84 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift @@ -54,7 +54,7 @@ class HTTP2ConnectionTests: XCTestCase { delegate: TestHTTP2ConnectionDelegate(), logger: logger ) - let startFuture = connection.start0() + let startFuture = connection._start0() XCTAssertNoThrow(try embedded.close().wait()) // to really destroy the channel we need to tick once