[HTTP2] Tolerate GoAway and Settings frames after connection close (swift-server#578)

Author: fabianfett

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift

@@ -168,7 +168,7 @@ extension HTTP2IdleHandler {
 
         mutating func settingsReceived(_ settings: HTTP2Settings) -> Action {
             switch self.state {
-            case .initialized, .closed:
+            case .initialized:
                 preconditionFailure("Invalid state: \(self.state)")
 
             case .connected:
@@ -188,12 +188,17 @@ extension HTTP2IdleHandler {
 
             case .closing:
                 return .nothing
+
+            case .closed:
+                // We may receive a Settings frame after we have called connection close, because of
+                // packages being delivered from the incoming buffer.
+                return .nothing
             }
         }
 
         mutating func goAwayReceived() -> Action {
             switch self.state {
-            case .initialized, .closed:
+            case .initialized:
                 preconditionFailure("Invalid state: \(self.state)")
 
             case .connected:
@@ -206,6 +211,11 @@ extension HTTP2IdleHandler {
 
             case .closing:
                 return .notifyConnectionGoAwayReceived(close: false)
+
+            case .closed:
+                // We may receive a GoAway frame after we have called connection close, because of
+                // packages being delivered from the incoming buffer.
+                return .nothing
             }
         }
 
@@ -234,6 +244,9 @@ extension HTTP2IdleHandler {
 
         mutating func streamCreated() -> Action {
             switch self.state {
+            case .initialized, .connected:
+                preconditionFailure("Invalid state: \(self.state)")
+
             case .active(var openStreams, let maxStreams):
                 openStreams += 1
                 self.state = .active(openStreams: openStreams, maxStreams: maxStreams)
@@ -246,13 +259,18 @@ extension HTTP2IdleHandler {
                 self.state = .closing(openStreams: openStreams, maxStreams: maxStreams)
                 return .nothing
 
-            case .initialized, .connected, .closed:
-                preconditionFailure("Invalid state: \(self.state)")
+            case .closed:
+                // We may receive a events after we have called connection close, because of
+                // internal races. We should just ignore these cases.
+                return .nothing
             }
         }
 
         mutating func streamClosed() -> Action {
             switch self.state {
+            case .initialized, .connected:
+                preconditionFailure("Invalid state: \(self.state)")
+
             case .active(var openStreams, let maxStreams):
                 openStreams -= 1
                 assert(openStreams >= 0)
@@ -269,8 +287,10 @@ extension HTTP2IdleHandler {
                 self.state = .closing(openStreams: openStreams, maxStreams: maxStreams)
                 return .nothing
 
-            case .initialized, .connected, .closed:
-                preconditionFailure("Invalid state: \(self.state)")
+            case .closed:
+                // We may receive a events after we have called connection close, because of
+                // internal races. We should just ignore these cases.
+                return .nothing
             }
         }
     }

Tests/AsyncHTTPClientTests/HTTP2IdleHandlerTests+XCTest.swift

@@ -35,6 +35,7 @@ extension HTTP2IdleHandlerTests {
             ("testCloseEventWhileNoOpenStreams", testCloseEventWhileNoOpenStreams),
             ("testCloseEventWhileThereAreOpenStreams", testCloseEventWhileThereAreOpenStreams),
             ("testGoAwayWhileThereAreOpenStreams", testGoAwayWhileThereAreOpenStreams),
+            ("testReceiveSettingsAndGoAwayAfterClientSideClose", testReceiveSettingsAndGoAwayAfterClientSideClose),
         ]
     }
 }

Tests/AsyncHTTPClientTests/HTTP2IdleHandlerTests.swift

@@ -225,6 +225,32 @@ class HTTP2IdleHandlerTests: XCTestCase {
             }
         }
     }
+
+    func testReceiveSettingsAndGoAwayAfterClientSideClose() {
+        let delegate = MockHTTP2IdleHandlerDelegate()
+        let idleHandler = HTTP2IdleHandler(delegate: delegate, logger: Logger(label: "test"))
+        let embedded = EmbeddedChannel(handlers: [idleHandler])
+        XCTAssertNoThrow(try embedded.connect(to: .makeAddressResolvingHost("localhost", port: 0)).wait())
+
+        let settingsFrame = HTTP2Frame(streamID: 0, payload: .settings(.settings([.init(parameter: .maxConcurrentStreams, value: 10)])))
+        XCTAssertEqual(delegate.maxStreams, nil)
+        XCTAssertNoThrow(try embedded.writeInbound(settingsFrame))
+        XCTAssertEqual(delegate.maxStreams, 10)
+
+        XCTAssertTrue(embedded.isActive)
+        embedded.pipeline.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil)
+        XCTAssertFalse(embedded.isActive)
+
+        let newSettingsFrame = HTTP2Frame(streamID: 0, payload: .settings(.settings([.init(parameter: .maxConcurrentStreams, value: 20)])))
+        XCTAssertEqual(delegate.maxStreams, 10)
+        XCTAssertNoThrow(try embedded.writeInbound(newSettingsFrame))
+        XCTAssertEqual(delegate.maxStreams, 10, "Expected message to not be forwarded.")
+
+        let goAwayFrame = HTTP2Frame(streamID: HTTP2StreamID(0), payload: .goAway(lastStreamID: 2, errorCode: .http11Required, opaqueData: nil))
+        XCTAssertEqual(delegate.goAwayReceived, false)
+        XCTAssertNoThrow(try embedded.writeInbound(goAwayFrame))
+        XCTAssertEqual(delegate.goAwayReceived, false, "Expected go away to not be forwarded.")
+    }
 }
 
 class MockHTTP2IdleHandlerDelegate: HTTP2IdleHandlerDelegate {