Adds a way to notify the `Http2Adapter` when the underlying codec decides not to send a frame.
This will help prevent a potential leak of `SelfDeletingMetadataSource` objects.
Protected by not protected; does not affect the GFE binary.
PiperOrigin-RevId: 615204887
diff --git a/quiche/http2/adapter/http2_adapter.h b/quiche/http2/adapter/http2_adapter.h
index 0ab0a72..817d4ea 100644
--- a/quiche/http2/adapter/http2_adapter.h
+++ b/quiche/http2/adapter/http2_adapter.h
@@ -143,6 +143,11 @@
// the stream was successfully resumed.
virtual bool ResumeStream(Http2StreamId stream_id) = 0;
+ // Called to communicate that a frame on a stream will not be sent.
+ // TODO(birenroy): remove when removing support for nghttp2.
+ virtual void FrameNotSent(Http2StreamId /*stream_id*/,
+ uint8_t /*frame_type*/) {}
+
protected:
// Subclasses should expose a public factory method for constructing and
// initializing (via Initialize()) adapter instances.
diff --git a/quiche/http2/adapter/nghttp2_adapter.cc b/quiche/http2/adapter/nghttp2_adapter.cc
index f724ea3..ded8307 100644
--- a/quiche/http2/adapter/nghttp2_adapter.cc
+++ b/quiche/http2/adapter/nghttp2_adapter.cc
@@ -19,11 +19,16 @@
using ConnectionError = Http2VisitorInterface::ConnectionError;
-// A metadata source that deletes itself upon completion.
-class SelfDeletingMetadataSource : public MetadataSource {
+} // anonymous namespace
+
+// A metadata source that notifies the owning NgHttp2Adapter upon completion or
+// failure.
+class NgHttp2Adapter::NotifyingMetadataSource : public MetadataSource {
public:
- explicit SelfDeletingMetadataSource(std::unique_ptr<MetadataSource> source)
- : source_(std::move(source)) {}
+ explicit NotifyingMetadataSource(NgHttp2Adapter* adapter,
+ Http2StreamId stream_id,
+ std::unique_ptr<MetadataSource> source)
+ : adapter_(adapter), stream_id_(stream_id), source_(std::move(source)) {}
size_t NumFrames(size_t max_frame_size) const override {
return source_->NumFrames(max_frame_size);
@@ -32,22 +37,22 @@
std::pair<int64_t, bool> Pack(uint8_t* dest, size_t dest_len) override {
const auto result = source_->Pack(dest, dest_len);
if (result.first < 0 || result.second) {
- delete this;
+ adapter_->RemovePendingMetadata(stream_id_);
}
return result;
}
void OnFailure() override {
source_->OnFailure();
- delete this;
+ adapter_->RemovePendingMetadata(stream_id_);
}
private:
+ NgHttp2Adapter* const adapter_;
+ const Http2StreamId stream_id_;
std::unique_ptr<MetadataSource> source_;
};
-} // anonymous namespace
-
/* static */
std::unique_ptr<NgHttp2Adapter> NgHttp2Adapter::CreateClientAdapter(
Http2VisitorInterface& visitor, const nghttp2_option* options) {
@@ -128,13 +133,15 @@
void NgHttp2Adapter::SubmitMetadata(Http2StreamId stream_id,
size_t max_frame_size,
std::unique_ptr<MetadataSource> source) {
- auto* wrapped_source = new SelfDeletingMetadataSource(std::move(source));
+ auto wrapped_source = std::make_unique<NotifyingMetadataSource>(
+ this, stream_id, std::move(source));
const size_t num_frames = wrapped_source->NumFrames(max_frame_size);
size_t num_successes = 0;
for (size_t i = 1; i <= num_frames; ++i) {
- const int result = nghttp2_submit_extension(
- session_->raw_ptr(), kMetadataFrameType,
- i == num_frames ? kMetadataEndFlag : 0, stream_id, wrapped_source);
+ const int result =
+ nghttp2_submit_extension(session_->raw_ptr(), kMetadataFrameType,
+ i == num_frames ? kMetadataEndFlag : 0,
+ stream_id, wrapped_source.get());
if (result != 0) {
QUICHE_LOG(DFATAL) << "Failed to submit extension frame " << i << " of "
<< num_frames;
@@ -142,8 +149,11 @@
}
++num_successes;
}
- if (num_successes == 0) {
- delete wrapped_source;
+ if (num_successes > 0) {
+ // Finds the MetadataSourceVec for `stream_id` or inserts a new one if not
+ // present.
+ auto [it, _] = stream_metadata_.insert({stream_id, MetadataSourceVec{}});
+ it->second.push_back(std::move(wrapped_source));
}
}
@@ -268,6 +278,12 @@
return 0 == nghttp2_session_resume_data(session_->raw_ptr(), stream_id);
}
+void NgHttp2Adapter::FrameNotSent(Http2StreamId stream_id, uint8_t frame_type) {
+ if (frame_type == kMetadataFrameType) {
+ RemovePendingMetadata(stream_id);
+ }
+}
+
void NgHttp2Adapter::RemoveStream(Http2StreamId stream_id) {
sources_.erase(stream_id);
}
@@ -305,5 +321,15 @@
options_ = nullptr;
}
+void NgHttp2Adapter::RemovePendingMetadata(Http2StreamId stream_id) {
+ auto it = stream_metadata_.find(stream_id);
+ if (it != stream_metadata_.end()) {
+ it->second.erase(it->second.begin());
+ if (it->second.empty()) {
+ stream_metadata_.erase(it);
+ }
+ }
+}
+
} // namespace adapter
} // namespace http2
diff --git a/quiche/http2/adapter/nghttp2_adapter.h b/quiche/http2/adapter/nghttp2_adapter.h
index 7ae9645..9337bb9 100644
--- a/quiche/http2/adapter/nghttp2_adapter.h
+++ b/quiche/http2/adapter/nghttp2_adapter.h
@@ -4,6 +4,7 @@
#include <cstdint>
#include "absl/container/flat_hash_map.h"
+#include "absl/container/inlined_vector.h"
#include "quiche/http2/adapter/http2_adapter.h"
#include "quiche/http2/adapter/http2_protocol.h"
#include "quiche/http2/adapter/nghttp2_session.h"
@@ -87,13 +88,25 @@
bool ResumeStream(Http2StreamId stream_id) override;
+ void FrameNotSent(Http2StreamId stream_id, uint8_t frame_type) override;
+
// Removes references to the `stream_id` from this adapter.
void RemoveStream(Http2StreamId stream_id);
// Accessor for testing.
size_t sources_size() const { return sources_.size(); }
+ size_t stream_metadata_size() const { return stream_metadata_.size(); }
+ size_t pending_metadata_count(Http2StreamId stream_id) const {
+ if (auto it = stream_metadata_.find(stream_id);
+ it != stream_metadata_.end()) {
+ return it->second.size();
+ }
+ return 0;
+ }
private:
+ class NotifyingMetadataSource;
+
NgHttp2Adapter(Http2VisitorInterface& visitor, Perspective perspective,
const nghttp2_option* options);
@@ -101,11 +114,18 @@
// such as preparing initial SETTINGS.
void Initialize();
+ void RemovePendingMetadata(Http2StreamId stream_id);
+
std::unique_ptr<NgHttp2Session> session_;
Http2VisitorInterface& visitor_;
const nghttp2_option* options_;
Perspective perspective_;
+ using MetadataSourceVec =
+ absl::InlinedVector<std::unique_ptr<MetadataSource>, 2>;
+ using MetadataMap = absl::flat_hash_map<Http2StreamId, MetadataSourceVec>;
+ MetadataMap stream_metadata_;
+
absl::flat_hash_map<int32_t, std::unique_ptr<DataFrameSource>> sources_;
};
diff --git a/quiche/http2/adapter/nghttp2_adapter_test.cc b/quiche/http2/adapter/nghttp2_adapter_test.cc
index 27770f5..2990190 100644
--- a/quiche/http2/adapter/nghttp2_adapter_test.cc
+++ b/quiche/http2/adapter/nghttp2_adapter_test.cc
@@ -2645,6 +2645,66 @@
EXPECT_FALSE(adapter->want_write());
}
+TEST(NgHttp2AdapterTest, ClientSubmitMetadataWithGoaway) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
+
+ adapter->SubmitSettings({});
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, _, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, _, _, 0x0, 0));
+ adapter->Send();
+
+ const std::vector<Header> headers =
+ ToHeaders({{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}});
+ const int32_t stream_id = adapter->SubmitRequest(headers, nullptr, nullptr);
+
+ auto source = std::make_unique<TestMetadataSource>(ToHeaderBlock(ToHeaders(
+ {{"query-cost", "is too darn high"}, {"secret-sauce", "hollandaise"}})));
+ adapter->SubmitMetadata(stream_id, 16384u, std::move(source));
+ EXPECT_TRUE(adapter->want_write());
+
+ const std::string initial_frames =
+ TestFrameSequence()
+ .ServerPreface()
+ .GoAway(3, Http2ErrorCode::HTTP2_NO_ERROR, "server shutting down")
+ .Serialize();
+ testing::InSequence s;
+
+ // Server preface
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+ EXPECT_CALL(visitor, OnFrameHeader(0, _, GOAWAY, 0));
+ EXPECT_CALL(visitor, OnGoAway(3, Http2ErrorCode::HTTP2_NO_ERROR, _));
+
+ const int64_t initial_result = adapter->ProcessBytes(initial_frames);
+ EXPECT_EQ(initial_frames.size(), initial_result);
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0));
+ // HEADERS frame is not sent.
+ EXPECT_CALL(visitor,
+ OnBeforeFrameSent(kMetadataFrameType, stream_id, _, 0x4));
+ EXPECT_CALL(visitor, OnFrameSent(kMetadataFrameType, stream_id, _, 0x4, 0));
+ EXPECT_CALL(visitor,
+ OnCloseStream(stream_id, Http2ErrorCode::REFUSED_STREAM));
+
+ int result = adapter->Send();
+ EXPECT_EQ(0, result);
+ absl::string_view serialized = visitor.data();
+ EXPECT_THAT(serialized,
+ testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix));
+ serialized.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix));
+ EXPECT_THAT(serialized,
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS,
+ static_cast<SpdyFrameType>(kMetadataFrameType)}));
+ EXPECT_FALSE(adapter->want_write());
+}
+
TEST(NgHttp2AdapterTest, ClientObeysMaxConcurrentStreams) {
DataSavingVisitor visitor;
auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
@@ -6614,6 +6674,84 @@
SpdyFrameType::RST_STREAM}));
}
+TEST(NgHttp2AdapterTest, ServerQueuesMetadataWithStreamReset) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+
+ const std::string initial_frames =
+ TestFrameSequence()
+ .ClientPreface()
+ .Headers(1,
+ {{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}},
+ /*fin=*/false)
+ .Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x4));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4);
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+
+ const int64_t initial_result = adapter->ProcessBytes(initial_frames);
+ EXPECT_EQ(static_cast<size_t>(initial_result), initial_frames.size());
+
+ auto body = std::make_unique<TestDataFrameSource>(visitor, true);
+ body->AppendPayload("Here is some data, which will be completely ignored!");
+
+ int submit_result = adapter->SubmitResponse(
+ 1, ToHeaders({{":status", "200"}}), std::move(body));
+ ASSERT_EQ(0, submit_result);
+
+ auto source = std::make_unique<TestMetadataSource>(ToHeaderBlock(ToHeaders(
+ {{"query-cost", "is too darn high"}, {"secret-sauce", "hollandaise"}})));
+ adapter->SubmitMetadata(1, 16384u, std::move(source));
+ adapter->SubmitWindowUpdate(1, 1024);
+
+ const std::string reset_frame =
+ TestFrameSequence().RstStream(1, Http2ErrorCode::CANCEL).Serialize();
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, RST_STREAM, 0x0));
+ EXPECT_CALL(visitor, OnRstStream(1, Http2ErrorCode::CANCEL));
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::CANCEL));
+ adapter->ProcessBytes(reset_frame);
+
+ source = std::make_unique<TestMetadataSource>(
+ ToHeaderBlock(ToHeaders({{"really-important", "information!"}})));
+ adapter->SubmitMetadata(1, 16384u, std::move(source));
+
+ EXPECT_EQ(1, adapter->stream_metadata_size());
+ EXPECT_EQ(2, adapter->pending_metadata_count(1));
+
+ // Server initial SETTINGS and SETTINGS ack.
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+
+ // nghttp2 apparently allows extension frames to be sent on reset streams.
+ // The response HEADERS, DATA and WINDOW_UPDATE are all discarded.
+ EXPECT_CALL(visitor, OnBeforeFrameSent(kMetadataFrameType, 1, _, 0x4));
+ EXPECT_CALL(visitor, OnFrameSent(kMetadataFrameType, 1, _, 0x4, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(kMetadataFrameType, 1, _, 0x4));
+ EXPECT_CALL(visitor, OnFrameSent(kMetadataFrameType, 1, _, 0x4, 0));
+
+ int send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(),
+ EqualsFrames({SpdyFrameType::SETTINGS,
+ static_cast<SpdyFrameType>(kMetadataFrameType),
+ static_cast<SpdyFrameType>(kMetadataFrameType)}));
+
+ EXPECT_EQ(0, adapter->stream_metadata_size());
+ EXPECT_EQ(0, adapter->pending_metadata_count(1));
+}
+
TEST(NgHttp2AdapterTest, ServerStartsShutdown) {
DataSavingVisitor visitor;
auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
