Allows request trailers in oghttp2. PiperOrigin-RevId: 414560203
diff --git a/http2/adapter/header_validator.cc b/http2/adapter/header_validator.cc index c68ac35..e37b737 100644 --- a/http2/adapter/header_validator.cc +++ b/http2/adapter/header_validator.cc
@@ -29,6 +29,10 @@ return pseudo_headers == *kRequiredHeaders; } +bool ValidateRequestTrailers(const std::vector<std::string>& pseudo_headers) { + return pseudo_headers.empty(); +} + bool ValidateResponseHeaders(const std::vector<std::string>& pseudo_headers) { static const std::vector<std::string>* kRequiredHeaders = new std::vector<std::string>({":status"}); @@ -91,6 +95,8 @@ switch (type) { case HeaderType::REQUEST: return ValidateRequestHeaders(pseudo_headers_); + case HeaderType::REQUEST_TRAILER: + return ValidateRequestTrailers(pseudo_headers_); case HeaderType::RESPONSE_100: case HeaderType::RESPONSE: return ValidateResponseHeaders(pseudo_headers_);
diff --git a/http2/adapter/header_validator.h b/http2/adapter/header_validator.h index dd8ab59..a27fdde 100644 --- a/http2/adapter/header_validator.h +++ b/http2/adapter/header_validator.h
@@ -12,6 +12,7 @@ enum class HeaderType : uint8_t { REQUEST, + REQUEST_TRAILER, RESPONSE_100, RESPONSE, RESPONSE_TRAILER,
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index 23322cb..8b7d694 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -402,6 +402,54 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); } +TEST(NgHttp2AdapterTest, ClientSendsTrailers) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); + + testing::InSequence s; + + const std::vector<const Header> headers1 = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}); + + const std::string kBody = "This is an example request body."; + auto body1 = absl::make_unique<TestDataFrameSource>(visitor, false); + body1->AppendPayload(kBody); + // nghttp2 does not require that the data source indicate the end of data + // before trailers are enqueued. + + const int32_t stream_id1 = + adapter->SubmitRequest(headers1, std::move(body1), nullptr); + ASSERT_GT(stream_id1, 0); + + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x4, 0)); + EXPECT_CALL(visitor, OnFrameSent(DATA, stream_id1, _, 0x0, 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, + spdy::SpdyFrameType::DATA})); + visitor.Clear(); + + const std::vector<const Header> trailers1 = + ToHeaders({{"extra-info", "Trailers are weird but good?"}}); + adapter->SubmitTrailer(stream_id1, trailers1); + + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0)); + + result = adapter->Send(); + EXPECT_EQ(0, result); + data = visitor.data(); + EXPECT_THAT(data, EqualsFrames({spdy::SpdyFrameType::HEADERS})); +} + TEST(NgHttp2AdapterTest, ClientHandlesMetadata) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); @@ -2756,6 +2804,87 @@ absl::StrJoin(visitor.GetMetadata(1), "")); } +TEST(NgHttp2AdapterTest, ServerRespondsToRequestWithTrailers) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); + EXPECT_FALSE(adapter->want_write()); + + const std::string frames = + TestFrameSequence() + .ClientPreface() + .Headers(1, {{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}) + .Data(1, "Example data, woohoo.") + .Serialize(); + + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // Stream 1 + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "GET")); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https")); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com")); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0)); + EXPECT_CALL(visitor, OnBeginDataForStream(1, _)); + EXPECT_CALL(visitor, OnDataForStream(1, _)); + + int64_t result = adapter->ProcessBytes(frames); + EXPECT_EQ(frames.size(), static_cast<size_t>(result)); + + const std::vector<const Header> headers1 = ToHeaders({{":status", "200"}}); + auto body1 = absl::make_unique<TestDataFrameSource>(visitor, true); + TestDataFrameSource* body1_ptr = body1.get(); + + int submit_result = adapter->SubmitResponse(1, headers1, std::move(body1)); + ASSERT_EQ(0, submit_result); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x4, 0)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::HEADERS})); + visitor.Clear(); + + const std::string more_frames = + TestFrameSequence() + .Headers(1, {{"extra-info", "Trailers are weird but good?"}}, + /*fin=*/true) + .Serialize(); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, "extra-info", + "Trailers are weird but good?")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnEndStream(1)); + + result = adapter->ProcessBytes(more_frames); + EXPECT_EQ(more_frames.size(), static_cast<size_t>(result)); + + body1_ptr->EndData(); + EXPECT_EQ(true, adapter->ResumeStream(1)); + + EXPECT_CALL(visitor, OnFrameSent(DATA, 1, 0, 0x1, 0)); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + + send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::DATA})); +} + TEST(NgHttp2AdapterTest, ServerSubmitsResponseWithDataSourceError) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index ea6eaf3..13d79a8 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -418,6 +418,57 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); } +TEST(OgHttp2AdapterClientTest, ClientSendsTrailers) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + testing::InSequence s; + + const std::vector<const Header> headers1 = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}); + + const std::string kBody = "This is an example request body."; + auto body1 = absl::make_unique<TestDataFrameSource>(visitor, false); + body1->AppendPayload(kBody); + body1->EndData(); + + const int32_t stream_id1 = + adapter->SubmitRequest(headers1, std::move(body1), nullptr); + ASSERT_GT(stream_id1, 0); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x4, 0)); + EXPECT_CALL(visitor, OnFrameSent(DATA, stream_id1, _, 0x0, 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, + spdy::SpdyFrameType::DATA})); + visitor.Clear(); + + const std::vector<const Header> trailers1 = + ToHeaders({{"extra-info", "Trailers are weird but good?"}}); + adapter->SubmitTrailer(stream_id1, trailers1); + + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0)); + + result = adapter->Send(); + EXPECT_EQ(0, result); + data = visitor.data(); + EXPECT_THAT(data, EqualsFrames({spdy::SpdyFrameType::HEADERS})); +} + TEST(OgHttp2AdapterClientTest, ClientHandlesMetadata) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; @@ -1955,6 +2006,91 @@ absl::StrJoin(visitor.GetMetadata(1), "")); } +TEST(OgHttp2AdapterServerTest, ServerRespondsToRequestWithTrailers) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kServer}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + EXPECT_FALSE(adapter->want_write()); + + const std::string frames = + TestFrameSequence() + .ClientPreface() + .Headers(1, {{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}) + .Data(1, "Example data, woohoo.") + .Serialize(); + + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // Stream 1 + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "GET")); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https")); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com")); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0)); + EXPECT_CALL(visitor, OnBeginDataForStream(1, _)); + EXPECT_CALL(visitor, OnDataForStream(1, _)); + + int64_t result = adapter->ProcessBytes(frames); + EXPECT_EQ(frames.size(), static_cast<size_t>(result)); + + const std::vector<const Header> headers1 = ToHeaders({{":status", "200"}}); + auto body1 = absl::make_unique<TestDataFrameSource>(visitor, true); + TestDataFrameSource* body1_ptr = body1.get(); + + int submit_result = adapter->SubmitResponse(1, headers1, std::move(body1)); + ASSERT_EQ(0, submit_result); + + 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)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x4, 0)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::HEADERS})); + visitor.Clear(); + + const std::string more_frames = + TestFrameSequence() + .Headers(1, {{"extra-info", "Trailers are weird but good?"}}, + /*fin=*/true) + .Serialize(); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, "extra-info", + "Trailers are weird but good?")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnEndStream(1)); + + result = adapter->ProcessBytes(more_frames); + EXPECT_EQ(more_frames.size(), static_cast<size_t>(result)); + + body1_ptr->EndData(); + EXPECT_EQ(true, adapter->ResumeStream(1)); + + EXPECT_CALL(visitor, OnFrameSent(DATA, 1, 0, 0x1, 0)); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + + send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::DATA})); +} + TEST(OgHttp2AdapterServerTest, ServerSubmitsResponseWithDataSourceError) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index 645a38d..a313755 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -210,6 +210,8 @@ result_ = Http2VisitorInterface::HEADER_OK; const bool status = visitor_.OnBeginHeadersForStream(stream_id_); if (!status) { + QUICHE_VLOG(1) + << "Visitor rejected header block, returning HEADER_CONNECTION_ERROR"; result_ = Http2VisitorInterface::HEADER_CONNECTION_ERROR; } validator_.StartHeaderBlock(); @@ -241,12 +243,14 @@ size_t /* compressed_header_bytes */) { if (result_ == Http2VisitorInterface::HEADER_OK) { if (!validator_.FinishHeaderBlock(type_)) { + QUICHE_VLOG(1) + << "FinishHeaderBlock returned false; returning HEADER_RST_STREAM"; result_ = Http2VisitorInterface::HEADER_RST_STREAM; } } if (frame_contains_fin_ && IsResponse(type_) && StatusIs1xx(status_header())) { - // Unexpected end of stream without final headers. + QUICHE_VLOG(1) << "Unexpected end of stream without final headers"; result_ = Http2VisitorInterface::HEADER_HTTP_MESSAGING; } if (result_ == Http2VisitorInterface::HEADER_OK) { @@ -586,9 +590,14 @@ connection_can_write = SendMetadata(stream_id, state.outbound_metadata); } - if (state.outbound_body == nullptr) { + if (state.outbound_body == nullptr || + (!options_.trailers_require_end_data && state.data_deferred)) { // No data to send, but there might be trailers. if (state.trailers != nullptr) { + // Trailers will include END_STREAM, so the data source can be discarded. + // Since data_deferred is true, there is no data waiting to be flushed for + // this stream. + state.outbound_body = nullptr; auto block_ptr = std::move(state.trailers); if (state.half_closed_local) { QUICHE_LOG(ERROR) << "Sent fin; can't send trailers."; @@ -775,10 +784,7 @@ // Save trailers so they can be written once data is done. state.trailers = absl::make_unique<spdy::SpdyHeaderBlock>(ToHeaderBlock(trailers)); - if (!options_.trailers_require_end_data) { - iter->second.data_deferred = false; - } - if (!iter->second.data_deferred) { + if (!options_.trailers_require_end_data || !iter->second.data_deferred) { write_scheduler_.MarkStreamReady(stream_id, false); } } @@ -1022,6 +1028,10 @@ } if (options_.perspective == Perspective::kServer) { const auto new_stream_id = static_cast<Http2StreamId>(stream_id); + if (stream_map_.find(new_stream_id) != stream_map_.end() && fin) { + // Not a new stream, must be trailers. + return; + } if (new_stream_id <= highest_processed_stream_id_) { // A new stream ID lower than the watermark is a connection error. LatchErrorAndNotify(Http2ErrorCode::PROTOCOL_ERROR, @@ -1341,7 +1351,12 @@ HeaderType OgHttp2Session::NextHeaderType( absl::optional<HeaderType> current_type) { if (IsServerSession()) { - return HeaderType::REQUEST; + if (!current_type) { + return HeaderType::REQUEST; + } else { + QUICHE_DCHECK(current_type == HeaderType::REQUEST); + return HeaderType::REQUEST_TRAILER; + } } else if (!current_type || current_type.value() == HeaderType::RESPONSE_100) { return HeaderType::RESPONSE;