From 5eba69a1f375413fb93fab4173f9c393ac8c2818 Mon Sep 17 00:00:00 2001 From: antonio Date: Mon, 8 Jun 2020 17:59:54 -0400 Subject: [PATCH] [buffer] Add on-drain hook to buffer API and use it to avoid fragmentation due to tracking of H2 data and control frames in the output buffer (#144) Signed-off-by: antonio --- include/envoy/buffer/buffer.h | 9 + source/common/buffer/buffer_impl.cc | 14 +- source/common/buffer/buffer_impl.h | 21 +- source/common/http/http2/codec_impl.cc | 26 +- source/common/http/http2/codec_impl.h | 8 +- test/common/buffer/buffer_fuzz.cc | 6 + test/common/buffer/owned_impl_test.cc | 353 ++++++++++++++++++++-- test/common/http/http2/codec_impl_test.cc | 10 +- 8 files changed, 390 insertions(+), 57 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index bb2d81259bc8..1f78f380f6b6 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -62,6 +62,15 @@ class Instance { public: virtual ~Instance() = default; + /** + * Register function to call when the last byte in the last slice of this + * buffer has fully drained. Note that slices may be transferred to + * downstream buffers, drain trackers are transferred along with the bytes + * they track so the function is called only after the last byte is drained + * from all buffers. + */ + virtual void addDrainTracker(std::function drain_tracker) PURE; + /** * Copy data into the buffer (deprecated, use absl::string_view variant * instead). diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index c53a51c02bd0..716869fac29b 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -33,6 +33,11 @@ void OwnedImpl::addImpl(const void* data, uint64_t size) { } } +void OwnedImpl::addDrainTracker(std::function drain_tracker) { + ASSERT(!slices_.empty()); + slices_.back()->addDrainTracker(std::move(drain_tracker)); +} + void OwnedImpl::add(const void* data, uint64_t size) { addImpl(data, size); } void OwnedImpl::addBufferFragment(BufferFragment& fragment) { @@ -231,9 +236,11 @@ void* OwnedImpl::linearize(uint32_t size) { auto dest = static_cast(reservation.mem_); do { uint64_t data_size = slices_.front()->dataSize(); - memcpy(dest, slices_.front()->data(), data_size); - bytes_copied += data_size; - dest += data_size; + if (data_size > 0) { + memcpy(dest, slices_.front()->data(), data_size); + bytes_copied += data_size; + dest += data_size; + } slices_.pop_front(); } while (bytes_copied < linearized_size); ASSERT(dest == static_cast(reservation.mem_) + linearized_size); @@ -256,6 +263,7 @@ void OwnedImpl::coalesceOrAddSlice(SlicePtr&& other_slice) { // Copy content of the `other_slice`. The `move` methods which call this method effectively // drain the source buffer. addImpl(other_slice->data(), slice_size); + other_slice->transferDrainTrackersTo(*slices_.back()); } else { // Take ownership of the slice. slices_.emplace_back(std::move(other_slice)); diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index 7da3adb82195..90d76da81d39 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -35,7 +35,11 @@ class Slice { public: using Reservation = RawSlice; - virtual ~Slice() = default; + virtual ~Slice() { + for (const auto& drain_tracker : drain_trackers_) { + drain_tracker(); + } + } /** * @return a pointer to the start of the usable content. @@ -137,6 +141,9 @@ class Slice { */ uint64_t append(const void* data, uint64_t size) { uint64_t copy_size = std::min(size, reservableSize()); + if (copy_size == 0) { + return 0; + } uint8_t* dest = base_ + reservable_; reservable_ += copy_size; // NOLINTNEXTLINE(clang-analyzer-core.NullDereference) @@ -193,6 +200,15 @@ class Slice { return SliceRepresentation{dataSize(), reservableSize(), capacity_}; } + void transferDrainTrackersTo(Slice& destination) { + destination.drain_trackers_.splice(destination.drain_trackers_.end(), drain_trackers_); + ASSERT(drain_trackers_.empty()); + } + + void addDrainTracker(std::function drain_tracker) { + drain_trackers_.emplace_back(std::move(drain_tracker)); + } + protected: Slice(uint64_t data, uint64_t reservable, uint64_t capacity) : data_(data), reservable_(reservable), capacity_(capacity) {} @@ -208,6 +224,8 @@ class Slice { /** Total number of bytes in the slice */ uint64_t capacity_; + + std::list> drain_trackers_; }; using SlicePtr = std::unique_ptr; @@ -510,6 +528,7 @@ class OwnedImpl : public LibEventInstance { OwnedImpl(const void* data, uint64_t size); // Buffer::Instance + void addDrainTracker(std::function drain_tracker) override; void add(const void* data, uint64_t size) override; void addBufferFragment(BufferFragment& fragment) override; void add(absl::string_view data) override; diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index ba8553a3266e..6c56463d4e76 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -499,13 +499,9 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat stream_error_on_invalid_http_messaging_( http2_options.stream_error_on_invalid_http_messaging()), flood_detected_(false), max_outbound_frames_(http2_options.max_outbound_frames().value()), - frame_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { - releaseOutboundFrame(fragment); - }), + frame_buffer_releasor_([this]() { releaseOutboundFrame(); }), max_outbound_control_frames_(http2_options.max_outbound_control_frames().value()), - control_frame_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { - releaseOutboundControlFrame(fragment); - }), + control_frame_buffer_releasor_([this]() { releaseOutboundControlFrame(); }), max_consecutive_inbound_frames_with_empty_payload_( http2_options.max_consecutive_inbound_frames_with_empty_payload().value()), max_inbound_priority_frames_per_stream_( @@ -819,27 +815,21 @@ bool ConnectionImpl::addOutboundFrameFragment(Buffer::OwnedImpl& output, const u return false; } - auto fragment = Buffer::OwnedBufferFragmentImpl::create( - absl::string_view(reinterpret_cast(data), length), - is_outbound_flood_monitored_control_frame ? control_frame_buffer_releasor_ - : frame_buffer_releasor_); - - // The Buffer::OwnedBufferFragmentImpl object will be deleted in the *frame_buffer_releasor_ - // callback. - output.addBufferFragment(*fragment.release()); + output.add(data, length); + output.addDrainTracker(is_outbound_flood_monitored_control_frame ? control_frame_buffer_releasor_ + : frame_buffer_releasor_); return true; } -void ConnectionImpl::releaseOutboundFrame(const Buffer::OwnedBufferFragmentImpl* fragment) { +void ConnectionImpl::releaseOutboundFrame() { ASSERT(outbound_frames_ >= 1); --outbound_frames_; - delete fragment; } -void ConnectionImpl::releaseOutboundControlFrame(const Buffer::OwnedBufferFragmentImpl* fragment) { +void ConnectionImpl::releaseOutboundControlFrame() { ASSERT(outbound_control_frames_ >= 1); --outbound_control_frames_; - releaseOutboundFrame(fragment); + releaseOutboundFrame(); } ssize_t ConnectionImpl::onSend(const uint8_t* data, size_t length) { diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 5751dba9c86d..c977299b0174 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -424,7 +424,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable frame_buffer_releasor_; // This counter keeps track of the number of outbound frames of types PING, SETTINGS and // RST_STREAM (these that were buffered in the underlying connection but not yet written into the // socket). If this counter exceeds the `max_outbound_control_frames_' value the connection is @@ -433,7 +433,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable control_frame_buffer_releasor_; // This counter keeps track of the number of consecutive inbound frames of types HEADERS, // CONTINUATION and DATA with an empty payload and no end stream flag. If this counter exceeds // the `max_consecutive_inbound_frames_with_empty_payload_` value the connection is terminated. @@ -497,8 +497,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable drain_tracker) override { + // Not implemented well. + ASSERT(false); + drain_tracker(); + } + void add(const void* data, uint64_t size) override { FUZZ_ASSERT(start_ + size_ + size <= data_.size()); ::memcpy(mutableEnd(), data, size); diff --git a/test/common/buffer/owned_impl_test.cc b/test/common/buffer/owned_impl_test.cc index 795a4416bc15..d622d6984e43 100644 --- a/test/common/buffer/owned_impl_test.cc +++ b/test/common/buffer/owned_impl_test.cc @@ -37,12 +37,21 @@ class OwnedImplTest : public testing::Test { static void expectSlices(std::vector> buffer_list, OwnedImpl& buffer) { const auto& buffer_slices = buffer.describeSlicesForTest(); + ASSERT_EQ(buffer_list.size(), buffer_slices.size()); for (uint64_t i = 0; i < buffer_slices.size(); i++) { EXPECT_EQ(buffer_slices[i].data, buffer_list[i][0]); EXPECT_EQ(buffer_slices[i].reservable, buffer_list[i][1]); EXPECT_EQ(buffer_slices[i].capacity, buffer_list[i][2]); } } + + static void expectFirstSlice(std::vector slice_description, OwnedImpl& buffer) { + const auto& buffer_slices = buffer.describeSlicesForTest(); + ASSERT_LE(1, buffer_slices.size()); + EXPECT_EQ(buffer_slices[0].data, slice_description[0]); + EXPECT_EQ(buffer_slices[0].reservable, slice_description[1]); + EXPECT_EQ(buffer_slices[0].capacity, slice_description[2]); + } }; TEST_F(OwnedImplTest, AddBufferFragmentNoCleanup) { @@ -80,6 +89,7 @@ TEST_F(OwnedImplTest, AddEmptyFragment) { BufferFragmentImpl frag2("", 0, [this](const void*, size_t, const BufferFragmentImpl*) { release_callback_called_ = true; }); + BufferFragmentImpl frag3(input, 11, [](const void*, size_t, const BufferFragmentImpl*) {}); Buffer::OwnedImpl buffer; buffer.addBufferFragment(frag1); EXPECT_EQ(11, buffer.length()); @@ -87,7 +97,18 @@ TEST_F(OwnedImplTest, AddEmptyFragment) { buffer.addBufferFragment(frag2); EXPECT_EQ(11, buffer.length()); - buffer.drain(11); + buffer.addBufferFragment(frag3); + EXPECT_EQ(22, buffer.length()); + + // Cover case of copying a buffer with an empty fragment. + Buffer::OwnedImpl buffer2; + buffer2.add(buffer); + + // Cover copyOut + std::unique_ptr outbuf(new char[buffer.length()]); + buffer.copyOut(0, buffer.length(), outbuf.get()); + + buffer.drain(22); EXPECT_EQ(0, buffer.length()); EXPECT_TRUE(release_callback_called_); } @@ -326,6 +347,282 @@ TEST_F(OwnedImplTest, Read) { EXPECT_THAT(buffer.describeSlicesForTest(), testing::IsEmpty()); } +TEST_F(OwnedImplTest, DrainTracking) { + testing::InSequence s; + + Buffer::OwnedImpl buffer; + buffer.add("a"); + + testing::MockFunction tracker1; + testing::MockFunction tracker2; + buffer.addDrainTracker(tracker1.AsStdFunction()); + buffer.addDrainTracker(tracker2.AsStdFunction()); + + testing::MockFunction done; + EXPECT_CALL(tracker1, Call()); + EXPECT_CALL(tracker2, Call()); + EXPECT_CALL(done, Call()); + buffer.drain(buffer.length()); + done.Call(); +} + +TEST_F(OwnedImplTest, MoveDrainTrackersWhenTransferingSlices) { + testing::InSequence s; + + Buffer::OwnedImpl buffer1; + buffer1.add("a"); + + testing::MockFunction tracker1; + buffer1.addDrainTracker(tracker1.AsStdFunction()); + + Buffer::OwnedImpl buffer2; + buffer2.add("b"); + + testing::MockFunction tracker2; + buffer2.addDrainTracker(tracker2.AsStdFunction()); + + buffer2.add(std::string(10000, 'c')); + testing::MockFunction tracker3; + buffer2.addDrainTracker(tracker3.AsStdFunction()); + EXPECT_EQ(2, buffer2.getRawSlices().size()); + + buffer1.move(buffer2); + EXPECT_EQ(10002, buffer1.length()); + EXPECT_EQ(0, buffer2.length()); + EXPECT_EQ(3, buffer1.getRawSlices().size()); + EXPECT_EQ(0, buffer2.getRawSlices().size()); + + testing::MockFunction done; + EXPECT_CALL(tracker1, Call()); + EXPECT_CALL(tracker2, Call()); + EXPECT_CALL(tracker3, Call()); + EXPECT_CALL(done, Call()); + buffer1.drain(buffer1.length()); + done.Call(); +} + +TEST_F(OwnedImplTest, MoveDrainTrackersWhenCopying) { + testing::InSequence s; + + Buffer::OwnedImpl buffer1; + buffer1.add("a"); + + testing::MockFunction tracker1; + buffer1.addDrainTracker(tracker1.AsStdFunction()); + + Buffer::OwnedImpl buffer2; + buffer2.add("b"); + + testing::MockFunction tracker2; + buffer2.addDrainTracker(tracker2.AsStdFunction()); + + buffer1.move(buffer2); + EXPECT_EQ(2, buffer1.length()); + EXPECT_EQ(0, buffer2.length()); + EXPECT_EQ(1, buffer1.getRawSlices().size()); + EXPECT_EQ(0, buffer2.getRawSlices().size()); + + buffer1.drain(1); + testing::MockFunction done; + EXPECT_CALL(tracker1, Call()); + EXPECT_CALL(tracker2, Call()); + EXPECT_CALL(done, Call()); + buffer1.drain(1); + done.Call(); +} + +TEST_F(OwnedImplTest, PartialMoveDrainTrackers) { + testing::InSequence s; + + Buffer::OwnedImpl buffer1; + buffer1.add("a"); + + testing::MockFunction tracker1; + buffer1.addDrainTracker(tracker1.AsStdFunction()); + + Buffer::OwnedImpl buffer2; + buffer2.add("b"); + + testing::MockFunction tracker2; + buffer2.addDrainTracker(tracker2.AsStdFunction()); + + buffer2.add(std::string(10000, 'c')); + testing::MockFunction tracker3; + buffer2.addDrainTracker(tracker3.AsStdFunction()); + EXPECT_EQ(2, buffer2.getRawSlices().size()); + + // Move the first slice and associated trackers and part of the second slice to buffer1. + buffer1.move(buffer2, 4999); + EXPECT_EQ(5000, buffer1.length()); + EXPECT_EQ(5002, buffer2.length()); + EXPECT_EQ(3, buffer1.getRawSlices().size()); + EXPECT_EQ(1, buffer2.getRawSlices().size()); + + testing::MockFunction done; + EXPECT_CALL(tracker1, Call()); + buffer1.drain(1); + + EXPECT_CALL(tracker2, Call()); + EXPECT_CALL(done, Call()); + buffer1.drain(buffer1.length()); + done.Call(); + + // tracker3 remained in buffer2. + EXPECT_CALL(tracker3, Call()); + buffer2.drain(buffer2.length()); +} + +TEST_F(OwnedImplTest, DrainTrackingOnDestruction) { + testing::InSequence s; + + auto buffer = std::make_unique(); + buffer->add("a"); + + testing::MockFunction tracker; + buffer->addDrainTracker(tracker.AsStdFunction()); + + testing::MockFunction done; + EXPECT_CALL(tracker, Call()); + EXPECT_CALL(done, Call()); + buffer.reset(); + done.Call(); +} + +TEST_F(OwnedImplTest, Linearize) { + Buffer::OwnedImpl buffer; + + // Unowned slice to track when linearize kicks in. + std::string input(1000, 'a'); + BufferFragmentImpl frag( + input.c_str(), input.size(), + [this](const void*, size_t, const BufferFragmentImpl*) { release_callback_called_ = true; }); + buffer.addBufferFragment(frag); + + // Second slice with more data. + buffer.add(std::string(1000, 'b')); + + // Linearize does not change the pointer associated with the first slice if requested size is less + // than or equal to size of the first slice. + EXPECT_EQ(input.c_str(), buffer.linearize(input.size())); + EXPECT_FALSE(release_callback_called_); + + constexpr uint64_t LinearizeSize = 2000; + void* out_ptr = buffer.linearize(LinearizeSize); + EXPECT_TRUE(release_callback_called_); + EXPECT_EQ(input + std::string(1000, 'b'), + absl::string_view(reinterpret_cast(out_ptr), LinearizeSize)); +} + +TEST_F(OwnedImplTest, LinearizeEmptyBuffer) { + Buffer::OwnedImpl buffer; + EXPECT_EQ(nullptr, buffer.linearize(0)); +} + +TEST_F(OwnedImplTest, LinearizeSingleSlice) { + auto buffer = std::make_unique(); + + // Unowned slice to track when linearize kicks in. + std::string input(1000, 'a'); + BufferFragmentImpl frag( + input.c_str(), input.size(), + [this](const void*, size_t, const BufferFragmentImpl*) { release_callback_called_ = true; }); + buffer->addBufferFragment(frag); + + EXPECT_EQ(input.c_str(), buffer->linearize(buffer->length())); + EXPECT_FALSE(release_callback_called_); + + buffer.reset(); + EXPECT_TRUE(release_callback_called_); +} + +TEST_F(OwnedImplTest, LinearizeDrainTracking) { + constexpr uint32_t SmallChunk = 200; + constexpr uint32_t LargeChunk = 16384 - SmallChunk; + constexpr uint32_t LinearizeSize = SmallChunk + LargeChunk; + + // Create a buffer with a eclectic combination of buffer OwnedSlice and UnownedSlices that will + // help us explore the properties of linearize. + Buffer::OwnedImpl buffer; + + // Large add below the target linearize size. + testing::MockFunction tracker1; + buffer.add(std::string(LargeChunk, 'a')); + buffer.addDrainTracker(tracker1.AsStdFunction()); + + // Unowned slice which causes some fragmentation. + testing::MockFunction tracker2; + testing::MockFunction + release_callback_tracker; + std::string frag_input(2 * SmallChunk, 'b'); + BufferFragmentImpl frag(frag_input.c_str(), frag_input.size(), + release_callback_tracker.AsStdFunction()); + buffer.addBufferFragment(frag); + buffer.addDrainTracker(tracker2.AsStdFunction()); + + // And an unowned slice with 0 size, because. + testing::MockFunction tracker3; + testing::MockFunction + release_callback_tracker2; + BufferFragmentImpl frag2(nullptr, 0, release_callback_tracker2.AsStdFunction()); + buffer.addBufferFragment(frag2); + buffer.addDrainTracker(tracker3.AsStdFunction()); + + // Add a very large chunk + testing::MockFunction tracker4; + buffer.add(std::string(LargeChunk + LinearizeSize, 'c')); + buffer.addDrainTracker(tracker4.AsStdFunction()); + + // Small adds that create no gaps. + testing::MockFunction tracker5; + for (int i = 0; i < 105; ++i) { + buffer.add(std::string(SmallChunk, 'd')); + } + buffer.addDrainTracker(tracker5.AsStdFunction()); + + expectSlices({{16184, 136, 16320}, + {400, 0, 400}, + {0, 0, 0}, + {32704, 0, 32704}, + {4032, 0, 4032}, + {4032, 0, 4032}, + {4032, 0, 4032}, + {4032, 0, 4032}, + {4032, 0, 4032}, + {704, 3328, 4032}}, + buffer); + + testing::InSequence s; + testing::MockFunction drain_tracker; + testing::MockFunction done_tracker; + EXPECT_CALL(tracker1, Call()); + EXPECT_CALL(release_callback_tracker, Call(_, _, _)); + EXPECT_CALL(tracker2, Call()); + EXPECT_CALL(drain_tracker, Call(3 * LargeChunk + 108 * SmallChunk, 16384)); + EXPECT_CALL(release_callback_tracker2, Call(_, _, _)); + EXPECT_CALL(tracker3, Call()); + EXPECT_CALL(tracker4, Call()); + EXPECT_CALL(drain_tracker, Call(2 * LargeChunk + 107 * SmallChunk, 16384)); + EXPECT_CALL(drain_tracker, Call(LargeChunk + 106 * SmallChunk, 16384)); + EXPECT_CALL(drain_tracker, Call(105 * SmallChunk, 16384)); + EXPECT_CALL(tracker5, Call()); + EXPECT_CALL(drain_tracker, Call(4616, 4616)); + EXPECT_CALL(done_tracker, Call()); + for (auto& expected_first_slice : std::vector>{{16584, 3832, 20416}, + {32904, 3896, 36800}, + {16520, 3896, 36800}, + {20296, 120, 20416}, + {4616, 3512, 8128}}) { + const uint32_t write_size = std::min(LinearizeSize, buffer.length()); + buffer.linearize(write_size); + expectFirstSlice(expected_first_slice, buffer); + drain_tracker.Call(buffer.length(), write_size); + buffer.drain(write_size); + } + done_tracker.Call(); + + expectSlices({}, buffer); +} + TEST_F(OwnedImplTest, ReserveCommit) { // This fragment will later be added to the buffer. It is declared in an enclosing scope to // ensure it is not destructed until after the buffer is. @@ -377,12 +674,12 @@ TEST_F(OwnedImplTest, ReserveCommit) { // Request a reservation that too big to fit in the existing slices. This should result // in the creation of a third slice. - expectSlices({{1, 4055, 4056}}, buffer); + expectSlices({{1, 4031, 4032}}, buffer); buffer.reserve(4096 - sizeof(OwnedSlice), iovecs, NumIovecs); - expectSlices({{1, 4055, 4056}, {0, 4056, 4056}}, buffer); + expectSlices({{1, 4031, 4032}, {0, 4032, 4032}}, buffer); const void* slice2 = iovecs[1].mem_; num_reserved = buffer.reserve(8192, iovecs, NumIovecs); - expectSlices({{1, 4055, 4056}, {0, 4056, 4056}, {0, 4056, 4056}}, buffer); + expectSlices({{1, 4031, 4032}, {0, 4032, 4032}, {0, 4032, 4032}}, buffer); EXPECT_EQ(3, num_reserved); EXPECT_EQ(slice1, iovecs[0].mem_); EXPECT_EQ(slice2, iovecs[1].mem_); @@ -391,11 +688,11 @@ TEST_F(OwnedImplTest, ReserveCommit) { // Append a fragment to the buffer, and then request a small reservation. The buffer // should make a new slice to satisfy the reservation; it cannot safely use any of // the previously seen slices, because they are no longer at the end of the buffer. - expectSlices({{1, 4055, 4056}}, buffer); + expectSlices({{1, 4031, 4032}}, buffer); buffer.addBufferFragment(fragment); EXPECT_EQ(13, buffer.length()); num_reserved = buffer.reserve(1, iovecs, NumIovecs); - expectSlices({{1, 4055, 4056}, {12, 0, 12}, {0, 4056, 4056}}, buffer); + expectSlices({{1, 4031, 4032}, {12, 0, 12}, {0, 4032, 4032}}, buffer); EXPECT_EQ(1, num_reserved); EXPECT_NE(slice1, iovecs[0].mem_); commitReservation(iovecs, num_reserved, buffer); @@ -426,16 +723,16 @@ TEST_F(OwnedImplTest, ReserveCommitReuse) { EXPECT_EQ(2, num_reserved); const void* first_slice = iovecs[0].mem_; iovecs[0].len_ = 1; - expectSlices({{8000, 4248, 12248}, {0, 12248, 12248}}, buffer); + expectSlices({{8000, 4224, 12224}, {0, 12224, 12224}}, buffer); buffer.commit(iovecs, 1); EXPECT_EQ(8001, buffer.length()); EXPECT_EQ(first_slice, iovecs[0].mem_); // The second slice is now released because there's nothing in the second slice. - expectSlices({{8001, 4247, 12248}}, buffer); + expectSlices({{8001, 4223, 12224}}, buffer); // Reserve 16KB again. num_reserved = buffer.reserve(16384, iovecs, NumIovecs); - expectSlices({{8001, 4247, 12248}, {0, 12248, 12248}}, buffer); + expectSlices({{8001, 4223, 12224}, {0, 12224, 12224}}, buffer); EXPECT_EQ(2, num_reserved); EXPECT_EQ(static_cast(first_slice) + 1, static_cast(iovecs[0].mem_)); @@ -462,7 +759,7 @@ TEST_F(OwnedImplTest, ReserveReuse) { EXPECT_EQ(2, num_reserved); EXPECT_EQ(first_slice, iovecs[0].mem_); EXPECT_EQ(second_slice, iovecs[1].mem_); - expectSlices({{0, 12248, 12248}, {0, 8152, 8152}}, buffer); + expectSlices({{0, 12224, 12224}, {0, 8128, 8128}}, buffer); // Request a larger reservation, verify that the second entry is replaced with a block with a // larger size. @@ -470,51 +767,51 @@ TEST_F(OwnedImplTest, ReserveReuse) { const void* third_slice = iovecs[1].mem_; EXPECT_EQ(2, num_reserved); EXPECT_EQ(first_slice, iovecs[0].mem_); - EXPECT_EQ(12248, iovecs[0].len_); + EXPECT_EQ(12224, iovecs[0].len_); EXPECT_NE(second_slice, iovecs[1].mem_); EXPECT_EQ(30000 - iovecs[0].len_, iovecs[1].len_); - expectSlices({{0, 12248, 12248}, {0, 8152, 8152}, {0, 20440, 20440}}, buffer); + expectSlices({{0, 12224, 12224}, {0, 8128, 8128}, {0, 20416, 20416}}, buffer); // Repeating a the reservation request for a smaller block returns the previous entry. num_reserved = buffer.reserve(16384, iovecs, NumIovecs); EXPECT_EQ(2, num_reserved); EXPECT_EQ(first_slice, iovecs[0].mem_); EXPECT_EQ(second_slice, iovecs[1].mem_); - expectSlices({{0, 12248, 12248}, {0, 8152, 8152}, {0, 20440, 20440}}, buffer); + expectSlices({{0, 12224, 12224}, {0, 8128, 8128}, {0, 20416, 20416}}, buffer); // Repeat the larger reservation notice that it doesn't match the prior reservation for 30000 // bytes. num_reserved = buffer.reserve(30000, iovecs, NumIovecs); EXPECT_EQ(2, num_reserved); EXPECT_EQ(first_slice, iovecs[0].mem_); - EXPECT_EQ(12248, iovecs[0].len_); + EXPECT_EQ(12224, iovecs[0].len_); EXPECT_NE(second_slice, iovecs[1].mem_); EXPECT_NE(third_slice, iovecs[1].mem_); EXPECT_EQ(30000 - iovecs[0].len_, iovecs[1].len_); - expectSlices({{0, 12248, 12248}, {0, 8152, 8152}, {0, 20440, 20440}, {0, 20440, 20440}}, buffer); + expectSlices({{0, 12224, 12224}, {0, 8128, 8128}, {0, 20416, 20416}, {0, 20416, 20416}}, buffer); // Commit the most recent reservation and verify the representation. buffer.commit(iovecs, num_reserved); - expectSlices({{12248, 0, 12248}, {0, 8152, 8152}, {0, 20440, 20440}, {17752, 2688, 20440}}, + expectSlices({{12224, 0, 12224}, {0, 8128, 8128}, {0, 20416, 20416}, {17776, 2640, 20416}}, buffer); // Do another reservation. num_reserved = buffer.reserve(16384, iovecs, NumIovecs); EXPECT_EQ(2, num_reserved); - expectSlices({{12248, 0, 12248}, - {0, 8152, 8152}, - {0, 20440, 20440}, - {17752, 2688, 20440}, - {0, 16344, 16344}}, + expectSlices({{12224, 0, 12224}, + {0, 8128, 8128}, + {0, 20416, 20416}, + {17776, 2640, 20416}, + {0, 16320, 16320}}, buffer); // And commit. buffer.commit(iovecs, num_reserved); - expectSlices({{12248, 0, 12248}, - {0, 8152, 8152}, - {0, 20440, 20440}, - {20440, 0, 20440}, - {13696, 2648, 16344}}, + expectSlices({{12224, 0, 12224}, + {0, 8128, 8128}, + {0, 20416, 20416}, + {20416, 0, 20416}, + {13744, 2576, 16320}}, buffer); } @@ -671,7 +968,7 @@ TEST_F(OwnedImplTest, ReserveZeroCommit) { ASSERT_EQ(os_sys_calls.close(pipe_fds[1]).rc_, 0); ASSERT_EQ(previous_length, buf.search(data.data(), rc, previous_length)); EXPECT_EQ("bbbbb", buf.toString().substr(0, 5)); - expectSlices({{5, 0, 4056}, {1953, 2103, 4056}}, buf); + expectSlices({{5, 0, 4032}, {1953, 2079, 4032}}, buf); } TEST_F(OwnedImplTest, ReadReserveAndCommit) { @@ -698,7 +995,7 @@ TEST_F(OwnedImplTest, ReadReserveAndCommit) { ASSERT_EQ(result.rc_, static_cast(rc)); ASSERT_EQ(os_sys_calls.close(pipe_fds[1]).rc_, 0); EXPECT_EQ("bbbbbe", buf.toString()); - expectSlices({{6, 4050, 4056}}, buf); + expectSlices({{6, 4026, 4032}}, buf); } TEST(OverflowDetectingUInt64, Arithmetic) { diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 400c4f564ce7..82e842be6efe 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1715,7 +1715,10 @@ TEST_P(Http2CodecImplTest, PingFloodMitigationDisabled) { // Verify that outbound control frame counter decreases when send buffer is drained TEST_P(Http2CodecImplTest, PingFloodCounterReset) { - static const int kMaxOutboundControlFrames = 100; + // Ping frames are 17 bytes each so 237 full frames and a partial frame fit in the current min + // size for buffer slices. Setting the limit to 2x+1 the number that fits in a single slice allows + // the logic below that verifies drain and overflow thresholds. + static const int kMaxOutboundControlFrames = 475; max_outbound_control_frames_ = kMaxOutboundControlFrames; initialize(); @@ -1740,16 +1743,17 @@ TEST_P(Http2CodecImplTest, PingFloodCounterReset) { EXPECT_NO_THROW(client_->sendPendingFrames()); EXPECT_EQ(ack_count, kMaxOutboundControlFrames); - // Drain kMaxOutboundFrames / 2 slices from the send buffer + // Drain floor(kMaxOutboundFrames / 2) slices from the send buffer buffer.drain(buffer.length() / 2); - // Send kMaxOutboundFrames / 2 more pings. + // Send floor(kMaxOutboundFrames / 2) more pings. for (int i = 0; i < kMaxOutboundControlFrames / 2; ++i) { EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); } // The number of outbound frames should be half of max so the connection should not be // terminated. EXPECT_NO_THROW(client_->sendPendingFrames()); + EXPECT_EQ(ack_count, kMaxOutboundControlFrames + kMaxOutboundControlFrames / 2); // 1 more ping frame should overflow the outbound frame limit. EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr));