Automatically send PING acks by default in OgHttp2Session. This CL allows OgHttp2Session to automatically enqueue PING ack frames in response to receiving non-ack PINGs from its peer. Note that OgHttp2Session already automatically enqueues SETTINGS ack frames: http://google3/third_party/http2/adapter/oghttp2_session.cc;l=843-845;rcl=403185538. This PING ack addition aligns OgHttp2Session with default nghttp2 behavior. This CL also adds an option to OgHttp2Session::Options to disable automatic PING acks, analogous to nghttp2_option_set_no_auto_ping_ack(). The default automatic PING acks are helpful in Envoy's frame flood detection tests. With this CL, the following four tests now pass with oghttp2 migration cl/392724171, as hypothesized/hoped for in http://b/201799377#comment2: - Http2FloodMitigationTest.UpstreamFloodDetectionIsOnByDefault http://sponge2/c75a3080-9839-4d4c-8ed6-4ad33a1d4aa5 - Http2FloodMitigationTest.UpstreamPingFlood http://sponge2/64cb3358-31f5-4b6e-b94c-bd7082c49fd4 - Http2FloodMitigationTest.UpstreamRstStreamOnStreamIdleTimeout http://sponge2/bb3585d3-ed50-4b2b-8d87-9123b6baa9ba - Http2FloodMitigationTest.UpstreamRstStreamOnDownstreamRemoteClose http://sponge2/34466067-2445-45ae-8cf8-61924a0f8d85 PiperOrigin-RevId: 405915725
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index 8e3aa6d..03e2629 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -6,6 +6,7 @@ #include "http2/adapter/oghttp2_util.h" #include "http2/adapter/test_frame_sequence.h" #include "http2/adapter/test_utils.h" +#include "third_party/nghttp2/src/lib/includes/nghttp2/nghttp2.h" #include "common/platform/api/quiche_test.h" namespace http2 { @@ -2691,6 +2692,76 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::GOAWAY})); } +TEST(NgHttp2AdapterTest, AutomaticSettingsAndPingAcks) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); + const std::string frames = + TestFrameSequence().ClientPreface().Ping(42).Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // PING + EXPECT_CALL(visitor, OnFrameHeader(0, _, PING, 0)); + EXPECT_CALL(visitor, OnPing(42, false)); + + const int64_t read_result = adapter->ProcessBytes(frames); + EXPECT_EQ(static_cast<size_t>(read_result), frames.size()); + + EXPECT_TRUE(adapter->want_write()); + + // Server preface does not appear to include the mandatory SETTINGS frame. + // SETTINGS ack + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + // PING ack + EXPECT_CALL(visitor, OnBeforeFrameSent(PING, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(PING, 0, _, 0x1, 0)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::PING})); +} + +TEST(NgHttp2AdapterTest, AutomaticPingAcksDisabled) { + DataSavingVisitor visitor; + nghttp2_option* options; + nghttp2_option_new(&options); + nghttp2_option_set_no_auto_ping_ack(options, 1); + auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor, options); + nghttp2_option_del(options); + + const std::string frames = + TestFrameSequence().ClientPreface().Ping(42).Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // PING + EXPECT_CALL(visitor, OnFrameHeader(0, _, PING, 0)); + EXPECT_CALL(visitor, OnPing(42, false)); + + const int64_t read_result = adapter->ProcessBytes(frames); + EXPECT_EQ(static_cast<size_t>(read_result), frames.size()); + + EXPECT_TRUE(adapter->want_write()); + + // Server preface does not appear to include the mandatory SETTINGS frame. + // SETTINGS ack + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + // No PING ack expected because automatic PING acks are disabled. + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); +} + } // namespace } // namespace test } // namespace adapter
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 4b19874..2fe13fd 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -120,6 +120,79 @@ } } +TEST_F(OgHttp2AdapterTest, AutomaticSettingsAndPingAcks) { + const std::string frames = + TestFrameSequence().ClientPreface().Ping(42).Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(http2_visitor_, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(http2_visitor_, OnSettingsStart()); + EXPECT_CALL(http2_visitor_, OnSettingsEnd()); + // PING + EXPECT_CALL(http2_visitor_, OnFrameHeader(0, _, PING, 0)); + EXPECT_CALL(http2_visitor_, OnPing(42, false)); + + const int64_t read_result = adapter_->ProcessBytes(frames); + EXPECT_EQ(static_cast<size_t>(read_result), frames.size()); + + EXPECT_TRUE(adapter_->want_write()); + + // Server preface (SETTINGS) + EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(http2_visitor_, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + // SETTINGS ack + EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(http2_visitor_, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + // PING ack + EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(PING, 0, _, 0x1)); + EXPECT_CALL(http2_visitor_, OnFrameSent(PING, 0, _, 0x1, 0)); + + int send_result = adapter_->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT( + http2_visitor_.data(), + EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::SETTINGS, spdy::SpdyFrameType::PING})); +} + +TEST_F(OgHttp2AdapterTest, AutomaticPingAcksDisabled) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kServer, + .auto_ping_ack = false}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + const std::string frames = + TestFrameSequence().ClientPreface().Ping(42).Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // PING + EXPECT_CALL(visitor, OnFrameHeader(0, _, PING, 0)); + EXPECT_CALL(visitor, OnPing(42, false)); + + const int64_t read_result = adapter->ProcessBytes(frames); + EXPECT_EQ(static_cast<size_t>(read_result), frames.size()); + + EXPECT_TRUE(adapter->want_write()); + + // Server preface (SETTINGS) + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + // SETTINGS ack + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + // No PING ack expected because automatic PING acks are disabled. + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::SETTINGS})); +} + TEST(OgHttp2AdapterClientTest, ClientHandlesTrailers) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kClient};
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index dd1a987..fe84230 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -851,6 +851,11 @@ void OgHttp2Session::OnPing(spdy::SpdyPingId unique_id, bool is_ack) { visitor_.OnPing(unique_id, is_ack); + if (options_.auto_ping_ack && !is_ack) { + auto ping = absl::make_unique<spdy::SpdyPingIR>(unique_id); + ping->set_is_ack(true); + EnqueueFrame(std::move(ping)); + } } void OgHttp2Session::OnGoAway(spdy::SpdyStreamId last_accepted_stream_id,
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h index 40a142b..3baf7dd 100644 --- a/http2/adapter/oghttp2_session.h +++ b/http2/adapter/oghttp2_session.h
@@ -32,6 +32,8 @@ public: struct QUICHE_EXPORT_PRIVATE Options { Perspective perspective = Perspective::kClient; + // Whether to automatically send PING acks when receiving a PING. + bool auto_ping_ack = true; }; OgHttp2Session(Http2VisitorInterface& visitor, Options options);
diff --git a/http2/adapter/oghttp2_session_test.cc b/http2/adapter/oghttp2_session_test.cc index 8951273..b4b464b 100644 --- a/http2/adapter/oghttp2_session_test.cc +++ b/http2/adapter/oghttp2_session_test.cc
@@ -611,14 +611,19 @@ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(PING, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(PING, 0, _, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(PING, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(PING, 0, _, 0x1, 0)); // Some bytes should have been serialized. int send_result = session.Send(); EXPECT_EQ(0, send_result); - // Initial SETTINGS, SETTINGS ack. - // TODO(birenroy): automatically queue PING acks. - EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, - spdy::SpdyFrameType::SETTINGS})); + // Initial SETTINGS, SETTINGS ack, and PING acks (for PING IDs 42 and 47). + EXPECT_THAT(visitor.data(), + EqualsFrames( + {spdy::SpdyFrameType::SETTINGS, spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::PING, spdy::SpdyFrameType::PING})); } // Verifies that a server session enqueues initial SETTINGS before whatever