Make QuicSpdyStreamBodyBuffer manage consuming HEADERS frame header and payload. Rename QuicSpdyStreamBodyBuffer::OnDataHeader(), update documentation, and rely on it to calculate when HEADERS frame header and payload bytes need to be consumed (but still call QuicStreamSequencer::MarkConsumed() from QuicSpdyStream). Rip out MaybeMarkHeadersBytesConsumed() and headers_bytes_to_be_marked_consumed_ from QuicSpdyStream, which are hacky and cannot be extended to take care of PUSH_PROMISE frames and unknown frames. gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99. PiperOrigin-RevId: 260059055 Change-Id: I0b457e57e81a12693726a82050dafcefb08e96c8
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc index c577225..ca1345d 100644 --- a/quic/core/http/quic_spdy_stream.cc +++ b/quic/core/http/quic_spdy_stream.cc
@@ -185,7 +185,6 @@ trailers_decompressed_(false), trailers_consumed_(false), priority_sent_(false), - headers_bytes_to_be_marked_consumed_(0), http_decoder_visitor_(QuicMakeUnique<HttpDecoderVisitor>(this)), decoder_(http_decoder_visitor_.get()), sequencer_offset_(0), @@ -221,7 +220,6 @@ trailers_decompressed_(false), trailers_consumed_(false), priority_sent_(false), - headers_bytes_to_be_marked_consumed_(0), http_decoder_visitor_(QuicMakeUnique<HttpDecoderVisitor>(this)), decoder_(http_decoder_visitor_.get()), sequencer_offset_(sequencer()->NumBytesConsumed()), @@ -398,12 +396,6 @@ size_t bytes_read = 0; sequencer()->MarkConsumed(body_buffer_.ReadBody(iov, iov_len, &bytes_read)); - if (VersionUsesQpack(transport_version())) { - // Maybe all DATA frame bytes have been read and some trailing HEADERS had - // already been processed, in which case MarkConsumed() should be called. - MaybeMarkHeadersBytesConsumed(); - } - return bytes_read; } @@ -421,13 +413,8 @@ sequencer()->MarkConsumed(num_bytes); return; } - sequencer()->MarkConsumed(body_buffer_.OnBodyConsumed(num_bytes)); - if (VersionUsesQpack(transport_version())) { - // Maybe all DATA frame bytes have been read and some trailing HEADERS had - // already been processed, in which case MarkConsumed() should be called. - MaybeMarkHeadersBytesConsumed(); - } + sequencer()->MarkConsumed(body_buffer_.OnBodyConsumed(num_bytes)); } bool QuicSpdyStream::IsDoneReading() const { @@ -772,7 +759,7 @@ } sequencer()->MarkConsumed( - body_buffer_.OnDataHeader(frame_lengths.header_length)); + body_buffer_.OnNonBody(frame_lengths.header_length)); return true; } @@ -780,7 +767,7 @@ bool QuicSpdyStream::OnDataFramePayload(QuicStringPiece payload) { DCHECK(VersionHasDataFrameHeader(transport_version())); - body_buffer_.OnDataPayload(payload); + body_buffer_.OnBody(payload); return true; } @@ -827,16 +814,6 @@ } } -void QuicSpdyStream::MaybeMarkHeadersBytesConsumed() { - DCHECK(VersionUsesQpack(transport_version())); - - if (!body_buffer_.HasBytesToRead() && !reading_stopped() && - headers_bytes_to_be_marked_consumed_ > 0) { - sequencer()->MarkConsumed(headers_bytes_to_be_marked_consumed_); - headers_bytes_to_be_marked_consumed_ = 0; - } -} - QuicByteCount QuicSpdyStream::GetNumFrameHeadersInInterval( QuicStreamOffset offset, QuicByteCount data_length) const { @@ -862,6 +839,8 @@ return false; } + sequencer()->MarkConsumed(body_buffer_.OnNonBody(frame_length.header_length)); + if (headers_decompressed_) { trailers_payload_length_ = frame_length.payload_length; } else { @@ -873,10 +852,6 @@ id(), spdy_session_->qpack_decoder(), this, spdy_session_->max_inbound_header_list_size()); - // Do not call MaybeMarkHeadersBytesConsumed() yet, because - // HEADERS frame header bytes might not have been parsed completely. - headers_bytes_to_be_marked_consumed_ += frame_length.header_length; - return true; } @@ -885,8 +860,7 @@ const bool success = qpack_decoded_headers_accumulator_->Decode(payload); - headers_bytes_to_be_marked_consumed_ += payload.size(); - MaybeMarkHeadersBytesConsumed(); + sequencer()->MarkConsumed(body_buffer_.OnNonBody(payload.size())); if (!success) { // TODO(124216424): Use HTTP_QPACK_DECOMPRESSION_FAILED error code.
diff --git a/quic/core/http/quic_spdy_stream.h b/quic/core/http/quic_spdy_stream.h index e030e93..bb111a9 100644 --- a/quic/core/http/quic_spdy_stream.h +++ b/quic/core/http/quic_spdy_stream.h
@@ -259,10 +259,6 @@ // Called internally when headers are decoded. void ProcessDecodedHeaders(const QuicHeaderList& headers); - // Call QuicStreamSequencer::MarkConsumed() with - // |headers_bytes_to_be_marked_consumed_| if appropriate. - void MaybeMarkHeadersBytesConsumed(); - // Given the interval marked by [|offset|, |offset| + |data_length|), return // the number of frame header bytes contained in it. QuicByteCount GetNumFrameHeadersInInterval(QuicStreamOffset offset, @@ -295,9 +291,6 @@ // True if the stream has already sent an priority frame. bool priority_sent_; - // Number of bytes consumed while decoding HEADERS frames that cannot be - // marked consumed in QuicStreamSequencer until later. - QuicByteCount headers_bytes_to_be_marked_consumed_; // The parsed trailers received from the peer. spdy::SpdyHeaderBlock received_trailers_;
diff --git a/quic/core/http/quic_spdy_stream_body_buffer.cc b/quic/core/http/quic_spdy_stream_body_buffer.cc index 2ae72b5..2c54167 100644 --- a/quic/core/http/quic_spdy_stream_body_buffer.cc +++ b/quic/core/http/quic_spdy_stream_body_buffer.cc
@@ -13,25 +13,25 @@ QuicSpdyStreamBodyBuffer::QuicSpdyStreamBodyBuffer() : total_body_bytes_received_(0) {} -size_t QuicSpdyStreamBodyBuffer::OnDataHeader(QuicByteCount length) { +size_t QuicSpdyStreamBodyBuffer::OnNonBody(QuicByteCount length) { DCHECK_NE(0u, length); if (fragments_.empty()) { - // DATA frame header can be consumed immediately, because all previously - // received DATA frame payload bytes have been read. + // Non-body bytes can be consumed immediately, because all previously + // received body bytes have been read. return length; } - // DATA frame header length will be consumed after last fragment is read. - fragments_.back().trailing_consumable_bytes += length; + // Non-body bytes will be consumed after last body fragment is read. + fragments_.back().trailing_non_body_byte_count += length; return 0; } -void QuicSpdyStreamBodyBuffer::OnDataPayload(QuicStringPiece payload) { - DCHECK(!payload.empty()); +void QuicSpdyStreamBodyBuffer::OnBody(QuicStringPiece body) { + DCHECK(!body.empty()); - fragments_.push_back({payload, 0}); - total_body_bytes_received_ += payload.length(); + fragments_.push_back({body, 0}); + total_body_bytes_received_ += body.length(); } size_t QuicSpdyStreamBodyBuffer::OnBodyConsumed(size_t num_bytes) { @@ -55,9 +55,9 @@ } // Consume entire fragment and the following - // |trailing_consumable_bytes| bytes. + // |trailing_non_body_byte_count| bytes. remaining_bytes -= body.length(); - bytes_to_consume += body.length() + fragment.trailing_consumable_bytes; + bytes_to_consume += body.length() + fragment.trailing_non_body_byte_count; fragments_.pop_front(); } @@ -111,7 +111,7 @@ if (bytes_to_copy == body.length()) { // Entire fragment read. - bytes_to_consume += fragment.trailing_consumable_bytes; + bytes_to_consume += fragment.trailing_non_body_byte_count; fragments_.pop_front(); } else { // Consume leading |bytes_to_copy| bytes of body.
diff --git a/quic/core/http/quic_spdy_stream_body_buffer.h b/quic/core/http/quic_spdy_stream_body_buffer.h index 77c17b8..c1a907a 100644 --- a/quic/core/http/quic_spdy_stream_body_buffer.h +++ b/quic/core/http/quic_spdy_stream_body_buffer.h
@@ -15,33 +15,41 @@ namespace quic { -// "Body" means DATA frame payload. +// All data that a request stream receives falls into one of two categories: +// * "body", that is, DATA frame payload, which the QuicStreamSequencer must +// buffer until it is read; +// * everything else, which QuicSpdyStream immediately processes and thus could +// be marked as consumed with QuicStreamSequencer, unless there is some piece +// of body received prior that still needs to be buffered. // QuicSpdyStreamBodyBuffer does two things: it keeps references to body // fragments (owned by QuicStreamSequencer) and offers methods to read them; and -// calculates the total number of bytes (including DATA frame headers) the -// caller needs to mark consumed (with QuicStreamSequencer) whenever DATA frame -// headers are received or body bytes are read. +// it calculates the total number of bytes (including non-body bytes) the caller +// needs to mark consumed (with QuicStreamSequencer) when non-body bytes are +// received or when body is consumed. // TODO(bnc): Rename to QuicSpdyStreamBodyManager or similar. class QUIC_EXPORT_PRIVATE QuicSpdyStreamBodyBuffer { public: QuicSpdyStreamBodyBuffer(); ~QuicSpdyStreamBodyBuffer() = default; - // Called when DATA frame header bytes are received. |length| must be - // positive. Returns number of bytes the caller shall mark consumed, which - // might be zero. - QUIC_MUST_USE_RESULT size_t OnDataHeader(QuicByteCount length); + // One of the following two methods must be called every time data is received + // on the request stream. - // Called when DATA frame payload is received. |payload| is added to - // |fragments_|. The data pointed to by |payload| must be kept alive until an - // OnBodyConsumed() or ReadBody() call consumes it. |payload| must not be - // empty. - void OnDataPayload(QuicStringPiece payload); + // Called when data that could immediately be marked consumed with the + // sequencer (provided that all previous body fragments are consumed) is + // received. |length| must be positive. Returns number of bytes the caller + // must mark consumed, which might be zero. + QUIC_MUST_USE_RESULT size_t OnNonBody(QuicByteCount length); - // Internally marks |num_bytes| of DATA frame payload consumed. |num_bytes| - // might be zero. Returns the number of bytes that the caller should mark - // consumed with the sequencer, which is the sum of |num_bytes| for payload - // and additional DATA frame header bytes, if any. + // Called when body is received. |body| is added to |fragments_|. The data + // pointed to by |body| must be kept alive until an OnBodyConsumed() or + // ReadBody() call consumes it. |body| must not be empty. + void OnBody(QuicStringPiece body); + + // Internally marks |num_bytes| of body consumed. |num_bytes| might be zero. + // Returns the number of bytes that the caller should mark consumed with the + // sequencer, which is the sum of |num_bytes| for body, and the number of any + // interleaving or immediately trailing non-body bytes. QUIC_MUST_USE_RESULT size_t OnBodyConsumed(size_t num_bytes); // Set up to |iov_len| elements of iov[] to point to available bodies: each @@ -51,11 +59,11 @@ int PeekBody(iovec* iov, size_t iov_len) const; // Copies data from available bodies into at most |iov_len| elements of iov[]. - // Internally consumes copied payload bytes as well as all interleaving and - // immediately following DATA frame header bytes. |iov.iov_base| and - // |iov.iov_len| are preassigned and will not be changed. Returns the total - // number of bytes the caller shall mark consumed. Sets - // |*total_bytes_read| to the total number of DATA payload bytes read. + // Internally consumes copied body bytes as well as all interleaving and + // immediately trailing non-body bytes. |iov.iov_base| and |iov.iov_len| are + // preassigned and will not be changed. Returns the total number of bytes the + // caller shall mark consumed. Sets |*total_bytes_read| to the total number + // of body bytes read. QUIC_MUST_USE_RESULT size_t ReadBody(const struct iovec* iov, size_t iov_len, size_t* total_bytes_read); @@ -74,9 +82,9 @@ // |body| must not be empty. QuicStringPiece body; // Might be zero. - QuicByteCount trailing_consumable_bytes; + QuicByteCount trailing_non_body_byte_count; }; - // Queue of DATA frame payload fragments and byte counts. + // Queue of body fragments and trailing non-body byte counts. QuicDeque<Fragment> fragments_; // Total body bytes received. QuicByteCount total_body_bytes_received_;
diff --git a/quic/core/http/quic_spdy_stream_body_buffer_test.cc b/quic/core/http/quic_spdy_stream_body_buffer_test.cc index 7c4787d..ef9dfe2 100644 --- a/quic/core/http/quic_spdy_stream_body_buffer_test.cc +++ b/quic/core/http/quic_spdy_stream_body_buffer_test.cc
@@ -29,13 +29,13 @@ EXPECT_EQ(0u, body_buffer_.total_body_bytes_received()); const QuicByteCount header_length = 3; - EXPECT_EQ(header_length, body_buffer_.OnDataHeader(header_length)); + EXPECT_EQ(header_length, body_buffer_.OnNonBody(header_length)); EXPECT_FALSE(body_buffer_.HasBytesToRead()); EXPECT_EQ(0u, body_buffer_.total_body_bytes_received()); std::string body(1024, 'a'); - body_buffer_.OnDataPayload(body); + body_buffer_.OnBody(body); EXPECT_TRUE(body_buffer_.HasBytesToRead()); EXPECT_EQ(1024u, body_buffer_.total_body_bytes_received()); @@ -43,7 +43,7 @@ TEST_F(QuicSpdyStreamBodyBufferTest, ConsumeMoreThanAvailable) { std::string body(1024, 'a'); - body_buffer_.OnDataPayload(body); + body_buffer_.OnBody(body); size_t bytes_to_consume = 0; EXPECT_QUIC_BUG(bytes_to_consume = body_buffer_.OnBodyConsumed(2048), "Not enough available body to consume."); @@ -91,8 +91,8 @@ // other frames. Each test case start with an empty // QuicSpdyStreamBodyBuffer. EXPECT_EQ(frame_index == 0 ? frame_header_lengths[frame_index] : 0u, - body_buffer_.OnDataHeader(frame_header_lengths[frame_index])); - body_buffer_.OnDataPayload(frame_payloads[frame_index]); + body_buffer_.OnNonBody(frame_header_lengths[frame_index])); + body_buffer_.OnBody(frame_payloads[frame_index]); } for (size_t call_index = 0; call_index < body_bytes_to_read.size(); @@ -141,8 +141,8 @@ // other frames. Each test case uses a new QuicSpdyStreamBodyBuffer // instance. EXPECT_EQ(frame_index == 0 ? frame_header_lengths[frame_index] : 0u, - body_buffer.OnDataHeader(frame_header_lengths[frame_index])); - body_buffer.OnDataPayload(frame_payloads[frame_index]); + body_buffer.OnNonBody(frame_header_lengths[frame_index])); + body_buffer.OnBody(frame_payloads[frame_index]); } std::vector<iovec> iovecs; @@ -238,8 +238,8 @@ // other frames. Each test case uses a new QuicSpdyStreamBodyBuffer // instance. EXPECT_EQ(frame_index == 0 ? frame_header_lengths[frame_index] : 0u, - body_buffer.OnDataHeader(frame_header_lengths[frame_index])); - body_buffer.OnDataPayload(frame_payloads[frame_index]); + body_buffer.OnNonBody(frame_header_lengths[frame_index])); + body_buffer.OnBody(frame_payloads[frame_index]); received_body.append(frame_payloads[frame_index]); }
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc index 20c1b65..5b2ca81 100644 --- a/quic/core/http/quic_spdy_stream_test.cc +++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -2098,8 +2098,7 @@ HeadersFrame(QuicTextUtils::HexDecode("00002a94e703626172")); // All HEADERS frame bytes are consumed even if the frame is not received - // completely (as long as at least some of the payload is received, which is - // an implementation detail that should not be tested). + // completely. OnStreamFrame(QuicStringPiece(headers).substr(0, headers.size() - 1)); EXPECT_EQ(headers.size() - 1, NewlyConsumedBytes());