Clear the OgHttp2Session pending frames queue on starting an immediate GOAWAY.
This CL completes an OgHttp2Session GOAWAY TODO that enables better GOAWAY
tracking and brings oghttp2 behavior more in line with nghttp2.
Note that OgHttp2Session does not clear RST_STREAM frames from the queue on
immediate GOAWAY, as the RST_STREAM frames provide important signal to the peer
about potential errors below the last stream ID.
This CL also adds logic to preserve user-submitted initial SETTINGS in the face
of an immediate GOAWAY.
PiperOrigin-RevId: 417871466
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index 15b3495..3e35524 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -3994,6 +3994,9 @@
DataSavingVisitor visitor;
auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+ // Submit a custom initial SETTINGS frame with one setting.
+ adapter->SubmitSettings({{HEADER_TABLE_SIZE, 100u}});
+
const std::string frames = TestFrameSequence()
.ClientPreface()
.Headers(1,
@@ -4026,9 +4029,6 @@
1, ToHeaders({{":status", "200"}}), std::move(body));
ASSERT_EQ(0, submit_result);
- // Submit a SETTINGS frame.
- adapter->SubmitSettings({});
-
// Submit a WINDOW_UPDATE frame.
adapter->SubmitWindowUpdate(kConnectionStreamId, 42);
@@ -4054,8 +4054,8 @@
EXPECT_TRUE(adapter->want_write());
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index 9635bdf..bb965c2 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -2576,8 +2576,6 @@
EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(GOAWAY, 0, _, 0x0,
@@ -2587,7 +2585,6 @@
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -3120,8 +3117,6 @@
EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, 4, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(RST_STREAM, 1, 4, 0x0,
@@ -3135,9 +3130,7 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS ack
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::RST_STREAM,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -3183,8 +3176,6 @@
EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(GOAWAY, 0, _, 0x0,
@@ -3193,9 +3184,7 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS, SETTINGS ack, and GOAWAY
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -3237,8 +3226,6 @@
EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(GOAWAY, 0, _, 0x0,
@@ -3247,9 +3234,7 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS, SETTINGS ack, and GOAWAY
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -3302,8 +3287,6 @@
EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(GOAWAY, 0, _, 0x0,
@@ -3312,9 +3295,7 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS, SETTINGS ack, and GOAWAY.
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -3360,8 +3341,6 @@
EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0));
// Since the library opted not to process the header, it generates a GOAWAY
// with error code COMPRESSION_ERROR.
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
@@ -3372,9 +3351,8 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS, SETTINGS ack, and GOAWAY.
+ // SETTINGS and GOAWAY.
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -3427,8 +3405,6 @@
EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(GOAWAY, 0, _, 0x0,
@@ -3437,9 +3413,7 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS, SETTINGS ack, and GOAWAY.
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -3557,8 +3531,6 @@
EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(GOAWAY, 0, _, 0x0,
@@ -3567,9 +3539,7 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS, SETTINGS ack, and GOAWAY.
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -3600,10 +3570,8 @@
EXPECT_TRUE(adapter->want_write());
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(GOAWAY, 0, _, 0x0,
@@ -3612,9 +3580,7 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS, SETTINGS ack, and GOAWAY.
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -3647,10 +3613,8 @@
EXPECT_TRUE(adapter->want_write());
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(GOAWAY, 0, _, 0x0,
@@ -3659,9 +3623,7 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS, SETTINGS ack, and GOAWAY.
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -3695,10 +3657,8 @@
EXPECT_TRUE(adapter->want_write());
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(GOAWAY, 0, _, 0x0,
@@ -3707,9 +3667,7 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS, SETTINGS ack, and GOAWAY.
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS,
spdy::SpdyFrameType::GOAWAY}));
}
@@ -4152,6 +4110,9 @@
OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
auto adapter = OgHttp2Adapter::Create(visitor, options);
+ // Submit a custom initial SETTINGS frame with one setting.
+ adapter->SubmitSettings({{HEADER_TABLE_SIZE, 100u}});
+
const std::string frames = TestFrameSequence()
.ClientPreface()
.Headers(1,
@@ -4184,9 +4145,6 @@
1, ToHeaders({{":status", "200"}}), std::move(body));
ASSERT_EQ(0, submit_result);
- // Submit a SETTINGS frame.
- adapter->SubmitSettings({});
-
// Submit a WINDOW_UPDATE frame.
adapter->SubmitWindowUpdate(kConnectionStreamId, 42);
@@ -4212,18 +4170,8 @@
EXPECT_TRUE(adapter->want_write());
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x4));
- EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x4, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 0, _, 0x0));
- EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 0, _, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0));
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
OnFrameSent(GOAWAY, 0, _, 0x0,
@@ -4232,14 +4180,8 @@
int send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // TODO(diannahu): Drop other frames like nghttp2.
- EXPECT_THAT(
- visitor.data(),
- EqualsFrames(
- {spdy::SpdyFrameType::SETTINGS, spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::HEADERS, spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::WINDOW_UPDATE, spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::GOAWAY}));
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::GOAWAY}));
visitor.Clear();
// Try to submit more frames for writing. They should not be written.
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index ec746d4..604b1cb 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -208,6 +208,11 @@
kMaximumHpackTableCapacity);
}
+bool IsNonAckSettings(const spdy::SpdyFrameIR& frame) {
+ return frame.frame_type() == spdy::SpdyFrameType::SETTINGS &&
+ !reinterpret_cast<const SpdySettingsIR&>(frame).is_ack();
+}
+
} // namespace
void OgHttp2Session::PassthroughHeadersHandler::OnHeaderBlockStart() {
@@ -491,8 +496,7 @@
if (frame->frame_type() == spdy::SpdyFrameType::GOAWAY) {
queued_goaway_ = true;
if (latched_error_) {
- // TODO(diannahu): Clear the frames queue.
- queued_immediate_goaway_ = true;
+ PrepareForImmediateGoAway();
}
} else if (frame->fin() ||
frame->frame_type() == spdy::SpdyFrameType::RST_STREAM) {
@@ -660,13 +664,15 @@
return false;
}
if (stream_id == 0) {
- const bool is_settings_ack =
- static_cast<FrameType>(frame_type) == FrameType::SETTINGS &&
- (flags & 0x01);
- if (is_settings_ack && encoder_header_table_capacity_when_acking_) {
- framer_.UpdateHeaderEncoderTableSize(
- encoder_header_table_capacity_when_acking_.value());
- encoder_header_table_capacity_when_acking_ = absl::nullopt;
+ if (static_cast<FrameType>(frame_type) == FrameType::SETTINGS) {
+ const bool is_settings_ack = (flags & 0x01);
+ if (is_settings_ack && encoder_header_table_capacity_when_acking_) {
+ framer_.UpdateHeaderEncoderTableSize(
+ encoder_header_table_capacity_when_acking_.value());
+ encoder_header_table_capacity_when_acking_ = absl::nullopt;
+ } else if (!is_settings_ack) {
+ sent_non_ack_settings_ = true;
+ }
}
return true;
}
@@ -1334,9 +1340,7 @@
spdy::kHttp2ConnectionHeaderPrefixSize);
}
// First frame must be a non-ack SETTINGS.
- if (frames_.empty() ||
- frames_.front()->frame_type() != spdy::SpdyFrameType::SETTINGS ||
- reinterpret_cast<SpdySettingsIR&>(*frames_.front()).is_ack()) {
+ if (frames_.empty() || !IsNonAckSettings(*frames_.front())) {
frames_.push_front(PrepareSettingsFrame(GetInitialSettings()));
}
queued_preface_ = true;
@@ -1540,5 +1544,32 @@
}
}
+void OgHttp2Session::PrepareForImmediateGoAway() {
+ queued_immediate_goaway_ = true;
+
+ // Keep the initial SETTINGS frame if the session has SETTINGS at the front of
+ // the queue but has not sent SETTINGS yet. The session should send initial
+ // SETTINGS before GOAWAY.
+ std::unique_ptr<spdy::SpdyFrameIR> initial_settings;
+ if (!sent_non_ack_settings_ && !frames_.empty() &&
+ IsNonAckSettings(*frames_.front())) {
+ initial_settings = std::move(frames_.front());
+ frames_.pop_front();
+ }
+
+ // Remove all pending frames except for RST_STREAMs. It is important to send
+ // RST_STREAMs so the peer knows of errors below the GOAWAY last stream ID.
+ // TODO(diannahu): Consider informing the visitor of dropped frames. This may
+ // mean keeping the frames and invoking a frame-not-sent callback, similar to
+ // nghttp2. Could add a closure to each frame in the frames queue.
+ frames_.remove_if([](const auto& frame) {
+ return frame->frame_type() != spdy::SpdyFrameType::RST_STREAM;
+ });
+
+ if (initial_settings != nullptr) {
+ frames_.push_front(std::move(initial_settings));
+ }
+}
+
} // namespace adapter
} // namespace http2
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h
index 72e36e9..a3075c0 100644
--- a/http2/adapter/oghttp2_session.h
+++ b/http2/adapter/oghttp2_session.h
@@ -377,6 +377,9 @@
void CloseStreamIfReady(uint8_t frame_type, uint32_t stream_id);
+ // Updates internal state to prepare for sending an immediate GOAWAY.
+ void PrepareForImmediateGoAway();
+
// Receives events when inbound frames are parsed.
Http2VisitorInterface& visitor_;
@@ -474,6 +477,7 @@
bool queued_preface_ = false;
bool peer_supports_metadata_ = false;
bool end_metadata_ = false;
+ bool sent_non_ack_settings_ = false;
// Recursion guard for ProcessBytes().
bool processing_bytes_ = false;
