From b9901cbb254b49a9041bdd4d945e6ab732555612 Mon Sep 17 00:00:00 2001 From: cnkwocha Date: Wed, 21 Aug 2024 14:06:55 +0100 Subject: [PATCH 1/3] Accept PING frames even when fully quiesced Motivation: When the connection is fully quiesced, an endpoint responds with a connection error when it receives a PING frame. This behaviour is unnecessary. Modifications: - Adjust `HTTP2ConnectionStateMachine` to accept PING frames even when the connection is fully quiesced. Result: When the connection is fully quiesced, an endpoint will treat the receipt of a PING frame as it would when the connection is active. --- .../ConnectionStateMachine/ConnectionStateMachine.swift | 5 +---- Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift | 4 +++- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift b/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift index aadf6147..c6f2a775 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") } diff --git a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift index dc002f76..6cabc6f2 100644 --- a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift +++ b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift @@ -1065,7 +1065,6 @@ class ConnectionStateMachineTests: XCTestCase { // 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 +1073,9 @@ class ConnectionStateMachineTests: XCTestCase { // Duplicate goaway is cool though. assertSucceeds(self.client.sendGoaway(lastStreamID: .rootStream)) assertSucceeds(self.server.receiveGoaway(lastStreamID: .rootStream)) + + // Receiving ping is cool too. + assertSucceeds(self.server.receivePing(ackFlagSet: false)) } func testPushesAfterSendingPrefaceAreInvalid() { From aae3b172927275416c0cc729309fbb411e7ed6f2 Mon Sep 17 00:00:00 2001 From: cnkwocha Date: Thu, 22 Aug 2024 17:06:31 +0100 Subject: [PATCH 2/3] Patch - Add E2E test. - Allow sending PINGs on a fully quiesced connection. --- .../ConnectionStateMachine.swift | 5 +-- .../ConnectionStateMachineTests.swift | 5 +-- .../SimpleClientServerTests.swift | 36 +++++++++++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift b/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift index c6f2a775..bac5a210 100644 --- a/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift +++ b/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift @@ -1187,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") } diff --git a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift index 6cabc6f2..788d01c2 100644 --- a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift +++ b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift @@ -1064,7 +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.client.sendPriority()) assertConnectionError(type: .protocolError, self.server.receivePriority()) assertConnectionError(type: .protocolError, self.client.sendSettings([])) @@ -1074,8 +1073,10 @@ class ConnectionStateMachineTests: XCTestCase { assertSucceeds(self.client.sendGoaway(lastStreamID: .rootStream)) assertSucceeds(self.server.receiveGoaway(lastStreamID: .rootStream)) - // Receiving ping is cool too. + // 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/SimpleClientServerTests.swift b/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift index ac25d258..567557c0 100644 --- a/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift +++ b/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift @@ -864,6 +864,42 @@ class SimpleClientServerTests: XCTestCase { XCTAssertTrue(error is NIOHTTP2Errors.UnableToParseFrame) } } + + @available(*, deprecated, message: "Deprecated so deprecated functionality can be tested without warnings") + func testSuccessfullyReceiveAndSendPingEvenWhenConnectionIsFullyQuiesced() throws { + let serverHandler = FrameRecorderHandler() + try self.basicHTTP2Connection() { channel, streamID in + return channel.pipeline.addHandler(serverHandler) + } + + let goAwayFrame = HTTP2Frame(streamID: .rootStream, payload: .goAway(lastStreamID: .rootStream, errorCode: .noError, opaqueData: nil)) + + // Fully quiesce the connection on the server. + serverChannel.writeAndFlush(goAwayFrame, promise: nil) + + let pingFrame = HTTP2Frame(streamID: .rootStream, payload: .ping(HTTP2PingData(), ack: false)) + + // Send PING frame to the server. + 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()) + } } final class ActionOnFlushHandler: ChannelOutboundHandler { From b97f845de6f11d9b5eb67a14cce55aecd8cfe981 Mon Sep 17 00:00:00 2001 From: cnkwocha Date: Fri, 23 Aug 2024 10:48:28 +0100 Subject: [PATCH 3/3] Adjust E2E test --- ...ntServerInlineStreamMultiplexerTests.swift | 33 +++++++++++++++++ .../SimpleClientServerTests.swift | 36 ------------------- 2 files changed, 33 insertions(+), 36 deletions(-) diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerInlineStreamMultiplexerTests.swift b/Tests/NIOHTTP2Tests/SimpleClientServerInlineStreamMultiplexerTests.swift index c0f2288c..3bca6486 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()) + } } diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift b/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift index 567557c0..ac25d258 100644 --- a/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift +++ b/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift @@ -864,42 +864,6 @@ class SimpleClientServerTests: XCTestCase { XCTAssertTrue(error is NIOHTTP2Errors.UnableToParseFrame) } } - - @available(*, deprecated, message: "Deprecated so deprecated functionality can be tested without warnings") - func testSuccessfullyReceiveAndSendPingEvenWhenConnectionIsFullyQuiesced() throws { - let serverHandler = FrameRecorderHandler() - try self.basicHTTP2Connection() { channel, streamID in - return channel.pipeline.addHandler(serverHandler) - } - - let goAwayFrame = HTTP2Frame(streamID: .rootStream, payload: .goAway(lastStreamID: .rootStream, errorCode: .noError, opaqueData: nil)) - - // Fully quiesce the connection on the server. - serverChannel.writeAndFlush(goAwayFrame, promise: nil) - - let pingFrame = HTTP2Frame(streamID: .rootStream, payload: .ping(HTTP2PingData(), ack: false)) - - // Send PING frame to the server. - 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()) - } } final class ActionOnFlushHandler: ChannelOutboundHandler {