Tolerate shutdown message after channel is closed (#646)

* Tolerate shutdown message after channel is closed

### 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.

Author: dnadoba

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)")
         }
     }
 
@@ -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,15 +164,23 @@ final class HTTP2Connection {
         return promise.futureResult
     }
 
-    private func start() -> EventLoopFuture<Int> {
+    func _start0() -> EventLoopFuture<Int> {
         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,26 @@ final class HTTP2Connection {
     private func shutdown0() {
         self.channel.eventLoop.assertInEventLoop()
 
-        self.state = .closing
+        switch self.state {
+        case .active:
+            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, .closing:
+            // we are already closing/closed and we need to tolerate this
+            break
+
+        case .initialized, .starting:
+            preconditionFailure("invalid state \(self.state)")
+        }
     }
 }
 

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),

Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift

@@ -42,6 +42,30 @@ class HTTP2ConnectionTests: XCTestCase {
         ).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
+        )
+        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()
+    }
+
     func testSimpleGetRequest() {
         let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1)
         let eventLoop = eventLoopGroup.next()