ソースを参照

Don't double ack pings (#1941)

Motivation:

The `ServerConnectionManagementHandler` acks pings, however NIOs HTTP/2
handler does this already. This results in pings received by the server
being acked twice.

Modifications:

- Don't ack pings in the `ServerConnectionManagementHandler`
- Update tests

Result:

Pings aren't ack'd twice
George Barnett 1 年間 前
コミット
f475dab85d

+ 1 - 3
Sources/GRPCHTTP2Core/Server/Connection/ServerConnectionManagementHandler.swift

@@ -458,9 +458,7 @@ extension ServerConnectionManagementHandler {
       context.close(promise: nil)
 
     case .sendAck:
-      let ping = HTTP2Frame(streamID: .rootStream, payload: .ping(data, ack: true))
-      context.write(self.wrapOutboundOut(ping), promise: nil)
-      self.maybeFlush(context: context)
+      ()  // ACKs are sent by NIO's HTTP/2 handler, don't double ack.
 
     case .none:
       ()

+ 3 - 18
Tests/GRPCHTTP2CoreTests/Server/Connection/ServerConnectionManagementHandlerTests.swift

@@ -213,12 +213,7 @@ final class ServerConnectionManagementHandlerTests: XCTestCase {
     // The first ping is valid, the second and third are strikes.
     for _ in 1 ... 3 {
       try connection.ping(data: HTTP2PingData(), ack: false)
-      let frame = try XCTUnwrap(connection.readFrame())
-      XCTAssertEqual(frame.streamID, .rootStream)
-      XCTAssertPing(frame.payload) { data, ack in
-        XCTAssertEqual(data, HTTP2PingData())
-        XCTAssertTrue(ack)
-      }
+      XCTAssertNil(try connection.readFrame())
     }
 
     // The fourth ping is the third strike and triggers a GOAWAY.
@@ -245,12 +240,7 @@ final class ServerConnectionManagementHandlerTests: XCTestCase {
 
     for _ in 1 ... 100 {
       try connection.ping(data: HTTP2PingData(), ack: false)
-      let frame = try XCTUnwrap(connection.readFrame())
-      XCTAssertEqual(frame.streamID, .rootStream)
-      XCTAssertPing(frame.payload) { data, ack in
-        XCTAssertEqual(data, HTTP2PingData())
-        XCTAssertTrue(ack)
-      }
+      XCTAssertNil(try connection.readFrame())
 
       // Advance by the ping interval.
       connection.advanceTime(by: .minutes(1))
@@ -268,12 +258,7 @@ final class ServerConnectionManagementHandlerTests: XCTestCase {
       // The first ping is valid, the second and third are strikes.
       for _ in 1 ... 3 {
         try connection.ping(data: HTTP2PingData(), ack: false)
-        let frame = try XCTUnwrap(connection.readFrame())
-        XCTAssertEqual(frame.streamID, .rootStream)
-        XCTAssertPing(frame.payload) { data, ack in
-          XCTAssertEqual(data, HTTP2PingData())
-          XCTAssertTrue(ack)
-        }
+        XCTAssertNil(try connection.readFrame())
       }
     }