Makes metadata serialization eager within OgHttp2Session. Deferred metadata serialization can result in stream metadata never being sent, if the library queues a stream-closing frame after a call to SubmitMetadata(). PiperOrigin-RevId: 447505380
diff --git a/quiche/http2/adapter/oghttp2_adapter_test.cc b/quiche/http2/adapter/oghttp2_adapter_test.cc index e177bf3..3acba23 100644 --- a/quiche/http2/adapter/oghttp2_adapter_test.cc +++ b/quiche/http2/adapter/oghttp2_adapter_test.cc
@@ -4683,9 +4683,8 @@ ASSERT_EQ(trailer_result, 0); EXPECT_TRUE(adapter->want_write()); - // BUG! The library forgets to serialize and send the metadata. - // 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)); EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x5)); EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x5, 0)); @@ -4694,7 +4693,9 @@ send_result = adapter->Send(); EXPECT_EQ(0, send_result); - EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::HEADERS})); + EXPECT_THAT(visitor.data(), + EqualsFrames({static_cast<SpdyFrameType>(kMetadataFrameType), + SpdyFrameType::HEADERS})); } TEST(OgHttp2AdapterTest, ServerHandlesDataWithPadding) {
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc index aa117a6..07d88ab 100644 --- a/quiche/http2/adapter/oghttp2_session.cc +++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -588,10 +588,6 @@ // case, return early here to avoid sending other frames. return InterpretSendResult(continue_writing); } - while (continue_writing == SendResult::SEND_OK && - !connection_metadata_.empty()) { - continue_writing = SendMetadata(0, connection_metadata_); - } // Notify on new/pending streams closed due to GOAWAY receipt. CloseGoAwayRejectedStreams(); // Wake streams for writes. @@ -618,18 +614,12 @@ } bool OgHttp2Session::HasReadyStream() const { - return !metadata_ready_.empty() || !trailers_ready_.empty() || + return !trailers_ready_.empty() || (write_scheduler_.HasReadyStreams() && connection_send_window_ > 0); } Http2StreamId OgHttp2Session::GetNextReadyStream() { QUICHE_DCHECK(HasReadyStream()); - if (!metadata_ready_.empty()) { - const Http2StreamId stream_id = *metadata_ready_.begin(); - // WriteForStream() will re-mark the stream as ready, if necessary. - write_scheduler_.MarkStreamNotReady(stream_id); - return stream_id; - } if (!trailers_ready_.empty()) { const Http2StreamId stream_id = *trailers_ready_.begin(); // WriteForStream() will re-mark the stream as ready, if necessary. @@ -786,14 +776,10 @@ // HEADERS. state.outbound_body = nullptr; state.trailers = nullptr; - state.outbound_metadata.clear(); return SendResult::SEND_OK; } - SendResult connection_can_write = SendResult::SEND_OK; - if (!state.outbound_metadata.empty()) { - connection_can_write = SendMetadata(stream_id, state.outbound_metadata); - } + SendResult connection_can_write = SendResult::SEND_OK; if (state.outbound_body == nullptr || (!options_.trailers_require_end_data && state.data_deferred)) { // No data to send, but there might be trailers. @@ -900,19 +886,18 @@ : SendResult::SEND_OK; } -OgHttp2Session::SendResult OgHttp2Session::SendMetadata( - Http2StreamId stream_id, OgHttp2Session::MetadataSequence& sequence) { +void OgHttp2Session::SerializeMetadata(Http2StreamId stream_id, + std::unique_ptr<MetadataSource> source) { const uint32_t max_payload_size = std::min(kMaxAllowedMetadataFrameSize, max_frame_payload_); auto payload_buffer = absl::make_unique<uint8_t[]>(max_payload_size); - while (!sequence.empty()) { - MetadataSource& source = *sequence.front(); + while (true) { auto [written, end_metadata] = - source.Pack(payload_buffer.get(), max_payload_size); + source->Pack(payload_buffer.get(), max_payload_size); if (written < 0) { - // Did not touch the connection, so perhaps writes are still possible. - return SendResult::SEND_OK; + // Unable to pack any metadata. + return; } QUICHE_DCHECK_LE(static_cast<size_t>(written), max_payload_size); auto payload = absl::string_view( @@ -921,11 +906,9 @@ stream_id, kMetadataFrameType, end_metadata ? kMetadataEndFlag : 0u, std::string(payload))); if (end_metadata) { - sequence.erase(sequence.begin()); - metadata_ready_.erase(stream_id); + return; } } - return SendQueuedFrames(); } int32_t OgHttp2Session::SubmitRequest( @@ -1003,13 +986,7 @@ void OgHttp2Session::SubmitMetadata(Http2StreamId stream_id, std::unique_ptr<MetadataSource> source) { - if (stream_id == 0) { - connection_metadata_.push_back(std::move(source)); - } else { - auto iter = CreateStream(stream_id); - iter->second.outbound_metadata.push_back(std::move(source)); - metadata_ready_.insert(stream_id); - } + SerializeMetadata(stream_id, std::move(source)); } void OgHttp2Session::SubmitSettings(absl::Span<const Http2Setting> settings) { @@ -1754,7 +1731,6 @@ } stream_map_.erase(stream_id); trailers_ready_.erase(stream_id); - metadata_ready_.erase(stream_id); streams_reset_.erase(stream_id); queued_frames_.erase(stream_id); if (write_scheduler_.StreamRegistered(stream_id)) {
diff --git a/quiche/http2/adapter/oghttp2_session.h b/quiche/http2/adapter/oghttp2_session.h index 916a38f..daca603 100644 --- a/quiche/http2/adapter/oghttp2_session.h +++ b/quiche/http2/adapter/oghttp2_session.h
@@ -145,8 +145,7 @@ } bool want_write() const override { return !fatal_send_error_ && - (!frames_.empty() || !buffered_data_.empty() || - !connection_metadata_.empty() || HasReadyStream() || + (!frames_.empty() || !buffered_data_.empty() || HasReadyStream() || !goaway_rejected_streams_.empty()); } int GetRemoteWindowSize() const override { return connection_send_window_; } @@ -209,8 +208,6 @@ void OnFramePayload(const char* data, size_t len) override; private: - using MetadataSequence = std::vector<std::unique_ptr<MetadataSource>>; - struct QUICHE_EXPORT_PRIVATE StreamState { StreamState(int32_t stream_receive_window, int32_t stream_send_window, WindowManager::WindowUpdateListener listener, @@ -222,7 +219,6 @@ WindowManager window_manager; std::unique_ptr<DataFrameSource> outbound_body; - MetadataSequence outbound_metadata; std::unique_ptr<spdy::SpdyHeaderBlock> trailers; void* user_data = nullptr; int32_t send_window; @@ -359,7 +355,8 @@ // Writes DATA frames for stream `stream_id`. SendResult WriteForStream(Http2StreamId stream_id); - SendResult SendMetadata(Http2StreamId stream_id, MetadataSequence& sequence); + void SerializeMetadata(Http2StreamId stream_id, + std::unique_ptr<MetadataSource> source); void SendHeaders(Http2StreamId stream_id, spdy::SpdyHeaderBlock headers, bool end_stream); @@ -487,13 +484,9 @@ absl::flat_hash_map<Http2StreamId, int> queued_frames_; // Includes streams that are currently ready to write trailers. absl::flat_hash_set<Http2StreamId> trailers_ready_; - // Includes streams that are currently ready to write metadata. - absl::flat_hash_set<Http2StreamId> metadata_ready_; // Includes streams that will not be written due to receipt of GOAWAY. absl::flat_hash_set<Http2StreamId> goaway_rejected_streams_; - MetadataSequence connection_metadata_; - Http2StreamId next_stream_id_ = 1; // The highest received stream ID is the highest stream ID in any frame read // from the peer. The highest processed stream ID is the highest stream ID for