engine: return new handles from EngineHandle::initEngine (#2129)

Until now, `EngineHandle` has always had a single value of `1`.
In other words, the Envoy engine has always been a singleton with no way
to run multiple independent engines.

In order to support multiple concurrent engines
(envoyproxy/envoy-mobile#332), we first need
to make `EngineHandle::initEngine` create a new engine and rather than
have functions that use the engine do a singleton lookup, we can use the
`envoy_engine_t` as an opaque handle type to get back the
`Envoy::Engine` instance.

Several places were using a hardcoded value of `1`, so we need to plumb
through the engine handles.

With this change although you can now create multiple engine instances,
it is not safe to do so because there is still some static state that
will need to be untangled. Also note that like the singleton we have had
until now, new engine instances that are created will leak, as there is
no way to fully release the engine through the public API. This is
intentional in order to reduce the risk associated with this change.

Mike and I made these changes while pairing. The bulk of the ideas here
are really from him.

Risk Level: High.
Testing: Existing tests & Lyft apps validation.
Docs Changes: None.
Release Notes: Added.

Co-authored-by: Mike Schore <mike.schore@gmail.com>
Co-authored-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: JP Simard <jp@jpsim.com>

mobile/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:
 

mobile/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;
 }
 

mobile/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);

mobile/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

mobile/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

mobile/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

mobile/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(

mobile/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.

mobile/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

mobile/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 {