Use QuicheMemSlice constructor with releasor. (envoyproxy#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 <wangsteve@google.com>

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",
     ],
 )
 

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<void(const void*, size_t, const BufferFragmentImpl*)>& releasor)
-      : data_(data), size_(size), releasor_(releasor) {}
+      absl::AnyInvocable<void(const void*, size_t, const BufferFragmentImpl*)> 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<void(const void*, size_t, const BufferFragmentImpl*)> releasor_;
+  absl::AnyInvocable<void(const void*, size_t, const BufferFragmentImpl*)> releasor_;
 };
 
 class LibEventInstance : public Instance {

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<Buffer::OwnedImpl>();
+        single_slice_buffer->move(data, slice.len_);
+        quic_slices.emplace_back(
+            reinterpret_cast<char*>(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<quiche::QuicheMemSlice> span(quic_slices);

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<Buffer::OwnedImpl>();
+        single_slice_buffer->move(data, slice.len_);
+        quic_slices.emplace_back(
+            reinterpret_cast<char*>(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<quiche::QuicheMemSlice> span(quic_slices);

source/common/quic/platform/quiche_mem_slice_impl.cc

@@ -55,7 +55,9 @@ QuicheMemSliceImpl::QuicheMemSliceImpl(std::unique_ptr<char[]> buffer, size_t le
 QuicheMemSliceImpl::QuicheMemSliceImpl(char buffer[], size_t length,
                                        SingleUseCallback<void(const char*)> deleter)
     : fragment_(std::make_unique<Envoy::Buffer::BufferFragmentImpl>(
-          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<const char*>(p));
           })) {
   single_slice_buffer_.addBufferFragment(*fragment_);

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

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",

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

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