diff --git a/mobile/library/cc/engine.cc b/mobile/library/cc/engine.cc index 1e839139df59..2e3df7c5d3ce 100644 --- a/mobile/library/cc/engine.cc +++ b/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(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 diff --git a/mobile/library/cc/engine.h b/mobile/library/cc/engine.h index f5d8eb51f280..374b51b52754 100644 --- a/mobile/library/cc/engine.h +++ b/mobile/library/cc/engine.h @@ -37,7 +37,6 @@ class Engine : public std::enable_shared_from_this { Envoy::Engine* engine_; StreamClientSharedPtr stream_client_; - bool terminated_; }; using EngineSharedPtr = std::shared_ptr; diff --git a/mobile/library/common/engine.cc b/mobile/library/common/engine.cc index 488314e13bc8..1153017ef085 100644 --- a/mobile/library/common/engine.cc +++ b/mobile/library/common/engine.cc @@ -146,6 +146,10 @@ envoy_status_t Engine::main(std::unique_ptr&& 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() { diff --git a/mobile/library/common/engine.h b/mobile/library/common/engine.h index 56ee604c29a5..e4200d482909 100644 --- a/mobile/library/common/engine.h +++ b/mobile/library/common/engine.h @@ -39,10 +39,14 @@ class Engine : public Logger::Loggable { envoy_status_t run(std::unique_ptr&& 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 { 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 { // 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; diff --git a/mobile/library/common/jni/jni_impl.cc b/mobile/library/common/jni/jni_impl.cc index cf1f9600a1d5..dbdc4b1d7751 100644 --- a/mobile/library/common/jni/jni_impl.cc +++ b/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(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 str = Envoy::JNI::envoyDataToJavaString(jni_helper, data); - release_envoy_data(data); - - return str.release(); + return jni_helper.newStringUtf(stats.c_str()).release(); } // JvmCallbackContext diff --git a/mobile/library/objective-c/EnvoyEngineImpl.mm b/mobile/library/objective-c/EnvoyEngineImpl.mm index e53301b53582..cd1af400b619 100644 --- a/mobile/library/objective-c/EnvoyEngineImpl.mm +++ b/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 { diff --git a/mobile/test/common/engine_test.cc b/mobile/test/common/engine_test.cc index ed2d05bb662f..f9ba6874290c 100644 --- a/mobile/test/common/engine_test.cc +++ b/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(); }