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());
