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