envoyproxy · jpsim · Jul 6, 2022 · Mar 25, 2022 · Mar 28, 2022 · Mar 28, 2022
diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst
@@ -15,6 +15,9 @@ Breaking changes:
 - iOS: enable usage of ``NWPathMonitor`` by default (:issue:`#2329 <2329>`)
 - iOS: replace ``enableNetworkPathMonitor`` with a new ``setNetworkMonitoringMode`` API to allow disabling monitoring (:issue:`#2345 <2345>`)
 - iOS: release artifacts no longer embed bitcode
+- api: engines are no longer a singleton, you may need to update your code to only create engines once and hold on to them.
+  You also cannot assume that an `envoy_engine_t` value of `1` will return the default engine.
+  Support for using multiple engines concurrently is coming later. (:issue:`#2129 <2129>`)
 
 Bugfixes:
 

diff --git a/library/cc/engine.cc b/library/cc/engine.cc
@@ -22,7 +22,7 @@ void Engine::terminate() {
   if (terminated_) {
     throw std::runtime_error("attempting to double terminate Engine");
   }
-  terminate_engine(engine_);
+  terminate_engine(engine_, /* release */ false);
   terminated_ = true;
 }
 

diff --git a/library/common/engine.cc b/library/common/engine.cc
@@ -78,6 +78,7 @@ envoy_status_t Engine::main(const std::string config, const std::string log_leve
             });
       }
 
+      // We let the thread clean up this log delegate pointer
       if (logger_.log) {
         log_delegate_ptr_ =
             std::make_unique<Logger::LambdaDelegate>(logger_, Logger::Registry::getSink());
@@ -143,7 +144,6 @@ envoy_status_t Engine::main(const std::string config, const std::string log_leve
   connectivity_manager_.reset();
   client_scope_.reset();
   stat_name_set_.reset();
-  log_delegate_ptr_.reset(nullptr);
   main_common.reset(nullptr);
   bug_handler_registration_.reset(nullptr);
   assert_handler_registration_.reset(nullptr);

diff --git a/library/common/engine_handle.cc b/library/common/engine_handle.cc
@@ -2,47 +2,36 @@
 
 namespace Envoy {
 
-envoy_status_t EngineHandle::runOnEngineDispatcher(envoy_engine_t,
+envoy_status_t EngineHandle::runOnEngineDispatcher(envoy_engine_t handle,
                                                    std::function<void(Envoy::Engine&)> func) {
-  if (auto e = engine()) {
-    return e->dispatcher().post([func]() {
-      if (auto e = engine()) {
-        func(*e);
-      }
-    });
+  if (auto engine = reinterpret_cast<Envoy::Engine*>(handle)) {
+    return engine->dispatcher().post([engine, func]() { func(*engine); });
   }
   return ENVOY_FAILURE;
 }
 
 envoy_engine_t EngineHandle::initEngine(envoy_engine_callbacks callbacks, envoy_logger logger,
                                         envoy_event_tracker event_tracker) {
-  // TODO(goaway): return new handle once multiple engine support is in place.
-  // https://github.com/envoyproxy/envoy-mobile/issues/332
-  strong_engine_ = std::make_shared<Envoy::Engine>(callbacks, logger, event_tracker);
-  engine_ = strong_engine_;
-  return 1;
+  auto engine = new Envoy::Engine(callbacks, logger, event_tracker);
+  return reinterpret_cast<envoy_engine_t>(engine);
 }
 
-envoy_status_t EngineHandle::runEngine(envoy_engine_t, const char* config, const char* log_level,
-                                       const char* admin_address_path) {
-  // This will change once multiple engine support is in place.
-  // https://github.com/envoyproxy/envoy-mobile/issues/332
-  if (auto e = engine()) {
-    e->run(config, log_level, admin_address_path);
+envoy_status_t EngineHandle::runEngine(envoy_engine_t handle, const char* config,
+                                       const char* log_level, const char* admin_address_path) {
+  if (auto engine = reinterpret_cast<Envoy::Engine*>(handle)) {
+    engine->run(config, log_level, admin_address_path);
     return ENVOY_SUCCESS;
   }
-
   return ENVOY_FAILURE;
 }
 
-void EngineHandle::terminateEngine(envoy_engine_t) {
-  // Reset the primary handle to the engine, but retain it long enough to synchronously terminate.
-  auto e = strong_engine_;
-  strong_engine_.reset();
-  e->terminate();
+void EngineHandle::terminateEngine(envoy_engine_t handle, bool release) {
+  auto engine = reinterpret_cast<Envoy::Engine*>(handle);
+  engine->terminate();
+  if (release) {
+    // TODO(jpsim): Always delete engine to avoid leaking it
+    delete engine;
+  }
 }
 
-EngineSharedPtr EngineHandle::strong_engine_;
-EngineWeakPtr EngineHandle::engine_;
-
 } // namespace Envoy
diff --git a/library/common/engine_handle.h b/library/common/engine_handle.h
@@ -23,26 +23,18 @@ class EngineHandle {
                                               std::function<void(Envoy::Engine&)> func);
 
 private:
-  static std::shared_ptr<Envoy::Engine> engine() {
-    // TODO(goaway): enable configurable heap-based allocation
-    return engine_.lock();
-  }
-
   static envoy_engine_t initEngine(envoy_engine_callbacks callbacks, envoy_logger logger,
                                    envoy_event_tracker event_tracker);
   static envoy_status_t runEngine(envoy_engine_t, const char* config, const char* log_level,
                                   const char* admin_address_path);
-  static void terminateEngine(envoy_engine_t);
-
-  static EngineSharedPtr strong_engine_;
-  static EngineWeakPtr engine_;
+  static void terminateEngine(envoy_engine_t handle, bool release);
 
   // Allow a specific list of functions to access the internal setup/teardown functionality.
   friend envoy_engine_t(::init_engine)(envoy_engine_callbacks callbacks, envoy_logger logger,
                                        envoy_event_tracker event_tracker);
   friend envoy_status_t(::run_engine)(envoy_engine_t, const char* config, const char* log_level,
                                       const char* admin_address_path);
-  friend void ::terminate_engine(envoy_engine_t engine);
+  friend void ::terminate_engine(envoy_engine_t engine, bool release);
 };
 
 } // namespace Envoy
diff --git a/library/common/jni/jni_interface.cc b/library/common/jni/jni_interface.cc
@@ -125,7 +125,7 @@ extern "C" JNIEXPORT jint JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibra
 
 extern "C" JNIEXPORT void JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_terminateEngine(
     JNIEnv* env, jclass, jlong engine_handle) {
-  terminate_engine(static_cast<envoy_engine_t>(engine_handle));
+  terminate_engine(static_cast<envoy_engine_t>(engine_handle), /* release */ false);
 }
 
 extern "C" JNIEXPORT jstring JNICALL

diff --git a/library/common/main_interface.cc b/library/common/main_interface.cc
@@ -188,7 +188,9 @@ envoy_status_t run_engine(envoy_engine_t engine, const char* config, const char*
   return Envoy::EngineHandle::runEngine(engine, config, log_level, admin_path);
 }
 
-void terminate_engine(envoy_engine_t engine) { Envoy::EngineHandle::terminateEngine(engine); }
+void terminate_engine(envoy_engine_t engine, bool release) {
+  Envoy::EngineHandle::terminateEngine(engine, release);
+}
 
 envoy_status_t reset_connectivity_state(envoy_engine_t e) {
   return Envoy::EngineHandle::runOnEngineDispatcher(

diff --git a/library/common/main_interface.h b/library/common/main_interface.h
@@ -201,8 +201,9 @@ envoy_status_t run_engine(envoy_engine_t engine, const char* config, const char*
  * Terminate an engine. Further interactions with a terminated engine, or streams created by a
  * terminated engine is illegal.
  * @param engine, handle to the engine to terminate.
+ * @param release, set to true to release the engine from memory.
  */
-void terminate_engine(envoy_engine_t engine);
+void terminate_engine(envoy_engine_t engine, bool release);
 
 /**
  * Refresh DNS, and drain connections associated with an engine.

diff --git a/library/objective-c/EnvoyEngineImpl.m b/library/objective-c/EnvoyEngineImpl.m
@@ -624,7 +624,7 @@ - (NSString *)dumpStats {
 }
 
 - (void)terminate {
-  terminate_engine(_engineHandle);
+  terminate_engine(_engineHandle, /* release */ false);
 }
 
 - (void)resetConnectivityState {
@@ -647,7 +647,7 @@ - (void)startObservingLifecycleNotifications {
 
 - (void)terminateNotification:(NSNotification *)notification {
   NSLog(@"[Envoy %ld] terminating engine (%@)", _engineHandle, notification.name);
-  terminate_engine(_engineHandle);
+  terminate_engine(_engineHandle, /* release */ false);
 }
 
 @end
diff --git a/test/common/engine_test.cc b/test/common/engine_test.cc
@@ -53,7 +53,9 @@ struct TestEngineHandle {
     run_engine(handle_, MINIMAL_TEST_CONFIG.c_str(), level.c_str(), "");
   }
 
-  void terminate() { terminate_engine(handle_); }
+  void terminate() { terminate_engine(handle_, /* release */ false); }
+
+  ~TestEngineHandle() { terminate_engine(handle_, /* release */ true); }
 };
 
 class EngineTest : public testing::Test {