mobile: Consolidate dumpStats and terminate implementations to common…

…/engine.cc (envoyproxy#32059)

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

mobile/library/cc/engine.cc

@@ -1,16 +1,15 @@
 #include "engine.h"
 
-#include "library/common/data/utility.h"
 #include "library/common/engine.h"
 #include "library/common/types/c_types.h"
 
 namespace Envoy {
 namespace Platform {
 
-Engine::Engine(::Envoy::Engine* engine) : engine_(engine), terminated_(false) {}
+Engine::Engine(::Envoy::Engine* engine) : engine_(engine) {}
 
 Engine::~Engine() {
-  if (!terminated_) {
+  if (!engine_->isTerminated()) {
     terminate();
   }
 }
@@ -23,26 +22,9 @@ StreamClientSharedPtr Engine::streamClient() {
   return std::make_shared<StreamClient>(shared_from_this());
 }
 
-std::string Engine::dumpStats() {
-  envoy_data data;
-  if (engine_->dumpStats(&data) == ENVOY_FAILURE) {
-    return "";
-  }
-  const std::string to_return = Data::Utility::copyToString(data);
-  release_envoy_data(data);
-
-  return to_return;
-}
+std::string Engine::dumpStats() { return engine_->dumpStats(); }
 
-envoy_status_t Engine::terminate() {
-  if (terminated_) {
-    IS_ENVOY_BUG("attempted to double terminate engine");
-    return ENVOY_FAILURE;
-  }
-  envoy_status_t ret = engine_->terminate();
-  terminated_ = true;
-  return ret;
-}
+envoy_status_t Engine::terminate() { return engine_->terminate(); }
 
 } // namespace Platform
 } // namespace Envoy

mobile/library/cc/engine.h

@@ -37,7 +37,6 @@ class Engine : public std::enable_shared_from_this<Engine> {
 
   Envoy::Engine* engine_;
   StreamClientSharedPtr stream_client_;
-  bool terminated_;
 };
 
 using EngineSharedPtr = std::shared_ptr<Engine>;

mobile/library/common/engine.cc

@@ -146,6 +146,10 @@ envoy_status_t Engine::main(std::unique_ptr<Envoy::OptionsImplBase>&& options) {
 }
 
 envoy_status_t Engine::terminate() {
+  if (terminated_) {
+    IS_ENVOY_BUG("attempted to double terminate engine");
+    return ENVOY_FAILURE;
+  }
   // If main_thread_ has finished (or hasn't started), there's nothing more to do.
   if (!main_thread_.joinable()) {
     return ENVOY_FAILURE;
@@ -177,11 +181,17 @@ envoy_status_t Engine::terminate() {
   if (std::this_thread::get_id() != main_thread_.get_id()) {
     main_thread_.join();
   }
-
+  terminated_ = true;
   return ENVOY_SUCCESS;
 }
 
-Engine::~Engine() { terminate(); }
+bool Engine::isTerminated() const { return terminated_; }
+
+Engine::~Engine() {
+  if (!terminated_) {
+    terminate();
+  }
+}
 
 envoy_status_t Engine::setProxySettings(const char* hostname, const uint16_t port) {
   return dispatcher_->post([&, host = std::string(hostname), port]() -> void {
@@ -254,23 +264,23 @@ void handlerStats(Stats::Store& stats, Buffer::Instance& response) {
   statsAsText(all_stats, histograms, response);
 }
 
-envoy_status_t Engine::dumpStats(envoy_data* out) {
-  // If the engine isn't running, fail.
+std::string Engine::dumpStats() {
   if (!main_thread_.joinable()) {
-    return ENVOY_FAILURE;
+    return "";
   }
 
+  std::string stats;
   absl::Notification stats_received;
   if (dispatcher_->post([&]() -> void {
         Envoy::Buffer::OwnedImpl instance;
         handlerStats(server_->stats(), instance);
-        *out = Envoy::Data::Utility::toBridgeData(instance, 1024 * 1024 * 100);
+        stats = instance.toString();
         stats_received.Notify();
       }) == ENVOY_SUCCESS) {
     stats_received.WaitForNotification();
-    return ENVOY_SUCCESS;
+    return stats;
   }
-  return ENVOY_FAILURE;
+  return stats;
 }
 
 Upstream::ClusterManager& Engine::getClusterManager() {

mobile/library/common/engine.h

@@ -39,10 +39,14 @@ class Engine : public Logger::Loggable<Logger::Id::main> {
   envoy_status_t run(std::unique_ptr<Envoy::OptionsImplBase>&& options);
 
   /**
-   * Immediately terminate the engine, if running.
+   * Immediately terminate the engine, if running. Calling this function when
+   * the engine has been terminated will result in `ENVOY_FAILURE`.
    */
   envoy_status_t terminate();
 
+  /** Returns true if the Engine has been terminated; false otherwise. */
+  bool isTerminated() const;
+
   /**
    * Accessor for the provisional event dispatcher.
    * @return Event::ProvisionalDispatcher&, the engine dispatcher.
@@ -98,13 +102,9 @@ class Engine : public Logger::Loggable<Logger::Id::main> {
                                   uint64_t count);
 
   /**
-   * Dump Envoy stats into the provided envoy_data
-   * @params envoy_data which will be filed with referenced stats dumped in Envoy's standard text
-   * format.
-   * @return failure status if the engine is no longer running.
-   * This can be called from any thread, but will block on engine-thread processing.
+   * Dumps Envoy stats into string. Returns an empty string when an error occurred.
    */
-  envoy_status_t dumpStats(envoy_data* out);
+  std::string dumpStats();
 
   /**
    * Get cluster manager from the Engine.
@@ -142,6 +142,7 @@ class Engine : public Logger::Loggable<Logger::Id::main> {
   // main_thread_ should be destroyed first, hence it is the last member variable. Objects with
   // instructions scheduled on the main_thread_ need to have a longer lifetime.
   std::thread main_thread_{}; // Empty placeholder to be populated later.
+  bool terminated_{false};
 };
 
 using EngineSharedPtr = std::shared_ptr<Engine>;

mobile/library/common/jni/jni_impl.cc

@@ -181,17 +181,9 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_dumpStats(JNIEnv* env,
                                                            jlong engine_handle) {
   jni_log("[Envoy]", "dumpStats");
   auto engine = reinterpret_cast<Envoy::Engine*>(engine_handle);
-  envoy_data data;
-  jint result = engine->dumpStats(&data);
+  std::string stats = engine->dumpStats();
   Envoy::JNI::JniHelper jni_helper(env);
-  if (result != ENVOY_SUCCESS) {
-    return jni_helper.newStringUtf("").release();
-  }
-
-  Envoy::JNI::LocalRefUniquePtr<jstring> str = Envoy::JNI::envoyDataToJavaString(jni_helper, data);
-  release_envoy_data(data);
-
-  return str.release();
+  return jni_helper.newStringUtf(stats.c_str()).release();
 }
 
 // JvmCallbackContext

mobile/library/objective-c/EnvoyEngineImpl.mm

@@ -569,17 +569,12 @@ - (int)recordCounterInc:(NSString *)elements tags:(EnvoyTags *)tags count:(NSUIn
 }
 
 - (NSString *)dumpStats {
-  envoy_data data;
-  envoy_status_t status = _engine->dumpStats(&data);
-  if (status != ENVOY_SUCCESS) {
+  std::string status = _engine->dumpStats();
+  if (status == "") {
     return @"";
   }
 
-  NSString *stringCopy = [[NSString alloc] initWithBytes:data.bytes
-                                                  length:data.length
-                                                encoding:NSUTF8StringEncoding];
-  release_envoy_data(data);
-  return stringCopy;
+  return @(status.c_str());
 }
 
 - (void)terminate {

mobile/test/common/engine_test.cc

@@ -21,8 +21,13 @@ struct TestEngine {
   }
 
   envoy_status_t terminate() { return engine_->terminate(); }
+  bool isTerminated() const { return engine_->isTerminated(); }
 
-  ~TestEngine() { engine_->terminate(); }
+  ~TestEngine() {
+    if (!engine_->isTerminated()) {
+      engine_->terminate();
+    }
+  }
 };
 
 class EngineTest : public testing::Test {
@@ -54,6 +59,7 @@ TEST_F(EngineTest, EarlyExit) {
   ASSERT_TRUE(test_context.on_engine_running.WaitForNotificationWithTimeout(absl::Seconds(10)));
 
   ASSERT_EQ(engine_->terminate(), ENVOY_SUCCESS);
+  ASSERT_TRUE(engine_->isTerminated());
   ASSERT_TRUE(test_context.on_exit.WaitForNotificationWithTimeout(absl::Seconds(10)));
 
   engine_->engine_->startStream(0, {}, false);
@@ -77,15 +83,14 @@ TEST_F(EngineTest, AccessEngineAfterInitialization) {
   ASSERT_TRUE(test_context.on_engine_running.WaitForNotificationWithTimeout(absl::Seconds(10)));
 
   absl::Notification getClusterManagerInvoked;
-  envoy_data stats_data;
   // Running engine functions should work because the engine is running
-  EXPECT_EQ(ENVOY_SUCCESS, engine_->engine_->dumpStats(&stats_data));
-  release_envoy_data(stats_data);
+  EXPECT_EQ("runtime.load_success: 1\n", engine_->engine_->dumpStats());
 
   engine_->terminate();
+  ASSERT_TRUE(engine_->isTerminated());
 
   // Now that the engine has been shut down, we no longer expect scheduling to work.
-  EXPECT_EQ(ENVOY_FAILURE, engine_->engine_->dumpStats(&stats_data));
+  EXPECT_EQ("", engine_->engine_->dumpStats());
 
   engine_.reset();
 }