mobile: refactoring engine handle away (envoyproxy#30668)

Moving the code out of the wrapper class 

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <[email protected]>

mobile/library/common/BUILD

@@ -18,11 +18,9 @@ envoy_cc_library(
     srcs = [
         "engine.cc",
         "engine.h",
-        "engine_handle.cc",
         "main_interface.cc",
     ],
     hdrs = [
-        "engine_handle.h",
         "main_interface.h",
     ],
     repository = "@envoy",

mobile/library/common/main_interface.cc

@@ -7,21 +7,30 @@
 #include "library/common/api/external.h"
 #include "library/common/data/utility.h"
 #include "library/common/engine.h"
-#include "library/common/engine_handle.h"
 #include "library/common/extensions/filters/http/platform_bridge/c_types.h"
 #include "library/common/http/client.h"
 #include "library/common/network/connectivity_manager.h"
 
 // NOLINT(namespace-envoy)
 
+namespace {
+envoy_status_t runOnEngineDispatcher(envoy_engine_t handle,
+                                     std::function<void(Envoy::Engine&)> func) {
+  if (auto engine = reinterpret_cast<Envoy::Engine*>(handle)) {
+    return engine->dispatcher().post([engine, func]() { func(*engine); });
+  }
+  return ENVOY_FAILURE;
+}
+} // namespace
+
 static std::atomic<envoy_stream_t> current_stream_handle_{0};
 
 envoy_stream_t init_stream(envoy_engine_t) { return current_stream_handle_++; }
 
 envoy_status_t start_stream(envoy_engine_t engine, envoy_stream_t stream,
                             envoy_http_callbacks callbacks, bool explicit_flow_control,
                             uint64_t min_delivery_size) {
-  return Envoy::EngineHandle::runOnEngineDispatcher(
+  return runOnEngineDispatcher(
       engine, [stream, callbacks, explicit_flow_control, min_delivery_size](auto& engine) -> void {
         engine.httpClient().startStream(stream, callbacks, explicit_flow_control,
                                         min_delivery_size);
@@ -30,25 +39,22 @@ envoy_status_t start_stream(envoy_engine_t engine, envoy_stream_t stream,
 
 envoy_status_t send_headers(envoy_engine_t engine, envoy_stream_t stream, envoy_headers headers,
                             bool end_stream) {
-  return Envoy::EngineHandle::runOnEngineDispatcher(
-      engine, ([stream, headers, end_stream](auto& engine) -> void {
-        engine.httpClient().sendHeaders(stream, headers, end_stream);
-      }));
+  return runOnEngineDispatcher(engine, ([stream, headers, end_stream](auto& engine) -> void {
+                                 engine.httpClient().sendHeaders(stream, headers, end_stream);
+                               }));
 }
 
 envoy_status_t read_data(envoy_engine_t engine, envoy_stream_t stream, size_t bytes_to_read) {
-  return Envoy::EngineHandle::runOnEngineDispatcher(
-      engine, [stream, bytes_to_read](auto& engine) -> void {
-        engine.httpClient().readData(stream, bytes_to_read);
-      });
+  return runOnEngineDispatcher(engine, [stream, bytes_to_read](auto& engine) -> void {
+    engine.httpClient().readData(stream, bytes_to_read);
+  });
 }
 
 envoy_status_t send_data(envoy_engine_t engine, envoy_stream_t stream, envoy_data data,
                          bool end_stream) {
-  return Envoy::EngineHandle::runOnEngineDispatcher(
-      engine, [stream, data, end_stream](auto& engine) -> void {
-        engine.httpClient().sendData(stream, data, end_stream);
-      });
+  return runOnEngineDispatcher(engine, [stream, data, end_stream](auto& engine) -> void {
+    engine.httpClient().sendData(stream, data, end_stream);
+  });
 }
 
 // TODO: implement.
@@ -57,58 +63,57 @@ envoy_status_t send_metadata(envoy_engine_t, envoy_stream_t, envoy_headers) {
 }
 
 envoy_status_t send_trailers(envoy_engine_t engine, envoy_stream_t stream, envoy_headers trailers) {
-  return Envoy::EngineHandle::runOnEngineDispatcher(
-      engine, [stream, trailers](auto& engine) -> void {
-        engine.httpClient().sendTrailers(stream, trailers);
-      });
+  return runOnEngineDispatcher(engine, [stream, trailers](auto& engine) -> void {
+    engine.httpClient().sendTrailers(stream, trailers);
+  });
 }
 
 envoy_status_t reset_stream(envoy_engine_t engine, envoy_stream_t stream) {
-  return Envoy::EngineHandle::runOnEngineDispatcher(
+  return runOnEngineDispatcher(
       engine, [stream](auto& engine) -> void { engine.httpClient().cancelStream(stream); });
 }
 
 envoy_status_t set_preferred_network(envoy_engine_t engine, envoy_network_t network) {
   envoy_netconf_t configuration_key =
       Envoy::Network::ConnectivityManagerImpl::setPreferredNetwork(network);
-  Envoy::EngineHandle::runOnEngineDispatcher(engine, [configuration_key](auto& engine) -> void {
+  runOnEngineDispatcher(engine, [configuration_key](auto& engine) -> void {
     engine.networkConnectivityManager().refreshDns(configuration_key, true);
   });
   // TODO(snowp): Should this return failure ever?
   return ENVOY_SUCCESS;
 }
 
 envoy_status_t set_proxy_settings(envoy_engine_t e, const char* host, const uint16_t port) {
-  return Envoy::EngineHandle::runOnEngineDispatcher(
+  return runOnEngineDispatcher(
       e,
       [proxy_settings = Envoy::Network::ProxySettings::parseHostAndPort(host, port)](auto& engine)
           -> void { engine.networkConnectivityManager().setProxySettings(proxy_settings); });
 }
 
 envoy_status_t record_counter_inc(envoy_engine_t e, const char* elements, envoy_stats_tags tags,
                                   uint64_t count) {
-  return Envoy::EngineHandle::runOnEngineDispatcher(
-      e, [name = std::string(elements), tags, count](auto& engine) -> void {
-        engine.recordCounterInc(name, tags, count);
-      });
+  return runOnEngineDispatcher(e,
+                               [name = std::string(elements), tags, count](auto& engine) -> void {
+                                 engine.recordCounterInc(name, tags, count);
+                               });
 }
 
 envoy_status_t dump_stats(envoy_engine_t engine, envoy_data* out) {
   absl::Notification stats_received;
-  if (Envoy::EngineHandle::runOnEngineDispatcher(
-          engine, ([out, &stats_received](auto& engine) -> void {
-            Envoy::Buffer::OwnedImpl dumped_stats = engine.dumpStats();
-            *out = Envoy::Data::Utility::toBridgeData(dumped_stats, 1024 * 1024 * 100);
-            stats_received.Notify();
-          })) == ENVOY_FAILURE) {
+  if (runOnEngineDispatcher(engine, ([out, &stats_received](auto& engine) -> void {
+                              Envoy::Buffer::OwnedImpl dumped_stats = engine.dumpStats();
+                              *out = Envoy::Data::Utility::toBridgeData(dumped_stats,
+                                                                        1024 * 1024 * 100);
+                              stats_received.Notify();
+                            })) == ENVOY_FAILURE) {
     return ENVOY_FAILURE;
   }
   stats_received.WaitForNotification();
   return ENVOY_SUCCESS;
 }
 
 void flush_stats(envoy_engine_t e) {
-  Envoy::EngineHandle::runOnEngineDispatcher(e, [](auto& engine) { engine.flushStats(); });
+  runOnEngineDispatcher(e, [](auto& engine) { engine.flushStats(); });
 }
 
 envoy_status_t register_platform_api(const char* name, void* api) {
@@ -118,18 +123,29 @@ envoy_status_t register_platform_api(const char* name, void* api) {
 
 envoy_engine_t init_engine(envoy_engine_callbacks callbacks, envoy_logger logger,
                            envoy_event_tracker event_tracker) {
-  return Envoy::EngineHandle::initEngine(callbacks, logger, event_tracker);
+  auto engine = new Envoy::Engine(callbacks, logger, event_tracker);
+  return reinterpret_cast<envoy_engine_t>(engine);
 }
 
-envoy_status_t run_engine(envoy_engine_t engine, const char* config, const char* log_level) {
-  return Envoy::EngineHandle::runEngine(engine, config, log_level);
+envoy_status_t run_engine(envoy_engine_t handle, const char* config, const char* log_level) {
+  if (auto engine = reinterpret_cast<Envoy::Engine*>(handle)) {
+    engine->run(config, log_level);
+    return ENVOY_SUCCESS;
+  }
+  return ENVOY_FAILURE;
 }
 
-envoy_status_t terminate_engine(envoy_engine_t engine, bool release) {
-  return Envoy::EngineHandle::terminateEngine(engine, release);
+envoy_status_t terminate_engine(envoy_engine_t handle, bool release) {
+  auto engine = reinterpret_cast<Envoy::Engine*>(handle);
+  envoy_status_t ret = engine->terminate();
+  if (release) {
+    // TODO(jpsim): Always delete engine to avoid leaking it
+    delete engine;
+  }
+  return ret;
 }
 
 envoy_status_t reset_connectivity_state(envoy_engine_t e) {
-  return Envoy::EngineHandle::runOnEngineDispatcher(
+  return runOnEngineDispatcher(
       e, [](auto& engine) { engine.networkConnectivityManager().resetConnectivityState(); });
 }

mobile/test/common/engine_test.cc

@@ -2,7 +2,6 @@
 #include "gtest/gtest.h"
 #include "library/cc/engine_builder.h"
 #include "library/common/engine.h"
-#include "library/common/engine_handle.h"
 #include "library/common/main_interface.h"
 
 namespace Envoy {
@@ -79,21 +78,15 @@ TEST_F(EngineTest, AccessEngineAfterInitialization) {
   ASSERT_TRUE(test_context.on_engine_running.WaitForNotificationWithTimeout(absl::Seconds(10)));
 
   absl::Notification getClusterManagerInvoked;
-  // Scheduling on the dispatcher should work, the engine is running.
-  EXPECT_EQ(ENVOY_SUCCESS, EngineHandle::runOnEngineDispatcher(
-                               handle, [&getClusterManagerInvoked](Envoy::Engine& engine) {
-                                 engine.getClusterManager();
-                                 getClusterManagerInvoked.Notify();
-                               }));
-
-  // Validate that we actually invoked the function.
-  EXPECT_TRUE(getClusterManagerInvoked.WaitForNotificationWithTimeout(absl::Seconds(1)));
+  envoy_data stats_data;
+  // Running engine functions should work because the engine is running
+  EXPECT_EQ(ENVOY_SUCCESS, dump_stats(handle, &stats_data));
+  release_envoy_data(stats_data);
 
   engine_->terminate();
 
   // Now that the engine has been shut down, we no longer expect scheduling to work.
-  EXPECT_EQ(ENVOY_FAILURE, EngineHandle::runOnEngineDispatcher(
-                               handle, [](Envoy::Engine& engine) { engine.getClusterManager(); }));
+  EXPECT_EQ(ENVOY_FAILURE, dump_stats(handle, &stats_data));
 
   engine_.reset();
 }

mobile/test/common/integration/base_client_integration_test.cc

@@ -10,7 +10,6 @@
 #include "library/cc/bridge_utility.h"
 #include "library/cc/log_level.h"
 #include "library/common/engine.h"
-#include "library/common/engine_handle.h"
 #include "library/common/http/header_utility.h"
 #include "spdlog/spdlog.h"
 
@@ -222,16 +221,15 @@ uint64_t BaseClientIntegrationTest::getCounterValue(const std::string& name) {
   uint64_t counter_value = 0UL;
   uint64_t* counter_value_ptr = &counter_value;
   absl::Notification counter_value_set;
-  EXPECT_EQ(ENVOY_SUCCESS,
-            EngineHandle::runOnEngineDispatcher(
-                rawEngine(), [counter_value_ptr, &name, &counter_value_set](Envoy::Engine& engine) {
-                  Stats::CounterSharedPtr counter =
-                      TestUtility::findCounter(engine.getStatsStore(), name);
-                  if (counter != nullptr) {
-                    *counter_value_ptr = counter->value();
-                  }
-                  counter_value_set.Notify();
-                }));
+  auto engine = reinterpret_cast<Envoy::Engine*>(rawEngine());
+  engine->dispatcher().post([&] {
+    Stats::CounterSharedPtr counter = TestUtility::findCounter(engine->getStatsStore(), name);
+    if (counter != nullptr) {
+      *counter_value_ptr = counter->value();
+    }
+    counter_value_set.Notify();
+  });
+
   EXPECT_TRUE(counter_value_set.WaitForNotificationWithTimeout(absl::Seconds(5)));
   return counter_value;
 }
@@ -254,16 +252,14 @@ uint64_t BaseClientIntegrationTest::getGaugeValue(const std::string& name) {
   uint64_t gauge_value = 0UL;
   uint64_t* gauge_value_ptr = &gauge_value;
   absl::Notification gauge_value_set;
-  EXPECT_EQ(ENVOY_SUCCESS,
-            EngineHandle::runOnEngineDispatcher(
-                rawEngine(), [gauge_value_ptr, &name, &gauge_value_set](Envoy::Engine& engine) {
-                  Stats::GaugeSharedPtr gauge =
-                      TestUtility::findGauge(engine.getStatsStore(), name);
-                  if (gauge != nullptr) {
-                    *gauge_value_ptr = gauge->value();
-                  }
-                  gauge_value_set.Notify();
-                }));
+  auto engine = reinterpret_cast<Envoy::Engine*>(rawEngine());
+  engine->dispatcher().post([&] {
+    Stats::GaugeSharedPtr gauge = TestUtility::findGauge(engine->getStatsStore(), name);
+    if (gauge != nullptr) {
+      *gauge_value_ptr = gauge->value();
+    }
+    gauge_value_set.Notify();
+  });
   EXPECT_TRUE(gauge_value_set.WaitForNotificationWithTimeout(absl::Seconds(5)));
   return gauge_value;
 }

mobile/test/non_hermetic/gcp_traffic_director_integration_test.cc

@@ -26,7 +26,6 @@
 #include "absl/synchronization/notification.h"
 #include "gtest/gtest.h"
 #include "library/common/data/utility.h"
-#include "library/common/engine_handle.h"
 #include "library/common/types/c_types.h"
 #include "tools/cpp/runfiles/runfiles.h"