Change QuicSpdyStreamBodyBuffer API to decouple from QuicStreamSequencer.
The primary motivation is cleaner tests.
Before this CL:
* QuicSpdyStreamBodyBuffer keeps QuicStreamSequencer* but only calls a single
method,
* MockSequencer is defined but its mock method is never called,
because QuicStreamSequencer::MarkConsumed() is not virtual,
* a MockStream is needed for QuicStreamSequencer,
* QuicStreamFrames have to be constructed and fed to QuicStreamSequencer so that
it does not crash on MarkConsumed() calls which cannot be mocked, but data is
never actually read from QuicStreamSequencer,
* an HttpEncoder has to be used to create actual frame headers to feed to
QuicStreamSequencer which are then not read,
* MockStream method calls triggered by QuicStreamSequencer::MarkConsumed() are
tested, which only indirectly tests QuicSpdyStreamBodyBuffer behavior.
After this CL:
* QuicSpdyStreamBodyBuffer methods return size_t, caller must call
QuicStreamSequencer::MarkConsumed(),
* no QuicStreamSequencer is needed,
* no MockStream is needed,
* no HttpEncoder is needed (int literals are used for frame header length),
* no QuicStreamFrames are needed,
* QuicSpdyStreamBodyBuffer behavior is directly tested by verifying return value.
gfe-relnote: n/a, no functional change to QUIC v99-only class.
PiperOrigin-RevId: 259526373
Change-Id: I3e67bf924649f0c12bd9706d9ebe13ec4be8e1bb
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index c731741..a8e81c1 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -188,7 +188,6 @@
headers_bytes_to_be_marked_consumed_(0),
http_decoder_visitor_(QuicMakeUnique<HttpDecoderVisitor>(this)),
decoder_(http_decoder_visitor_.get()),
- body_buffer_(sequencer()),
sequencer_offset_(0),
is_decoder_processing_input_(false),
ack_listener_(nullptr) {
@@ -225,7 +224,6 @@
headers_bytes_to_be_marked_consumed_(0),
http_decoder_visitor_(QuicMakeUnique<HttpDecoderVisitor>(this)),
decoder_(http_decoder_visitor_.get()),
- body_buffer_(sequencer()),
sequencer_offset_(sequencer()->NumBytesConsumed()),
is_decoder_processing_input_(false),
ack_listener_(nullptr) {
@@ -397,7 +395,8 @@
if (!VersionHasDataFrameHeader(transport_version())) {
return sequencer()->Readv(iov, iov_len);
}
- size_t bytes_read = body_buffer_.ReadBody(iov, iov_len);
+ 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
@@ -422,7 +421,7 @@
sequencer()->MarkConsumed(num_bytes);
return;
}
- body_buffer_.MarkBodyConsumed(num_bytes);
+ sequencer()->MarkConsumed(body_buffer_.OnBodyConsumed(num_bytes));
if (VersionUsesQpack(transport_version())) {
// Maybe all DATA frame bytes have been read and some trailing HEADERS had
diff --git a/quic/core/http/quic_spdy_stream_body_buffer.cc b/quic/core/http/quic_spdy_stream_body_buffer.cc
index 4238a3b..c29c766 100644
--- a/quic/core/http/quic_spdy_stream_body_buffer.cc
+++ b/quic/core/http/quic_spdy_stream_body_buffer.cc
@@ -6,20 +6,15 @@
#include <algorithm>
-#include "net/third_party/quiche/src/quic/core/quic_stream_sequencer.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_logging.h"
namespace quic {
-QuicSpdyStreamBodyBuffer::QuicSpdyStreamBodyBuffer(
- QuicStreamSequencer* sequencer)
+QuicSpdyStreamBodyBuffer::QuicSpdyStreamBodyBuffer()
: bytes_remaining_(0),
total_body_bytes_readable_(0),
total_body_bytes_received_(0),
- total_payload_lengths_(0),
- sequencer_(sequencer) {}
-
-QuicSpdyStreamBodyBuffer::~QuicSpdyStreamBodyBuffer() {}
+ total_payload_lengths_(0) {}
void QuicSpdyStreamBodyBuffer::OnDataHeader(Http3FrameLengths frame_lengths) {
frame_meta_.push_back(frame_lengths);
@@ -34,21 +29,21 @@
DCHECK_LE(total_body_bytes_received_, total_payload_lengths_);
}
-void QuicSpdyStreamBodyBuffer::MarkBodyConsumed(size_t num_bytes) {
+size_t QuicSpdyStreamBodyBuffer::OnBodyConsumed(size_t num_bytes) {
// Check if the stream has enough decoded data.
if (num_bytes > total_body_bytes_readable_) {
- QUIC_BUG << "Invalid argument to MarkBodyConsumed."
+ QUIC_BUG << "Invalid argument to OnBodyConsumed."
<< " expect to consume: " << num_bytes
<< ", but not enough bytes available. "
<< "Total bytes readable are: " << total_body_bytes_readable_;
- return;
+ return 0;
}
// Discard references in the stream before the sequencer marks them consumed.
size_t remaining = num_bytes;
while (remaining > 0) {
if (bodies_.empty()) {
QUIC_BUG << "Failed to consume because body buffer is empty.";
- return;
+ return 0;
}
auto body = bodies_.front();
bodies_.pop_front();
@@ -60,12 +55,13 @@
remaining = 0;
}
}
+
// Consume headers.
size_t bytes_to_consume = 0;
while (bytes_remaining_ < num_bytes) {
if (frame_meta_.empty()) {
QUIC_BUG << "Faild to consume because frame header buffer is empty.";
- return;
+ return 0;
}
auto meta = frame_meta_.front();
frame_meta_.pop_front();
@@ -73,11 +69,12 @@
bytes_to_consume += meta.header_length;
}
bytes_to_consume += num_bytes;
+
// Update accountings.
bytes_remaining_ -= num_bytes;
total_body_bytes_readable_ -= num_bytes;
- sequencer_->MarkConsumed(bytes_to_consume);
+ return bytes_to_consume;
}
int QuicSpdyStreamBodyBuffer::PeekBody(iovec* iov, size_t iov_len) const {
@@ -101,8 +98,9 @@
}
size_t QuicSpdyStreamBodyBuffer::ReadBody(const struct iovec* iov,
- size_t iov_len) {
- size_t total_data_read = 0;
+ size_t iov_len,
+ size_t* total_bytes_read) {
+ *total_bytes_read = 0;
QuicByteCount total_remaining = total_body_bytes_readable_;
size_t index = 0;
size_t src_offset = 0;
@@ -117,7 +115,7 @@
bytes_to_copy);
dest += bytes_to_copy;
dest_remaining -= bytes_to_copy;
- total_data_read += bytes_to_copy;
+ *total_bytes_read += bytes_to_copy;
total_remaining -= bytes_to_copy;
if (bytes_to_copy < body.length() - src_offset) {
src_offset += bytes_to_copy;
@@ -128,8 +126,7 @@
}
}
- MarkBodyConsumed(total_data_read);
- return total_data_read;
+ return OnBodyConsumed(*total_bytes_read);
}
} // namespace quic
diff --git a/quic/core/http/quic_spdy_stream_body_buffer.h b/quic/core/http/quic_spdy_stream_body_buffer.h
index d8431c1..a37f3bf 100644
--- a/quic/core/http/quic_spdy_stream_body_buffer.h
+++ b/quic/core/http/quic_spdy_stream_body_buffer.h
@@ -10,6 +10,7 @@
#include "net/third_party/quiche/src/quic/platform/api/quic_containers.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_export.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_iovec.h"
+#include "net/third_party/quiche/src/quic/platform/api/quic_macros.h"
namespace quic {
@@ -19,11 +20,8 @@
// consume data.
class QUIC_EXPORT_PRIVATE QuicSpdyStreamBodyBuffer {
public:
- // QuicSpdyStreamBodyBuffer doesn't own the sequencer and the sequencer can
- // outlive the buffer.
- explicit QuicSpdyStreamBodyBuffer(QuicStreamSequencer* sequencer);
-
- ~QuicSpdyStreamBodyBuffer();
+ QuicSpdyStreamBodyBuffer();
+ ~QuicSpdyStreamBodyBuffer() = default;
// Add metadata of the frame to accountings.
// Called when QuicSpdyStream receives data frame header.
@@ -35,9 +33,10 @@
// QuicStreamSequencer::MarkConsumed().
void OnDataPayload(QuicStringPiece payload);
- // Take |num_bytes| as the body size, calculate header sizes accordingly, and
- // consume the right amount of data in the stream sequencer.
- void MarkBodyConsumed(size_t num_bytes);
+ // Internally marks |num_bytes| of DATA frame payload consumed, and returns
+ // the number of bytes that the caller should mark consumed with the
+ // sequencer, including DATA frame header bytes, if any.
+ QUIC_MUST_USE_RESULT size_t OnBodyConsumed(size_t num_bytes);
// Fill up to |iov_len| with bodies available in buffer. No data is consumed.
// |iov|.iov_base will point to data in the buffer, and |iov|.iov_len will
@@ -47,9 +46,11 @@
// Copies from buffer into |iov| up to |iov_len|, and consume data in
// sequencer. |iov.iov_base| and |iov.iov_len| are preassigned and will not be
- // changed.
- // Returns the number of bytes read.
- size_t ReadBody(const struct iovec* iov, size_t iov_len);
+ // changed. |*total_bytes_read| is set to the number of bytes read. Returns
+ // the number of bytes that should be marked consumed with the sequencer.
+ QUIC_MUST_USE_RESULT size_t ReadBody(const struct iovec* iov,
+ size_t iov_len,
+ size_t* total_bytes_read);
bool HasBytesToRead() const { return !bodies_.empty(); }
@@ -70,8 +71,6 @@
QuicByteCount total_body_bytes_received_;
// Total length of payloads tracked by frame_meta_.
QuicByteCount total_payload_lengths_;
- // Stream sequencer that directly manages data in the stream.
- QuicStreamSequencer* sequencer_;
};
} // namespace quic
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 a47880e..d5e8161 100644
--- a/quic/core/http/quic_spdy_stream_body_buffer_test.cc
+++ b/quic/core/http/quic_spdy_stream_body_buffer_test.cc
@@ -6,13 +6,9 @@
#include <string>
-#include "net/third_party/quiche/src/quic/core/quic_stream_sequencer.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_expect_bug.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_logging.h"
-#include "net/third_party/quiche/src/quic/platform/api/quic_ptr_util.h"
-#include "net/third_party/quiche/src/quic/platform/api/quic_str_cat.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_test.h"
-#include "net/third_party/quiche/src/quic/test_tools/quic_test_utils.h"
namespace quic {
@@ -20,51 +16,16 @@
namespace {
-class MockStream : public QuicStreamSequencer::StreamInterface {
- public:
- MOCK_METHOD0(OnFinRead, void());
- MOCK_METHOD0(OnDataAvailable, void());
- MOCK_METHOD2(CloseConnectionWithDetails,
- void(QuicErrorCode error, const std::string& details));
- MOCK_METHOD1(Reset, void(QuicRstStreamErrorCode error));
- MOCK_METHOD0(OnCanWrite, void());
- MOCK_METHOD1(AddBytesConsumed, void(QuicByteCount bytes));
-
- QuicStreamId id() const override { return 1; }
-
- const QuicSocketAddress& PeerAddressOfLatestPacket() const override {
- return peer_address_;
- }
-
- protected:
- QuicSocketAddress peer_address_ =
- QuicSocketAddress(QuicIpAddress::Any4(), 65535);
-};
-
-class MockSequencer : public QuicStreamSequencer {
- public:
- explicit MockSequencer(MockStream* stream) : QuicStreamSequencer(stream) {}
- virtual ~MockSequencer() = default;
- MOCK_METHOD1(MarkConsumed, void(size_t num_bytes_consumed));
-};
-
class QuicSpdyStreamBodyBufferTest : public QuicTest {
- public:
- QuicSpdyStreamBodyBufferTest()
- : sequencer_(&stream_), body_buffer_(&sequencer_) {}
-
protected:
- MockStream stream_;
- MockSequencer sequencer_;
QuicSpdyStreamBodyBuffer body_buffer_;
- HttpEncoder encoder_;
};
TEST_F(QuicSpdyStreamBodyBufferTest, ReceiveBodies) {
std::string body(1024, 'a');
EXPECT_FALSE(body_buffer_.HasBytesToRead());
body_buffer_.OnDataHeader(Http3FrameLengths(3, 1024));
- body_buffer_.OnDataPayload(QuicStringPiece(body));
+ body_buffer_.OnDataPayload(body);
EXPECT_EQ(1024u, body_buffer_.total_body_bytes_received());
EXPECT_TRUE(body_buffer_.HasBytesToRead());
}
@@ -72,7 +33,7 @@
TEST_F(QuicSpdyStreamBodyBufferTest, PeekBody) {
std::string body(1024, 'a');
body_buffer_.OnDataHeader(Http3FrameLengths(3, 1024));
- body_buffer_.OnDataPayload(QuicStringPiece(body));
+ body_buffer_.OnDataPayload(body);
EXPECT_EQ(1024u, body_buffer_.total_body_bytes_received());
iovec vec;
EXPECT_EQ(1, body_buffer_.PeekBody(&vec, 1));
@@ -85,18 +46,11 @@
TEST_F(QuicSpdyStreamBodyBufferTest, MarkConsumedPartialSingleFrame) {
testing::InSequence seq;
std::string body(1024, 'a');
- std::unique_ptr<char[]> buffer;
- QuicByteCount header_length =
- encoder_.SerializeDataFrameHeader(body.length(), &buffer);
- std::string header = std::string(buffer.get(), header_length);
+ const QuicByteCount header_length = 3;
Http3FrameLengths lengths(header_length, 1024);
- std::string data = header + body;
- QuicStreamFrame frame(1, false, 0, data);
- sequencer_.OnStreamFrame(frame);
body_buffer_.OnDataHeader(lengths);
- body_buffer_.OnDataPayload(QuicStringPiece(body));
- EXPECT_CALL(stream_, AddBytesConsumed(header_length + 1024));
- body_buffer_.MarkBodyConsumed(1024);
+ body_buffer_.OnDataPayload(body);
+ EXPECT_EQ(header_length + 1024, body_buffer_.OnBodyConsumed(1024));
}
// Buffer received 2 frames. Stream consumes multiple times.
@@ -104,35 +58,21 @@
testing::InSequence seq;
// 1st frame.
std::string body1(1024, 'a');
- std::unique_ptr<char[]> buffer;
- QuicByteCount header_length1 =
- encoder_.SerializeDataFrameHeader(body1.length(), &buffer);
- std::string header1 = std::string(buffer.get(), header_length1);
+ const QuicByteCount header_length1 = 2;
Http3FrameLengths lengths1(header_length1, 1024);
- std::string data1 = header1 + body1;
- QuicStreamFrame frame1(1, false, 0, data1);
- sequencer_.OnStreamFrame(frame1);
body_buffer_.OnDataHeader(lengths1);
- body_buffer_.OnDataPayload(QuicStringPiece(body1));
+ body_buffer_.OnDataPayload(body1);
// 2nd frame.
std::string body2(2048, 'b');
- QuicByteCount header_length2 =
- encoder_.SerializeDataFrameHeader(body2.length(), &buffer);
- std::string header2 = std::string(buffer.get(), header_length2);
+ const QuicByteCount header_length2 = 4;
Http3FrameLengths lengths2(header_length2, 2048);
- std::string data2 = header2 + body2;
- QuicStreamFrame frame2(1, false, data1.length(), data2);
- sequencer_.OnStreamFrame(frame2);
body_buffer_.OnDataHeader(lengths2);
- body_buffer_.OnDataPayload(QuicStringPiece(body2));
+ body_buffer_.OnDataPayload(body2);
- EXPECT_CALL(stream_, AddBytesConsumed(header_length1 + 512));
- body_buffer_.MarkBodyConsumed(512);
- EXPECT_CALL(stream_, AddBytesConsumed(header_length2 + 2048));
- body_buffer_.MarkBodyConsumed(2048);
- EXPECT_CALL(stream_, AddBytesConsumed(512));
- body_buffer_.MarkBodyConsumed(512);
+ EXPECT_EQ(header_length1 + 512, body_buffer_.OnBodyConsumed(512));
+ EXPECT_EQ(header_length2 + 2048, body_buffer_.OnBodyConsumed(2048));
+ EXPECT_EQ(512u, body_buffer_.OnBodyConsumed(512));
}
TEST_F(QuicSpdyStreamBodyBufferTest, MarkConsumedMoreThanBuffered) {
@@ -140,32 +80,29 @@
Http3FrameLengths lengths(3, 1024);
body_buffer_.OnDataHeader(lengths);
body_buffer_.OnDataPayload(body);
+ size_t bytes_to_consume = 0;
EXPECT_QUIC_BUG(
- body_buffer_.MarkBodyConsumed(2048),
- "Invalid argument to MarkBodyConsumed. expect to consume: 2048, but not "
+ bytes_to_consume = body_buffer_.OnBodyConsumed(2048),
+ "Invalid argument to OnBodyConsumed. expect to consume: 2048, but not "
"enough bytes available. Total bytes readable are: 1024");
+ EXPECT_EQ(0u, bytes_to_consume);
}
// Buffer receives 1 frame. Stream read from the buffer.
TEST_F(QuicSpdyStreamBodyBufferTest, ReadSingleBody) {
testing::InSequence seq;
std::string body(1024, 'a');
- std::unique_ptr<char[]> buffer;
- QuicByteCount header_length =
- encoder_.SerializeDataFrameHeader(body.length(), &buffer);
- std::string header = std::string(buffer.get(), header_length);
+ const QuicByteCount header_length = 2;
Http3FrameLengths lengths(header_length, 1024);
- std::string data = header + body;
- QuicStreamFrame frame(1, false, 0, data);
- sequencer_.OnStreamFrame(frame);
body_buffer_.OnDataHeader(lengths);
- body_buffer_.OnDataPayload(QuicStringPiece(body));
-
- EXPECT_CALL(stream_, AddBytesConsumed(header_length + 1024));
+ body_buffer_.OnDataPayload(body);
char base[1024];
iovec iov = {&base[0], 1024};
- EXPECT_EQ(1024u, body_buffer_.ReadBody(&iov, 1));
+ size_t total_bytes_read = 0;
+ EXPECT_EQ(header_length + 1024,
+ body_buffer_.ReadBody(&iov, 1, &total_bytes_read));
+ EXPECT_EQ(1024u, total_bytes_read);
EXPECT_EQ(1024u, iov.iov_len);
EXPECT_EQ(body,
QuicStringPiece(static_cast<const char*>(iov.iov_base), 1024));
@@ -176,52 +113,44 @@
testing::InSequence seq;
// 1st frame.
std::string body1(1024, 'a');
- std::unique_ptr<char[]> buffer;
- QuicByteCount header_length1 =
- encoder_.SerializeDataFrameHeader(body1.length(), &buffer);
- std::string header1 = std::string(buffer.get(), header_length1);
+ const QuicByteCount header_length1 = 2;
Http3FrameLengths lengths1(header_length1, 1024);
- std::string data1 = header1 + body1;
- QuicStreamFrame frame1(1, false, 0, data1);
- sequencer_.OnStreamFrame(frame1);
body_buffer_.OnDataHeader(lengths1);
- body_buffer_.OnDataPayload(QuicStringPiece(body1));
+ body_buffer_.OnDataPayload(body1);
// 2nd frame.
std::string body2(2048, 'b');
- QuicByteCount header_length2 =
- encoder_.SerializeDataFrameHeader(body2.length(), &buffer);
- std::string header2 = std::string(buffer.get(), header_length2);
+ const QuicByteCount header_length2 = 4;
Http3FrameLengths lengths2(header_length2, 2048);
- std::string data2 = header2 + body2;
- QuicStreamFrame frame2(1, false, data1.length(), data2);
- sequencer_.OnStreamFrame(frame2);
body_buffer_.OnDataHeader(lengths2);
- body_buffer_.OnDataPayload(QuicStringPiece(body2));
+ body_buffer_.OnDataPayload(body2);
// First read of 512 bytes.
- EXPECT_CALL(stream_, AddBytesConsumed(header_length1 + 512));
char base[512];
iovec iov = {&base[0], 512};
- EXPECT_EQ(512u, body_buffer_.ReadBody(&iov, 1));
+ size_t total_bytes_read = 0;
+ EXPECT_EQ(header_length1 + 512,
+ body_buffer_.ReadBody(&iov, 1, &total_bytes_read));
+ EXPECT_EQ(512u, total_bytes_read);
EXPECT_EQ(512u, iov.iov_len);
EXPECT_EQ(body1.substr(0, 512),
QuicStringPiece(static_cast<const char*>(iov.iov_base), 512));
// Second read of 2048 bytes.
- EXPECT_CALL(stream_, AddBytesConsumed(header_length2 + 2048));
char base2[2048];
iovec iov2 = {&base2[0], 2048};
- EXPECT_EQ(2048u, body_buffer_.ReadBody(&iov2, 1));
+ EXPECT_EQ(header_length2 + 2048,
+ body_buffer_.ReadBody(&iov2, 1, &total_bytes_read));
+ EXPECT_EQ(2048u, total_bytes_read);
EXPECT_EQ(2048u, iov2.iov_len);
EXPECT_EQ(body1.substr(512, 512) + body2.substr(0, 1536),
QuicStringPiece(static_cast<const char*>(iov2.iov_base), 2048));
// Third read of the rest 512 bytes.
- EXPECT_CALL(stream_, AddBytesConsumed(512));
char base3[512];
iovec iov3 = {&base3[0], 512};
- EXPECT_EQ(512u, body_buffer_.ReadBody(&iov3, 1));
+ EXPECT_EQ(512u, body_buffer_.ReadBody(&iov3, 1, &total_bytes_read));
+ EXPECT_EQ(512u, total_bytes_read);
EXPECT_EQ(512u, iov3.iov_len);
EXPECT_EQ(body2.substr(1536, 512),
QuicStringPiece(static_cast<const char*>(iov3.iov_base), 512));
