Let OgHttp2Session invoke the visitor OnMetadataEndForStream() callback for empty metadata payloads. blaze test //third_party/envoy/src/test/integration:multiplexed_integration_test --test_sharding_strategy=disabled --test_filter=IpVersions/Http2MetadataIntegrationTest.UpstreamSendingEmptyMetadata/IPv6_Http2Downstream_Http2UpstreamWrappedHttp2 http://sponge2/0e30db4a-ede6-4d29-9110-24cb144b78a8 PiperOrigin-RevId: 421049675
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index ced8a82..4e010b1 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -594,6 +594,66 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); } +TEST(NgHttp2AdapterTest, ClientHandlesMetadataWithEmptyPayload) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); + + testing::InSequence s; + + const std::vector<Header> headers1 = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}); + + const int32_t stream_id = adapter->SubmitRequest(headers1, nullptr, nullptr); + ASSERT_GT(stream_id, 0); + + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id, _, 0x5, 0)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + absl::string_view data = visitor.data(); + EXPECT_THAT(data, testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix)); + data.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix)); + EXPECT_THAT(data, EqualsFrames({spdy::SpdyFrameType::HEADERS})); + visitor.Clear(); + + const std::string stream_frames = + TestFrameSequence() + .ServerPreface() + .Headers(1, + {{":status", "200"}, + {"server", "my-fake-server"}, + {"date", "Tue, 6 Apr 2021 12:54:01 GMT"}}, + /*fin=*/false) + .Metadata(1, "") + .Data(1, "This is the response body.", true) + .Serialize(); + + // Server 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, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(3); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, _, kMetadataFrameType, 4)); + EXPECT_CALL(visitor, OnBeginMetadataForStream(1, _)); + EXPECT_CALL(visitor, OnMetadataEndForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 1)); + EXPECT_CALL(visitor, OnBeginDataForStream(1, _)); + EXPECT_CALL(visitor, OnDataForStream(1, "This is the response body.")); + EXPECT_CALL(visitor, OnEndStream(1)); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + + const int64_t stream_result = adapter->ProcessBytes(stream_frames); + EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); +} + TEST(NgHttp2AdapterTest, ClientHandlesMetadataWithError) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 2d87254..e9b863c 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -622,6 +622,70 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); } +TEST(OgHttp2AdapterClientTest, ClientHandlesMetadataWithEmptyPayload) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + testing::InSequence s; + + const std::vector<Header> headers1 = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}); + + const int32_t stream_id = adapter->SubmitRequest(headers1, nullptr, nullptr); + ASSERT_GT(stream_id, 0); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id, _, 0x5, 0)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + absl::string_view data = visitor.data(); + EXPECT_THAT(data, testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix)); + data.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix)); + EXPECT_THAT(data, EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::HEADERS})); + visitor.Clear(); + + const std::string stream_frames = + TestFrameSequence() + .ServerPreface() + .Headers(1, + {{":status", "200"}, + {"server", "my-fake-server"}, + {"date", "Tue, 6 Apr 2021 12:54:01 GMT"}}, + /*fin=*/false) + .Metadata(1, "") + .Data(1, "This is the response body.", true) + .Serialize(); + + // Server 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, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(3); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, _, kMetadataFrameType, 4)); + EXPECT_CALL(visitor, OnBeginMetadataForStream(1, _)); + EXPECT_CALL(visitor, OnMetadataEndForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 1)); + EXPECT_CALL(visitor, OnBeginDataForStream(1, _)); + EXPECT_CALL(visitor, OnDataForStream(1, "This is the response body.")); + EXPECT_CALL(visitor, OnEndStream(1)); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + + const int64_t stream_result = adapter->ProcessBytes(stream_frames); + EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); +} + TEST(OgHttp2AdapterClientTest, ClientHandlesMetadataWithPayloadError) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kClient};
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index 09d3d93..016792a 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -1315,6 +1315,11 @@ metadata_stream_id_ = stream_id; metadata_length_ = length; end_metadata_ = flags & kMetadataEndFlag; + + // Empty metadata payloads will not trigger OnFramePayload(), so handle + // that possibility here. + MaybeHandleMetadataEndForStream(metadata_stream_id_); + return true; } else { QUICHE_DLOG(INFO) << "Unexpected frame type " << static_cast<int>(type) @@ -1330,16 +1335,7 @@ metadata_stream_id_, absl::string_view(data, len)); if (payload_success) { metadata_length_ -= len; - if (metadata_length_ == 0 && end_metadata_) { - const bool completion_success = - visitor_.OnMetadataEndForStream(metadata_stream_id_); - if (!completion_success) { - fatal_visitor_callback_failure_ = true; - decoder_.StopProcessing(); - } - metadata_stream_id_ = 0; - end_metadata_ = false; - } + MaybeHandleMetadataEndForStream(metadata_stream_id_); } else { fatal_visitor_callback_failure_ = true; decoder_.StopProcessing(); @@ -1588,5 +1584,17 @@ } } +void OgHttp2Session::MaybeHandleMetadataEndForStream(Http2StreamId stream_id) { + if (metadata_length_ == 0 && end_metadata_) { + const bool completion_success = visitor_.OnMetadataEndForStream(stream_id); + if (!completion_success) { + fatal_visitor_callback_failure_ = true; + decoder_.StopProcessing(); + } + metadata_stream_id_ = 0; + end_metadata_ = false; + } +} + } // namespace adapter } // namespace http2
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h index a3075c0..97cff20 100644 --- a/http2/adapter/oghttp2_session.h +++ b/http2/adapter/oghttp2_session.h
@@ -380,6 +380,9 @@ // Updates internal state to prepare for sending an immediate GOAWAY. void PrepareForImmediateGoAway(); + // Handles the potential end of received metadata for the given `stream_id`. + void MaybeHandleMetadataEndForStream(Http2StreamId stream_id); + // Receives events when inbound frames are parsed. Http2VisitorInterface& visitor_;