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
