From 83eaa8a69e66a03597026c92f6b378cbee5de4c4 Mon Sep 17 00:00:00 2001 From: Steve Wang <794155+steveWang@users.noreply.github.com> Date: Tue, 12 Dec 2023 18:10:54 -0500 Subject: [PATCH] Use QuicheMemSlice constructor with releasor. (#31167) The existing model of forwarding to the quiche::MemSliceImpl constructor is an abstraction violation and makes it hard to swap out the underlying MemSlice platform implementation. As such, we've recently added a new constructor to QuicheMemSlice (and the Impl API) that allows for an arbitrary custom releasor. This PR uses the new constructor, guarded behind the runtime flag envoy.reloadable_features.quiche_use_mem_slice_releasor_api (disabled by default). Since we're storing a unique_ptr in the capture list (allowable since C++17), we can't store this in a std::function which requires copyability. As such, we use absl::AnyInvocable. (I've marked this as medium risk since it interacts with Envoy's memory management model for QUIC streams and so deserves some scrutiny.) Commit Message: Use QuicheMemSlice constructor with releasor. Additional Description: Risk Level: Medium Testing: This is a refactor, so it should be covered by existing tests (test/common/buffer:buffer_test, test/common/quic:envoy_quic_{client,server}_stream_test) Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Steve Wang --- source/common/buffer/BUILD | 1 + source/common/buffer/buffer_impl.h | 8 +++-- .../common/quic/envoy_quic_client_stream.cc | 14 +++++++- .../common/quic/envoy_quic_server_stream.cc | 14 +++++++- .../quic/platform/quiche_mem_slice_impl.cc | 4 ++- source/common/runtime/runtime_features.cc | 2 ++ test/common/quic/BUILD | 2 ++ .../quic/envoy_quic_client_stream_test.cc | 32 +++++++++++++++++++ .../quic/envoy_quic_server_stream_test.cc | 16 ++++++++++ 9 files changed, 87 insertions(+), 6 deletions(-) diff --git a/source/common/buffer/BUILD b/source/common/buffer/BUILD index b1cd4bb68dfc..0545b3b1558d 100644 --- a/source/common/buffer/BUILD +++ b/source/common/buffer/BUILD @@ -30,6 +30,7 @@ envoy_cc_library( "//source/common/common:non_copyable", "//source/common/common:utility_lib", "//source/common/event:libevent_lib", + "@com_google_absl//absl/functional:any_invocable", ], ) diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index 37a0778facdb..317f4b4a06c1 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -14,6 +14,8 @@ #include "source/common/common/utility.h" #include "source/common/event/libevent.h" +#include "absl/functional/any_invocable.h" + namespace Envoy { namespace Buffer { @@ -604,8 +606,8 @@ class BufferFragmentImpl : NonCopyable, public BufferFragment { */ BufferFragmentImpl( const void* data, size_t size, - const std::function& releasor) - : data_(data), size_(size), releasor_(releasor) {} + absl::AnyInvocable releasor) + : data_(data), size_(size), releasor_(std::move(releasor)) {} // Buffer::BufferFragment const void* data() const override { return data_; } @@ -619,7 +621,7 @@ class BufferFragmentImpl : NonCopyable, public BufferFragment { private: const void* const data_; const size_t size_; - const std::function releasor_; + absl::AnyInvocable releasor_; }; class LibEventInstance : public Instance { diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 4a131edc6538..1c0211e083d4 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -147,7 +147,19 @@ void EnvoyQuicClientStream::encodeData(Buffer::Instance& data, bool end_stream) // TODO(danzh): investigate the cost of allocating one buffer per slice. // If it turns out to be expensive, add a new function to free data in the middle in buffer // interface and re-design QuicheMemSliceImpl. - quic_slices.emplace_back(quiche::QuicheMemSlice::InPlace(), data, slice.len_); + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.quiche_use_mem_slice_releasor_api")) { + quic_slices.emplace_back(quiche::QuicheMemSlice::InPlace(), data, slice.len_); + } else { + auto single_slice_buffer = std::make_unique(); + single_slice_buffer->move(data, slice.len_); + quic_slices.emplace_back( + reinterpret_cast(slice.mem_), slice.len_, + [single_slice_buffer = std::move(single_slice_buffer)](const char*) mutable { + // Free this memory explicitly when the callback is invoked. + single_slice_buffer = nullptr; + }); + } } quic::QuicConsumedData result{0, false}; absl::Span span(quic_slices); diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index a89cef722fab..3ccc044d385b 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -121,7 +121,19 @@ void EnvoyQuicServerStream::encodeData(Buffer::Instance& data, bool end_stream) // TODO(danzh): investigate the cost of allocating one buffer per slice. // If it turns out to be expensive, add a new function to free data in the middle in buffer // interface and re-design QuicheMemSliceImpl. - quic_slices.emplace_back(quiche::QuicheMemSlice::InPlace(), data, slice.len_); + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.quiche_use_mem_slice_releasor_api")) { + quic_slices.emplace_back(quiche::QuicheMemSlice::InPlace(), data, slice.len_); + } else { + auto single_slice_buffer = std::make_unique(); + single_slice_buffer->move(data, slice.len_); + quic_slices.emplace_back( + reinterpret_cast(slice.mem_), slice.len_, + [single_slice_buffer = std::move(single_slice_buffer)](const char*) mutable { + // Free this memory explicitly when the callback is invoked. + single_slice_buffer = nullptr; + }); + } } quic::QuicConsumedData result{0, false}; absl::Span span(quic_slices); diff --git a/source/common/quic/platform/quiche_mem_slice_impl.cc b/source/common/quic/platform/quiche_mem_slice_impl.cc index 357b886c7a5b..44ffc80338a2 100644 --- a/source/common/quic/platform/quiche_mem_slice_impl.cc +++ b/source/common/quic/platform/quiche_mem_slice_impl.cc @@ -55,7 +55,9 @@ QuicheMemSliceImpl::QuicheMemSliceImpl(std::unique_ptr buffer, size_t le QuicheMemSliceImpl::QuicheMemSliceImpl(char buffer[], size_t length, SingleUseCallback deleter) : fragment_(std::make_unique( - buffer, length, [&](const void* p, size_t, const Envoy::Buffer::BufferFragmentImpl*) { + buffer, length, + [deleter = std::move(deleter)](const void* p, size_t, + const Envoy::Buffer::BufferFragmentImpl*) mutable { std::move(deleter)(reinterpret_cast(p)); })) { single_slice_buffer_.addBufferFragment(*fragment_); diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 60a58519b118..0b12a6a73427 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -116,6 +116,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_include_histograms); FALSE_RUNTIME_GUARD(envoy_reloadable_features_refresh_rtt_after_request); // TODO(danzh) false deprecate it once QUICHE has its own enable/disable flag. FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_reject_all); +// TODO(steveWang) flip this to true after this is verified in prod. +FALSE_RUNTIME_GUARD(envoy_reloadable_features_quiche_use_mem_slice_releasor_api); // TODO(suniltheta): Once the newly added http async technique is stabilized move it under // RUNTIME_GUARD so that this option becomes default enabled. Once this option proves effective // remove the feature flag and remove code path that relies on old technique to fetch credentials diff --git a/test/common/quic/BUILD b/test/common/quic/BUILD index 9a7f2426d1c3..e2d38d20e3a8 100644 --- a/test/common/quic/BUILD +++ b/test/common/quic/BUILD @@ -127,6 +127,7 @@ envoy_cc_test( "//test/mocks/http:http_mocks", "//test/mocks/http:stream_decoder_mock", "//test/mocks/network:network_mocks", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@com_github_google_quiche//:quic_core_http_spdy_session_lib", "@com_github_google_quiche//:quic_test_tools_qpack_qpack_test_utils_lib", @@ -148,6 +149,7 @@ envoy_cc_test( "//test/mocks/http:http_mocks", "//test/mocks/http:stream_decoder_mock", "//test/mocks/network:network_mocks", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@com_github_google_quiche//:quic_core_http_spdy_session_lib", "@com_github_google_quiche//:quic_test_tools_qpack_qpack_test_utils_lib", diff --git a/test/common/quic/envoy_quic_client_stream_test.cc b/test/common/quic/envoy_quic_client_stream_test.cc index ef4caa13d87e..996e621ec42c 100644 --- a/test/common/quic/envoy_quic_client_stream_test.cc +++ b/test/common/quic/envoy_quic_client_stream_test.cc @@ -8,6 +8,7 @@ #include "test/mocks/http/mocks.h" #include "test/mocks/http/stream_decoder.h" #include "test/mocks/network/mocks.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -247,6 +248,37 @@ TEST_F(EnvoyQuicClientStreamTest, PostRequestAndResponse) { quic_stream_->OnStreamFrame(frame); } +TEST_F(EnvoyQuicClientStreamTest, PostRequestAndResponseWithMemSliceReleasor) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.quiche_use_mem_slice_releasor_api", "true"}}); + + EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions()); + const auto result = quic_stream_->encodeHeaders(request_headers_, false); + EXPECT_TRUE(result.ok()); + quic_stream_->encodeData(request_body_, false); + quic_stream_->encodeTrailers(request_trailers_); + + size_t offset = receiveResponse(response_body_, false); + EXPECT_CALL(stream_decoder_, decodeTrailers_(_)) + .WillOnce(Invoke([](const Http::ResponseTrailerMapPtr& headers) { + Http::LowerCaseString key1("key1"); + Http::LowerCaseString key2(":final-offset"); + EXPECT_EQ("value1", headers->get(key1)[0]->value().getStringView()); + EXPECT_TRUE(headers->get(key2).empty()); + })); + std::string more_response_body{"bbb"}; + EXPECT_CALL(stream_decoder_, decodeData(_, _)) + .WillOnce(Invoke([&](Buffer::Instance& buffer, bool finished_reading) { + EXPECT_EQ(more_response_body, buffer.toString()); + EXPECT_EQ(false, finished_reading); + })); + std::string payload = absl::StrCat(bodyToHttp3StreamPayload(more_response_body), + spdyHeaderToHttp3StreamPayload(spdy_trailers_)); + quic::QuicStreamFrame frame(stream_id_, true, offset, payload); + quic_stream_->OnStreamFrame(frame); +} + TEST_F(EnvoyQuicClientStreamTest, PostRequestAndResponseWithAccounting) { EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions()); EXPECT_EQ(0, quic_stream_->bytesMeter()->wireBytesSent()); diff --git a/test/common/quic/envoy_quic_server_stream_test.cc b/test/common/quic/envoy_quic_server_stream_test.cc index 883fb61ef174..5f976e56ba57 100644 --- a/test/common/quic/envoy_quic_server_stream_test.cc +++ b/test/common/quic/envoy_quic_server_stream_test.cc @@ -15,6 +15,7 @@ #include "test/mocks/http/mocks.h" #include "test/mocks/http/stream_decoder.h" #include "test/mocks/network/mocks.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/test_time.h" #include "test/test_common/utility.h" @@ -278,6 +279,21 @@ TEST_F(EnvoyQuicServerStreamTest, PostRequestAndResponse) { EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions()); receiveRequest(request_body_, true, request_body_.size() * 2); quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false); + + std::string response(18 * 1024, 'a'); + Buffer::OwnedImpl buffer(response); + quic_stream_->encodeData(buffer, false); + quic_stream_->encodeTrailers(response_trailers_); +} + +TEST_F(EnvoyQuicServerStreamTest, PostRequestAndResponseWithMemSliceReleasor) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.quiche_use_mem_slice_releasor_api", "true"}}); + + EXPECT_EQ(absl::nullopt, quic_stream_->http1StreamEncoderOptions()); + receiveRequest(request_body_, true, request_body_.size() * 2); + quic_stream_->encodeHeaders(response_headers_, /*end_stream=*/false); quic_stream_->encodeTrailers(response_trailers_); }