From e2613cabc11dd13bb2185e65f7e6d5d13e426bb0 Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Wed, 22 May 2024 15:53:48 -0500 Subject: [PATCH] mobile: Wait until the Engine is ready before calling terminate (#34287) When the Engine is not fully initialized, calling terminate() will cause an assertion failure. [2024-05-21 21:28:16.718][18][critical][assert] [external/envoy/source/common/stats/thread_local_store.cc:49] assert failure: scopes_.empty(). This PR adds a workaround to wait until the Engine before proceeding to call terminate(). Risk Level: low Testing: unit tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: mobile Signed-off-by: Fredy Wijaya --- mobile/library/common/internal_engine.cc | 8 ++++ mobile/library/common/internal_engine.h | 2 + mobile/test/cc/engine_test.cc | 52 ++++++++++++++++++++++++ 3 files changed, 62 insertions(+) diff --git a/mobile/library/common/internal_engine.cc b/mobile/library/common/internal_engine.cc index b3a0d668657e..46d6a09e2e61 100644 --- a/mobile/library/common/internal_engine.cc +++ b/mobile/library/common/internal_engine.cc @@ -11,6 +11,9 @@ #include "library/common/stats/utility.h" namespace Envoy { +namespace { +constexpr absl::Duration ENGINE_RUNNING_TIMEOUT = absl::Seconds(3); +} // namespace static std::atomic current_stream_handle_{0}; @@ -173,6 +176,7 @@ envoy_status_t InternalEngine::main(std::shared_ptr opti server_->serverFactoryContext().scope(), server_->api().randomGenerator()); dispatcher_->drain(server_->dispatcher()); + engine_running_.Notify(); callbacks_->on_engine_running_(); }); } // mutex_ @@ -212,6 +216,10 @@ envoy_status_t InternalEngine::terminate() { return ENVOY_FAILURE; } + // Wait until the Engine is ready before calling terminate to avoid assertion failures. + // TODO(fredyw): Fix this without having to wait. + ASSERT(engine_running_.WaitForNotificationWithTimeout(ENGINE_RUNNING_TIMEOUT)); + // We need to be sure that MainCommon is finished being constructed so we can dispatch shutdown. { Thread::LockGuard lock(mutex_); diff --git a/mobile/library/common/internal_engine.h b/mobile/library/common/internal_engine.h index 6a247719f785..f70af3a2a31a 100644 --- a/mobile/library/common/internal_engine.h +++ b/mobile/library/common/internal_engine.h @@ -7,6 +7,7 @@ #include "source/common/common/posix/thread_impl.h" #include "source/common/common/thread.h" +#include "absl/synchronization/notification.h" #include "absl/types/optional.h" #include "extension_registry.h" #include "library/common/engine_common.h" @@ -163,6 +164,7 @@ class InternalEngine : public Logger::Loggable { // instructions scheduled on the main_thread_ need to have a longer lifetime. Thread::PosixThreadPtr main_thread_{nullptr}; // Empty placeholder to be populated later. bool terminated_{false}; + absl::Notification engine_running_; }; } // namespace Envoy diff --git a/mobile/test/cc/engine_test.cc b/mobile/test/cc/engine_test.cc index 3bd96e8bc9f1..b8d111bce024 100644 --- a/mobile/test/cc/engine_test.cc +++ b/mobile/test/cc/engine_test.cc @@ -141,4 +141,56 @@ TEST(EngineTest, SetEventTracker) { EXPECT_TRUE(on_track.WaitForNotificationWithTimeout(absl::Seconds(3))); } +TEST(EngineTest, DontWaitForOnEngineRunning) { + Platform::EngineBuilder engine_builder; + engine_builder.setLogLevel(Logger::Logger::debug).enforceTrustChainVerification(false); + EngineWithTestServer engine_with_test_server(engine_builder, TestServerType::HTTP2_WITH_TLS); + + std::string actual_status_code; + bool actual_end_stream = false; + absl::Notification stream_complete; + EnvoyStreamCallbacks stream_callbacks; + stream_callbacks.on_headers_ = [&](const Http::ResponseHeaderMap& headers, bool end_stream, + envoy_stream_intel) { + actual_status_code = headers.getStatusValue(); + actual_end_stream = end_stream; + }; + stream_callbacks.on_data_ = [&](const Buffer::Instance&, uint64_t /* length */, bool end_stream, + envoy_stream_intel) { actual_end_stream = end_stream; }; + stream_callbacks.on_complete_ = [&](envoy_stream_intel, envoy_final_stream_intel) { + stream_complete.Notify(); + }; + stream_callbacks.on_error_ = [&](EnvoyError, envoy_stream_intel, envoy_final_stream_intel) { + stream_complete.Notify(); + }; + stream_callbacks.on_cancel_ = [&](envoy_stream_intel, envoy_final_stream_intel) { + stream_complete.Notify(); + }; + auto stream_prototype = engine_with_test_server.engine()->streamClient()->newStreamPrototype(); + Platform::StreamSharedPtr stream = stream_prototype->start(std::move(stream_callbacks)); + + auto headers = Http::Utility::createRequestHeaderMapPtr(); + headers->addCopy(Http::LowerCaseString(":method"), "GET"); + headers->addCopy(Http::LowerCaseString(":scheme"), "https"); + headers->addCopy(Http::LowerCaseString(":authority"), + engine_with_test_server.testServer().getAddress()); + headers->addCopy(Http::LowerCaseString(":path"), "/"); + stream->sendHeaders(std::move(headers), true); + stream_complete.WaitForNotification(); + + EXPECT_EQ(actual_status_code, "200"); + EXPECT_TRUE(actual_end_stream); +} + +TEST(EngineTest, TerminateWithoutWaitingForOnEngineRunning) { + absl::Notification engine_running; + auto engine_callbacks = std::make_unique(); + engine_callbacks->on_engine_running_ = [&] { engine_running.Notify(); }; + + Platform::EngineBuilder engine_builder; + auto engine = engine_builder.setLogLevel(Logger::Logger::debug).build(); + + engine->terminate(); +} + } // namespace Envoy