mobile: Update InternalEngine::sendTrailers to use Envoy::Http::Reque…

…stTrailerMap (envoyproxy#33417)

This PR updates InternalEngine::sendTrailers to use C++ Envoy::Http::RequestTrailerMap instead of envoy_headers C wrapper type. This helps to reduce the number of indirection as well as unnecessary copies. The JNI code for sendTrailers has been rewritten to use proper Java type, such as Map<String, List<String>> instead of byte[][]. This change also helps to reduce unnecessary copies.

Risk Level: low
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: mobile

Signed-off-by: Fredy Wijaya <[email protected]>

mobile/library/cc/bridge_utility.cc

@@ -1,38 +1,10 @@
 #include "bridge_utility.h"
 
-#include <sstream>
-
 #include "library/common/data/utility.h"
 
 namespace Envoy {
 namespace Platform {
 
-envoy_headers rawHeaderMapAsEnvoyHeaders(const RawHeaderMap& headers) {
-  size_t header_count = 0;
-  for (const auto& pair : headers) {
-    header_count += pair.second.size();
-  }
-
-  envoy_map_entry* headers_list =
-      static_cast<envoy_map_entry*>(safe_malloc(sizeof(envoy_map_entry) * header_count));
-
-  size_t i = 0;
-  for (const auto& pair : headers) {
-    const auto& key = pair.first;
-    for (const auto& value : pair.second) {
-      envoy_map_entry& header = headers_list[i++];
-      header.key = Data::Utility::copyToBridgeData(key);
-      header.value = Data::Utility::copyToBridgeData(value);
-    }
-  }
-
-  envoy_headers raw_headers{
-      static_cast<envoy_map_size_t>(header_count),
-      headers_list,
-  };
-  return raw_headers;
-}
-
 RawHeaderMap envoyHeadersAsRawHeaderMap(envoy_headers raw_headers) {
   RawHeaderMap headers;
   for (auto i = 0; i < raw_headers.length; i++) {

mobile/library/cc/bridge_utility.h

@@ -1,15 +1,11 @@
 #pragma once
 
-#include <string>
-#include <vector>
-
 #include "library/cc/headers.h"
 #include "library/common/types/c_types.h"
 
 namespace Envoy {
 namespace Platform {
 
-envoy_headers rawHeaderMapAsEnvoyHeaders(const RawHeaderMap& headers);
 RawHeaderMap envoyHeadersAsRawHeaderMap(envoy_headers raw_headers);
 
 } // namespace Platform

mobile/library/cc/stream.cc

@@ -38,8 +38,13 @@ Stream& Stream::readData(size_t bytes_to_read) {
 }
 
 void Stream::close(RequestTrailersSharedPtr trailers) {
-  envoy_headers raw_headers = rawHeaderMapAsEnvoyHeaders(trailers->allHeaders());
-  engine_->sendTrailers(handle_, raw_headers);
+  auto request_trailer_map = Http::Utility::createRequestTrailerMapPtr();
+  for (const auto& [key, values] : trailers->allHeaders()) {
+    for (const auto& value : values) {
+      request_trailer_map->addCopy(Http::LowerCaseString(key), value);
+    }
+  }
+  engine_->sendTrailers(handle_, std::move(request_trailer_map));
 }
 
 void Stream::close(envoy_data data) { engine_->sendData(handle_, data, true); }

mobile/library/common/http/client.cc

@@ -648,7 +648,7 @@ void Client::sendData(envoy_stream_t stream, envoy_data data, bool end_stream) {
 
 void Client::sendMetadata(envoy_stream_t, envoy_headers) { PANIC("not implemented"); }
 
-void Client::sendTrailers(envoy_stream_t stream, envoy_headers trailers) {
+void Client::sendTrailers(envoy_stream_t stream, RequestTrailerMapPtr trailers) {
   ASSERT(dispatcher_.isThreadSafe());
   Client::DirectStreamSharedPtr direct_stream =
       getStream(stream, GetStreamFilters::ALLOW_ONLY_FOR_OPEN_STREAMS);
@@ -667,9 +667,8 @@ void Client::sendTrailers(envoy_stream_t stream, envoy_headers trailers) {
     return;
   }
   ScopeTrackerScopeState scope(direct_stream.get(), scopeTracker());
-  RequestTrailerMapPtr internal_trailers = Utility::toRequestTrailers(trailers);
-  ENVOY_LOG(debug, "[S{}] request trailers for stream:\n{}", stream, *internal_trailers);
-  request_decoder->decodeTrailers(std::move(internal_trailers));
+  ENVOY_LOG(debug, "[S{}] request trailers for stream:\n{}", stream, *trailers);
+  request_decoder->decodeTrailers(std::move(trailers));
 }
 
 void Client::cancelStream(envoy_stream_t stream) {

mobile/library/common/http/client.h

@@ -74,6 +74,7 @@ class Client : public Logger::Loggable<Logger::Id::http> {
   /**
    * Send headers over an open HTTP stream. This method can be invoked once and needs to be called
    * before send_data.
+   *
    * @param stream the stream to send headers over.
    * @param headers the headers to send.
    * @param end_stream indicates whether to close the stream locally after sending this frame.
@@ -105,10 +106,11 @@ class Client : public Logger::Loggable<Logger::Id::http> {
   /**
    * Send trailers over an open HTTP stream. This method can only be invoked once per stream.
    * Note that this method implicitly closes the stream locally.
-   * @param stream, the stream to send trailers over.
-   * @param trailers, the trailers to send.
+   *
+   * @param stream the stream to send trailers over.
+   * @param trailers the trailers to send.
    */
-  void sendTrailers(envoy_stream_t stream, envoy_headers trailers);
+  void sendTrailers(envoy_stream_t stream, RequestTrailerMapPtr trailers);
 
   /**
    * Reset an open HTTP stream. This operation closes the stream locally, and remote.

mobile/library/common/http/header_utility.cc

@@ -49,17 +49,7 @@ RequestHeaderMapPtr createRequestHeaderMapPtr() {
   return transformed_headers;
 }
 
-RequestTrailerMapPtr toRequestTrailers(envoy_headers trailers) {
-  RequestTrailerMapPtr transformed_trailers = RequestTrailerMapImpl::create();
-  for (envoy_map_size_t i = 0; i < trailers.length; i++) {
-    transformed_trailers->addCopy(
-        LowerCaseString(Data::Utility::copyToString(trailers.entries[i].key)),
-        Data::Utility::copyToString(trailers.entries[i].value));
-  }
-  // The C envoy_headers struct can be released now because the headers have been copied.
-  release_envoy_headers(trailers);
-  return transformed_trailers;
-}
+RequestTrailerMapPtr createRequestTrailerMapPtr() { return RequestTrailerMapImpl::create(); }
 
 envoy_headers toBridgeHeaders(const HeaderMap& header_map, absl::string_view alpn) {
   int alpn_entry = alpn.empty() ? 0 : 1;

mobile/library/common/http/header_utility.h

@@ -37,17 +37,11 @@ void toEnvoyHeaders(HeaderMap& envoy_result_headers, envoy_headers headers);
  */
 RequestHeaderMapPtr toRequestHeaders(envoy_headers headers);
 
-/** Creates an empty `RequestHeaderMapPtr with a preserve case header formatter. */
+/** Creates an empty `RequestHeaderMapPtr` with a preserve case header formatter. */
 RequestHeaderMapPtr createRequestHeaderMapPtr();
 
-/**
- * Transform envoy_headers to RequestHeaderMap.
- * This function copies the content.
- * @param trailers, the envoy_headers (trailers) to transform. headers is free'd. Use after function
- * return is unsafe.
- * @return RequestTrailerMapPtr, the RequestTrailerMap 1:1 transformation of the headers param.
- */
-RequestTrailerMapPtr toRequestTrailers(envoy_headers trailers);
+/** Creates an empty `RequestTrailerMapPtr`. */
+RequestTrailerMapPtr createRequestTrailerMapPtr();
 
 /**
  * Transform envoy_headers to HeaderMap.

mobile/library/common/internal_engine.cc

@@ -79,9 +79,11 @@ envoy_status_t InternalEngine::sendData(envoy_stream_t stream, envoy_data data,
       [&, stream, data, end_stream]() { http_client_->sendData(stream, data, end_stream); });
 }
 
-envoy_status_t InternalEngine::sendTrailers(envoy_stream_t stream, envoy_headers trailers) {
-  return dispatcher_->post(
-      [&, stream, trailers]() { http_client_->sendTrailers(stream, trailers); });
+envoy_status_t InternalEngine::sendTrailers(envoy_stream_t stream,
+                                            Http::RequestTrailerMapPtr trailers) {
+  return dispatcher_->post([&, stream, trailers = std::move(trailers)]() mutable {
+    http_client_->sendTrailers(stream, std::move(trailers));
+  });
 }
 
 envoy_status_t InternalEngine::cancelStream(envoy_stream_t stream) {

mobile/library/common/internal_engine.h

@@ -82,7 +82,14 @@ class InternalEngine : public Logger::Loggable<Logger::Id::main> {
 
   envoy_status_t sendData(envoy_stream_t stream, envoy_data data, bool end_stream);
 
-  envoy_status_t sendTrailers(envoy_stream_t stream, envoy_headers trailers);
+  /**
+   * Send trailers over an open HTTP stream. This method can only be invoked once per stream.
+   * Note that this method implicitly closes the stream locally.
+   *
+   * @param stream the stream to send trailers over.
+   * @param trailers the trailers to send.
+   */
+  envoy_status_t sendTrailers(envoy_stream_t stream, Http::RequestTrailerMapPtr trailers);
 
   envoy_status_t cancelStream(envoy_stream_t stream);
 

mobile/library/java/io/envoyproxy/envoymobile/engine/EnvoyHTTPStream.java

@@ -104,7 +104,7 @@ public void readData(long byteCount) {
    * @param trailers, the trailers to send.
    */
   public void sendTrailers(Map<String, List<String>> trailers) {
-    JniLibrary.sendTrailers(engineHandle, streamHandle, JniBridgeUtility.toJniHeaders(trailers));
+    JniLibrary.sendTrailers(engineHandle, streamHandle, trailers);
   }
 
   /**

mobile/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java

@@ -126,20 +126,21 @@ protected static native int sendData(long engine, long stream, ByteBuffer data,
    * Send trailers over an open HTTP stream. This method can only be invoked once
    * per stream. Note that this method implicitly ends the stream.
    *
-   * @param engine,   the stream's associated engine.
-   * @param stream,   the stream to send trailers over.
-   * @param trailers, the trailers to send.
+   * @param engine   the stream's associated engine.
+   * @param stream   the stream to send trailers over.
+   * @param trailers the trailers to send.
    * @return int, the resulting status of the operation.
    */
-  protected static native int sendTrailers(long engine, long stream, byte[][] trailers);
+  protected static native int sendTrailers(long engine, long stream,
+                                           Map<String, List<String>> trailers);
 
   /**
    * Detach all callbacks from a stream and send an interrupt upstream if
    * supported by transport.
    *
    * @param engine, the stream's associated engine.
    * @param stream, the stream to evict.
-   * @return int, the resulting status of the operation.
+   * @return the resulting status of the operation.
    */
   protected static native int resetStream(long engine, long stream);
 

mobile/library/jni/jni_impl.cc

@@ -992,11 +992,12 @@ extern "C" JNIEXPORT jint JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibra
 }
 
 extern "C" JNIEXPORT jint JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_sendTrailers(
-    JNIEnv* env, jclass, jlong engine_handle, jlong stream_handle, jobjectArray trailers) {
+    JNIEnv* env, jclass, jlong engine_handle, jlong stream_handle, jobject trailers) {
   Envoy::JNI::JniHelper jni_helper(env);
+  auto cpp_trailers = Envoy::Http::Utility::createRequestTrailerMapPtr();
+  Envoy::JNI::javaHeadersToCppHeaders(jni_helper, trailers, *cpp_trailers);
   return reinterpret_cast<Envoy::InternalEngine*>(engine_handle)
-      ->sendTrailers(static_cast<envoy_stream_t>(stream_handle),
-                     Envoy::JNI::javaArrayOfObjectArrayToEnvoyHeaders(jni_helper, trailers));
+      ->sendTrailers(static_cast<envoy_stream_t>(stream_handle), std::move(cpp_trailers));
 }
 
 extern "C" JNIEXPORT jint JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_resetStream(

mobile/library/jni/jni_utility.cc

@@ -426,9 +426,8 @@ absl::flat_hash_map<std::string, std::string> javaMapToCppMap(JniHelper& jni_hel
   return cpp_map;
 }
 
-LocalRefUniquePtr<jobject>
-cppHeadersToJavaHeaders(JniHelper& jni_helper,
-                        const Http::RequestOrResponseHeaderMap& cpp_headers) {
+LocalRefUniquePtr<jobject> cppHeadersToJavaHeaders(JniHelper& jni_helper,
+                                                   const Http::HeaderMap& cpp_headers) {
   auto java_map_class = jni_helper.findClass("java/util/HashMap");
   auto java_map_init_method_id = jni_helper.getMethodId(java_map_class.get(), "<init>", "()V");
   auto java_map_put_method_id = jni_helper.getMethodId(
@@ -475,7 +474,7 @@ cppHeadersToJavaHeaders(JniHelper& jni_helper,
 }
 
 void javaHeadersToCppHeaders(JniHelper& jni_helper, jobject java_headers,
-                             Http::RequestOrResponseHeaderMap& cpp_headers) {
+                             Http::HeaderMap& cpp_headers) {
   auto java_map_class = jni_helper.getObjectClass(java_headers);
   auto java_entry_set_method_id =
       jni_helper.getMethodId(java_map_class.get(), "entrySet", "()Ljava/util/Set;");

mobile/library/jni/jni_utility.h

@@ -166,16 +166,22 @@ absl::flat_hash_map<std::string, std::string> javaMapToCppMap(JniHelper& jni_hel
                                                               jobject java_map);
 
 /**
- * Converts from C++ `RequestOrResponseHeaderMap` to Java `Map<String, List<String>>`.
+ * Converts from C++ `HeaderMap` to Java `Map<String, List<String>>`.
+ *
+ * Both `RequestHeaderMap` and `RequestTrailerMap` inherit from `HeaderMap`. So this function can be
+ * used for converting trailers, too.
  */
-LocalRefUniquePtr<jobject>
-cppHeadersToJavaHeaders(JniHelper& jni_helper, const Http::RequestOrResponseHeaderMap& cpp_headers);
+LocalRefUniquePtr<jobject> cppHeadersToJavaHeaders(JniHelper& jni_helper,
+                                                   const Http::HeaderMap& cpp_headers);
 
 /**
- * Converts from Java `Map<String, List<String>>` to C++ `RequestOrResponseHeaderMap`.
+ * Converts from Java `Map<String, List<String>>` to C++ `HeaderMap`.
+ *
+ * Both `RequestHeaderMap` and `RequestTrailerMap` inherit from `HeaderMap`. So this function can be
+ * used for converting trailers, too.
  */
 void javaHeadersToCppHeaders(JniHelper& jni_helper, jobject java_headers,
-                             Http::RequestOrResponseHeaderMap& cpp_headers);
+                             Http::HeaderMap& cpp_headers);
 
 } // namespace JNI
 } // namespace Envoy

mobile/library/objective-c/EnvoyHTTPStreamImpl.mm

@@ -204,7 +204,17 @@ - (void)readData:(size_t)byteCount {
 }
 
 - (void)sendTrailers:(EnvoyHeaders *)trailers {
-  _engine->sendTrailers(_streamHandle, toNativeHeaders(trailers));
+  Envoy::Http::RequestTrailerMapPtr cppTrailers =
+      Envoy::Http::Utility::createRequestTrailerMapPtr();
+  for (id trailerKey in trailers) {
+    std::string cppTrailerKey = std::string([trailerKey UTF8String]);
+    NSArray *trailerList = trailers[trailerKey];
+    for (NSString *trailerValue in trailerList) {
+      std::string cppTrailerValue = std::string([trailerValue UTF8String]);
+      cppTrailers->addCopy(Envoy::Http::LowerCaseString(cppTrailerKey), cppTrailerValue);
+    }
+  }
+  _engine->sendTrailers(_streamHandle, std::move((cppTrailers)));
 }
 
 - (int)cancel {

mobile/test/common/http/client_test.cc

@@ -246,10 +246,6 @@ TEST_P(ClientTest, BasicStreamTrailers) {
     cc->on_trailers_calls++;
   };
 
-  // Build a set of request trailers.
-  TestRequestTrailerMapImpl trailers;
-  envoy_headers c_trailers = Utility::toBridgeHeaders(trailers);
-
   // Create a stream, and set up request_decoder_ and response_encoder_
   createStream();
 
@@ -258,7 +254,7 @@ TEST_P(ClientTest, BasicStreamTrailers) {
   EXPECT_CALL(dispatcher_, pushTrackedObject(_));
   EXPECT_CALL(dispatcher_, popTrackedObject(_));
   EXPECT_CALL(*request_decoder_, decodeTrailers_(_));
-  http_client_.sendTrailers(stream_, c_trailers);
+  http_client_.sendTrailers(stream_, Utility::createRequestTrailerMapPtr());
   resumeDataIfExplicitFlowControl(20);
 
   // Encode response trailers.

mobile/test/common/http/header_utility_test.cc

@@ -27,15 +27,6 @@ TEST(RequestHeaderDataConstructorTest, FromCToCppEmpty) {
   ASSERT_TRUE(cpp_headers->empty());
 }
 
-TEST(RequestTrailerDataConstructorTest, FromCToCppEmpty) {
-  std::map<std::string, std::string> empty_map;
-  envoy_headers empty_trailers = Bridge::Utility::makeEnvoyMap(empty_map);
-
-  RequestTrailerMapPtr cpp_trailers = Utility::toRequestTrailers(empty_trailers);
-
-  ASSERT_TRUE(cpp_trailers->empty());
-}
-
 TEST(RequestHeaderDataConstructorTest, FromCToCpp) {
   // Backing strings for all the envoy_datas in the c_headers.
   std::vector<std::pair<std::string, std::string>> headers = {
@@ -78,48 +69,6 @@ TEST(RequestHeaderDataConstructorTest, FromCToCpp) {
   delete sentinel;
 }
 
-TEST(RequestTrailerDataConstructorTest, FromCToCpp) {
-  // Backing strings for all the envoy_datas in the c_trailers.
-  std::vector<std::pair<std::string, std::string>> trailers = {
-      {"processing-duration-ms", "25"}, {"response-compression-ratio", "0.61"}};
-
-  envoy_map_entry* header_array =
-      static_cast<envoy_map_entry*>(safe_malloc(sizeof(envoy_map_entry) * trailers.size()));
-
-  uint32_t* sentinel = new uint32_t;
-  *sentinel = 0;
-  for (size_t i = 0; i < trailers.size(); i++) {
-    header_array[i] = {
-        envoyTestString(trailers[i].first, sentinel),
-        envoyTestString(trailers[i].second, sentinel),
-    };
-  }
-
-  envoy_headers c_trailers = {static_cast<envoy_map_size_t>(trailers.size()), header_array};
-  // This copy is used for assertions given that envoy_trailers are released when toRequestTrailers
-  // is called.
-  envoy_headers c_trailers_copy = copy_envoy_headers(c_trailers);
-
-  RequestTrailerMapPtr cpp_trailers = Utility::toRequestTrailers(c_trailers);
-
-  // Check that the sentinel was advance due to c_trailers being released;
-  ASSERT_EQ(*sentinel, 2 * c_trailers_copy.length);
-
-  ASSERT_EQ(cpp_trailers->size(), c_trailers_copy.length);
-
-  for (envoy_map_size_t i = 0; i < c_trailers_copy.length; i++) {
-    LowerCaseString expected_key(Data::Utility::copyToString(c_trailers_copy.entries[i].key));
-    std::string expected_value = Data::Utility::copyToString(c_trailers_copy.entries[i].value);
-
-    // Key is present.
-    EXPECT_FALSE(cpp_trailers->get(expected_key).empty());
-    // Value for the key is the same.
-    EXPECT_EQ(cpp_trailers->get(expected_key)[0]->value().getStringView(), expected_value);
-  }
-  release_envoy_headers(c_trailers_copy);
-  delete sentinel;
-}
-
 TEST(HeaderDataConstructorTest, FromCppToCEmpty) {
   RequestHeaderMapPtr empty_headers = RequestHeaderMapImpl::create();
   envoy_headers c_headers = Utility::toBridgeHeaders(*empty_headers);