Repository: apple/swift-nio-http2
Pull request #455 (2024-08-21 14:33:47)

Accept PING frames even when fully quiesced
Motivation:

When the connection is fully quiesced, an endpoint responds with a connection error when it receives or tries to respond to a PING frame. This behaviour is unnecessary.

Modifications:

- Adjust `HTTP2ConnectionStateMachine` to successfully receive and send PING frames even when the connection is fully quiesced.

Result:

When the connection is fully quiesced, an endpoint will treat receipt and sending of a PING frame as it would when the connection is active.

clintonpi: Oops. Yes, of course.

Lukasa: @swift-server-bot test this please

Patch:
diff --git a/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift b/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift
index aadf614..bac5a21 100644
--- a/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift
+++ b/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift
@@ -1169,16 +1169,13 @@ extension HTTP2ConnectionStateMachine {
         // RFC 7540 that says that receiving PINGs with ACK flags set when no PING ACKs are expected is forbidden. This is
         // very strange, but we allow it.
         switch self.state {
-        case .prefaceReceived, .active, .locallyQuiesced, .remotelyQuiesced, .bothQuiescing, .quiescingPrefaceReceived:
+        case .prefaceReceived, .active, .locallyQuiesced, .remotelyQuiesced, .bothQuiescing, .quiescingPrefaceReceived, .fullyQuiesced:
             return (.init(result: .succeed, effect: nil), ackFlagSet ? .nothing : .sendAck)
 
         case .idle, .prefaceSent, .quiescingPrefaceSent:
             // We're waiting for the remote preface.
             return (.init(result: .connectionError(underlyingError: NIOHTTP2Errors.missingPreface(), type: .protocolError), effect: nil), .nothing)
 
-        case .fullyQuiesced:
-            return (.init(result: .connectionError(underlyingError: NIOHTTP2Errors.ioOnClosedConnection(), type: .protocolError), effect: nil), .nothing)
-
         case .modifying:
             preconditionFailure("Must not be left in modifying state")
         }
@@ -1190,16 +1187,13 @@ extension HTTP2ConnectionStateMachine {
         // RFC 7540 that says that sending PINGs with ACK flags set when no PING ACKs are expected is forbidden. This is
         // very strange, but we allow it.
         switch self.state {
-        case .prefaceSent, .active, .locallyQuiesced, .remotelyQuiesced, .bothQuiescing, .quiescingPrefaceSent:
+        case .prefaceSent, .active, .locallyQuiesced, .remotelyQuiesced, .bothQuiescing, .quiescingPrefaceSent, .fullyQuiesced:
             return .init(result: .succeed, effect: nil)
 
         case .idle, .prefaceReceived, .quiescingPrefaceReceived:
             // We're waiting for the local preface.
             return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.missingPreface(), type: .protocolError), effect: nil)
 
-        case .fullyQuiesced:
-            return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.ioOnClosedConnection(), type: .protocolError), effect: nil)
-
         case .modifying:
             preconditionFailure("Must not be left in modifying state")
         }


Test patch:
diff --git a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
index dc002f7..788d01c 100644
--- a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
+++ b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
@@ -1064,8 +1064,6 @@ class ConnectionStateMachineTests: XCTestCase {
         assertConnectionError(type: .protocolError, self.client.receivePushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders))
 
         // Connectiony things don't work either.
-        assertConnectionError(type: .protocolError, self.client.sendPing())
-        assertConnectionError(type: .protocolError, self.server.receivePing(ackFlagSet: false))
         assertConnectionError(type: .protocolError, self.client.sendPriority())
         assertConnectionError(type: .protocolError, self.server.receivePriority())
         assertConnectionError(type: .protocolError, self.client.sendSettings([]))
@@ -1074,6 +1072,11 @@ class ConnectionStateMachineTests: XCTestCase {
         // Duplicate goaway is cool though.
         assertSucceeds(self.client.sendGoaway(lastStreamID: .rootStream))
         assertSucceeds(self.server.receiveGoaway(lastStreamID: .rootStream))
+
+        // PINGing is cool too.
+        assertSucceeds(self.client.sendPing())
+        assertSucceeds(self.server.receivePing(ackFlagSet: false))
+        assertSucceeds(self.client.receivePing(ackFlagSet: true))
     }
 
     func testPushesAfterSendingPrefaceAreInvalid() {
diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerInlineStreamMultiplexerTests.swift b/Tests/NIOHTTP2Tests/SimpleClientServerInlineStreamMultiplexerTests.swift
index c0f2288..3bca648 100644
--- a/Tests/NIOHTTP2Tests/SimpleClientServerInlineStreamMultiplexerTests.swift
+++ b/Tests/NIOHTTP2Tests/SimpleClientServerInlineStreamMultiplexerTests.swift
@@ -328,4 +328,37 @@ class SimpleClientServerInlineStreamMultiplexerTests: XCTestCase {
         promise.succeed(())
         stream.writeAndFlush(headerPayload, promise: promise)
     }
+
+    func testSuccessfullyReceiveAndSendPingEvenWhenConnectionIsFullyQuiesced() throws {
+        let serverHandler = InboundFramePayloadRecorder()
+        try self.basicHTTP2Connection() { channel in
+            return channel.pipeline.addHandler(serverHandler)
+        }
+
+        // Fully quiesce the connection on the server.
+        let goAwayFrame = HTTP2Frame(streamID: .rootStream, payload: .goAway(lastStreamID: .rootStream, errorCode: .noError, opaqueData: nil))
+        serverChannel.writeAndFlush(goAwayFrame, promise: nil)
+
+        // Send PING frame to the server.
+        let pingFrame = HTTP2Frame(streamID: .rootStream, payload: .ping(HTTP2PingData(), ack: false))
+        clientChannel.writeAndFlush(pingFrame, promise: nil)
+
+        self.interactInMemory(self.clientChannel, self.serverChannel)
+
+        // The client receives the GOAWAY frame and fully quiesces the connection.
+        try self.clientChannel.assertReceivedFrame().assertGoAwayFrame(lastStreamID: .rootStream, errorCode: 0, opaqueData: nil)
+
+        // The server receives and responds to the PING.
+        try self.serverChannel.assertReceivedFrame().assertPingFrame(ack: false, opaqueData: HTTP2PingData())
+
+        // The client receives the PING response.
+        try self.clientChannel.assertReceivedFrame().assertPingFrame(ack: true, opaqueData: HTTP2PingData())
+
+        // No frames left.
+        self.clientChannel.assertNoFramesReceived()
+        self.serverChannel.assertNoFramesReceived()
+
+        XCTAssertNoThrow(try self.clientChannel.finish())
+        XCTAssertNoThrow(try self.serverChannel.finish())
+    }
 }