logger: Create only one Context per process for mobile

mobile/library/common/BUILD

@@ -36,7 +36,9 @@ envoy_cc_library(
         "//library/common/types:c_types_lib",
         "@envoy//envoy/server:lifecycle_notifier_interface",
         "@envoy//envoy/stats:stats_interface",
+        "@envoy//source/common/common:minimal_logger_lib",
         "@envoy//source/common/common:thread_impl_lib_posix",
+        "@envoy//source/common/common:thread_lib",
         "@envoy//source/common/common:utility_lib",
         "@envoy//source/common/runtime:runtime_lib",
         "@envoy_build_config//:extension_registry",

mobile/library/common/engine_common.cc

@@ -100,10 +100,12 @@ EngineCommon::EngineCommon(std::shared_ptr<Envoy::OptionsImplBase> options) : op
       };
   // `set_new_handler` is false because the application using Envoy Mobile should decide how to
   // handle `new` memory allocation failures.
-  base_ = std::make_unique<StrippedMainBase>(
-      *options_, real_time_system_, default_listener_hooks_, prod_component_factory_,
-      std::make_unique<PlatformImpl>(), std::make_unique<Random::RandomGeneratorImpl>(), nullptr,
-      create_instance, /*set_new_handler=*/false);
+  std::unique_ptr<Random::RandomGeneratorImpl> random_generator =
+      std::make_unique<Random::RandomGeneratorImpl>();
+  base_ = std::make_unique<StrippedMainBase>(*options_, prod_component_factory_,
+                                             std::make_unique<PlatformImpl>(), *random_generator);
+  base_->init(real_time_system_, default_listener_hooks_, std::move(random_generator), nullptr,
+              create_instance);
   // Disabling signal handling in the options makes it so that the server's event dispatcher _does
   // not_ listen for termination signals such as SIGTERM, SIGINT, etc
   // (https://github.com/envoyproxy/envoy/blob/048f4231310fbbead0cbe03d43ffb4307fff0517/source/server/server.cc#L519).

mobile/library/common/internal_engine.cc

@@ -4,6 +4,8 @@
 
 #include "source/common/api/os_sys_calls_impl.h"
 #include "source/common/common/lock_guard.h"
+#include "source/common/common/logger.h"
+#include "source/common/common/thread.h"
 #include "source/common/common/utility.h"
 #include "source/common/http/http_server_properties_cache_manager_impl.h"
 #include "source/common/network/io_socket_handle_impl.h"
@@ -19,6 +21,17 @@ constexpr absl::Duration ENGINE_RUNNING_TIMEOUT = absl::Seconds(30);
 // Google DNS address used for IPv6 probes.
 constexpr absl::string_view IPV6_PROBE_ADDRESS = "2001:4860:4860::8888";
 constexpr uint32_t IPV6_PROBE_PORT = 53;
+
+// There is only one shared static Logger::Context instance for all Envoy Mobile engines.
+// This helps avoid issues on Logger::Context destruction when the previous saved context
+// could be activated in a thread-unsafe manner.
+void initOnceLoggerContext(const OptionsImplBase& options) {
+  static Thread::MutexBasicLockable* log_lock = new Thread::MutexBasicLockable();
+  static Logger::Context* context =
+      new Logger::Context(options.logLevel(), options.logFormat(), *log_lock,
+                          options.logFormatEscaped(), options.enableFineGrainLogging());
+  UNREFERENCED_PARAMETER(context);
+}
 } // namespace
 
 static std::atomic<envoy_stream_t> current_stream_handle_{0};
@@ -90,15 +103,16 @@ envoy_status_t InternalEngine::cancelStream(envoy_stream_t stream) {
 // This function takes a `std::shared_ptr` instead of `std::unique_ptr` because `std::function` is a
 // copy-constructible type, so it's not possible to move capture `std::unique_ptr` with
 // `std::function`.
-envoy_status_t InternalEngine::run(std::shared_ptr<Envoy::OptionsImplBase> options) {
+envoy_status_t InternalEngine::run(std::shared_ptr<OptionsImplBase> options) {
+  initOnceLoggerContext(*options);
   Thread::Options thread_options;
   thread_options.priority_ = thread_priority_;
   main_thread_ = thread_factory_->createThread([this, options]() mutable -> void { main(options); },
                                                thread_options, /* crash_on_failure= */ false);
   return (main_thread_ != nullptr) ? ENVOY_SUCCESS : ENVOY_FAILURE;
 }
 
-envoy_status_t InternalEngine::main(std::shared_ptr<Envoy::OptionsImplBase> options) {
+envoy_status_t InternalEngine::main(std::shared_ptr<OptionsImplBase> options) {
   // Using unique_ptr ensures main_common's lifespan is strictly scoped to this function.
   std::unique_ptr<EngineCommon> main_common;
   {

mobile/library/common/internal_engine.h

@@ -41,7 +41,7 @@ class InternalEngine : public Logger::Loggable<Logger::Id::main> {
    * Run the engine with the provided options.
    * @param options, the Envoy options, including the Bootstrap configuration and log level.
    */
-  envoy_status_t run(std::shared_ptr<Envoy::OptionsImplBase> options);
+  envoy_status_t run(std::shared_ptr<OptionsImplBase> options);
 
   /**
    * Immediately terminate the engine, if running. Calling this function when
@@ -170,7 +170,7 @@ class InternalEngine : public Logger::Loggable<Logger::Id::main> {
                  std::unique_ptr<EnvoyEventTracker> event_tracker,
                  absl::optional<int> thread_priority, Thread::PosixThreadFactoryPtr thread_factory);
 
-  envoy_status_t main(std::shared_ptr<Envoy::OptionsImplBase> options);
+  envoy_status_t main(std::shared_ptr<OptionsImplBase> options);
   static void logInterfaces(absl::string_view event,
                             std::vector<Network::InterfacePair>& interfaces);
   /** Returns true if there is IPv6 connectivity. */

source/exe/main_common.cc

@@ -55,14 +55,23 @@ MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem
                                std::unique_ptr<Server::Platform> platform_impl,
                                std::unique_ptr<Random::RandomGenerator>&& random_generator,
                                std::unique_ptr<ProcessContext> process_context)
-    : StrippedMainBase(options, time_system, listener_hooks, component_factory,
-                       std::move(platform_impl), std::move(random_generator),
-                       std::move(process_context), createFunction())
+    : StrippedMainBase(options, component_factory, std::move(platform_impl), *random_generator)
 #ifdef ENVOY_ADMIN_FUNCTIONALITY
       ,
       shared_response_set_(std::make_shared<AdminResponse::PtrSet>())
 #endif
 {
+  if (options.mode() == Server::Mode::Serve) {
+    // Provide consistent behavior for out-of-memory, regardless of whether it occurs in a
+    // try/catch block or not.
+    std::set_new_handler([]() { PANIC("out of memory"); });
+  }
+
+  logging_context_ = std::make_unique<Logger::Context>(
+      options_.logLevel(), options_.logFormat(), restarter_->logLock(), options_.logFormatEscaped(),
+      options_.mode() == Server::Mode::Validate ? false : options_.enableFineGrainLogging());
+  init(time_system, listener_hooks, std::move(random_generator), std::move(process_context),
+       createFunction());
 }
 
 bool MainCommonBase::run() {

source/exe/stripped_main_base.cc

@@ -41,18 +41,15 @@ Runtime::LoaderPtr ProdComponentFactory::createRuntime(Server::Instance& server,
   return Server::InstanceUtil::createRuntime(server, config);
 }
 
-StrippedMainBase::StrippedMainBase(const Server::Options& options, Event::TimeSystem& time_system,
-                                   ListenerHooks& listener_hooks,
+StrippedMainBase::StrippedMainBase(const Server::Options& options,
                                    Server::ComponentFactory& component_factory,
                                    std::unique_ptr<Server::Platform> platform_impl,
-                                   std::unique_ptr<Random::RandomGenerator>&& random_generator,
-                                   std::unique_ptr<ProcessContext> process_context,
-                                   CreateInstanceFunction create_instance, bool set_new_handler)
+                                   Random::RandomGenerator& random_generator)
     : platform_impl_(std::move(platform_impl)), options_(options),
       component_factory_(component_factory), stats_allocator_(symbol_table_) {
   // Process the option to disable extensions as early as possible,
   // before we do any configuration loading.
-  OptionsImplBase::disableExtensions(options.disabledExtensions());
+  OptionsImplBase::disableExtensions(options_.disabledExtensions());
 
   // Enable core dumps as early as possible.
   if (options_.coreDumpEnabled()) {
@@ -66,37 +63,33 @@ StrippedMainBase::StrippedMainBase(const Server::Options& options, Event::TimeSy
 
   switch (options_.mode()) {
   case Server::Mode::InitOnly:
-  case Server::Mode::Serve: {
-    configureHotRestarter(*random_generator);
-
+  case Server::Mode::Serve:
+    configureHotRestarter(random_generator);
     tls_ = std::make_unique<ThreadLocal::InstanceImpl>();
-    Thread::BasicLockable& log_lock = restarter_->logLock();
-    Thread::BasicLockable& access_log_lock = restarter_->accessLogLock();
-    logging_context_ = std::make_unique<Logger::Context>(options_.logLevel(), options_.logFormat(),
-                                                         log_lock, options_.logFormatEscaped(),
-                                                         options_.enableFineGrainLogging());
+    stats_store_ = std::make_unique<Stats::ThreadLocalStoreImpl>(stats_allocator_);
+    break;
+  case Server::Mode::Validate:
+    restarter_ = std::make_unique<Server::HotRestartNopImpl>();
+    break;
+  }
+}
 
+void StrippedMainBase::init(Event::TimeSystem& time_system, ListenerHooks& listener_hooks,
+                            std::unique_ptr<Random::RandomGenerator>&& random_generator,
+                            std::unique_ptr<ProcessContext> process_context,
+                            CreateInstanceFunction create_instance) {
+  switch (options_.mode()) {
+  case Server::Mode::InitOnly:
+  case Server::Mode::Serve: {
     configureComponentLogLevels();
 
-    if (set_new_handler) {
-      // Provide consistent behavior for out-of-memory, regardless of whether it occurs in a
-      // try/catch block or not.
-      std::set_new_handler([]() { PANIC("out of memory"); });
-    }
-
-    stats_store_ = std::make_unique<Stats::ThreadLocalStoreImpl>(stats_allocator_);
-
     server_ = create_instance(*init_manager_, options_, time_system, listener_hooks, *restarter_,
-                              *stats_store_, access_log_lock, component_factory,
+                              *stats_store_, restarter_->accessLogLock(), component_factory_,
                               std::move(random_generator), *tls_, platform_impl_->threadFactory(),
                               platform_impl_->fileSystem(), std::move(process_context), nullptr);
     break;
   }
   case Server::Mode::Validate:
-    restarter_ = std::make_unique<Server::HotRestartNopImpl>();
-    logging_context_ =
-        std::make_unique<Logger::Context>(options_.logLevel(), options_.logFormat(),
-                                          restarter_->logLock(), options_.logFormatEscaped());
     process_context_ = std::move(process_context);
     break;
   }

source/exe/stripped_main_base.h

@@ -47,13 +47,18 @@ class StrippedMainBase {
   static std::string hotRestartVersion(bool hot_restart_enabled);
 
   // Consumer must guarantee that all passed references are alive until this object is
-  // destructed.
-  StrippedMainBase(const Server::Options& options, Event::TimeSystem& time_system,
-                   ListenerHooks& listener_hooks, Server::ComponentFactory& component_factory,
+  // destructed, except for `random_generator` which only needs to be alive until the constructor
+  // completes execution.
+  StrippedMainBase(const Server::Options& options, Server::ComponentFactory& component_factory,
                    std::unique_ptr<Server::Platform> platform_impl,
-                   std::unique_ptr<Random::RandomGenerator>&& random_generator,
-                   std::unique_ptr<ProcessContext> process_context,
-                   CreateInstanceFunction create_instance, bool set_new_handler = true);
+                   Random::RandomGenerator& random_generator);
+
+  // Initialize the Envoy server instance. Must be called prior to any other method to use the
+  // server. Separated from the constructor to allow for globals to be initialized first.
+  void init(Event::TimeSystem& time_system, ListenerHooks& listener_hooks,
+            std::unique_ptr<Random::RandomGenerator>&& random_generator,
+            std::unique_ptr<ProcessContext> process_context,
+            CreateInstanceFunction create_instance);
 
   void runServer() {
     ASSERT(options_.mode() == Server::Mode::Serve);
@@ -78,6 +83,10 @@ class StrippedMainBase {
   ThreadLocal::InstanceImplPtr tls_;
   std::unique_ptr<Server::HotRestart> restarter_;
   Stats::ThreadLocalStoreImplPtr stats_store_;
+  // logging_context_ is only used in (some) subclasses, but it must be declared here so that it's
+  // destructed after the server_. This is necessary because the Server and its threads may log
+  // during destruction, causing data race issues with the Context's destruction and activation of
+  // the saved context.
   std::unique_ptr<Logger::Context> logging_context_;
   std::unique_ptr<Init::Manager> init_manager_{std::make_unique<Init::ManagerImpl>("Server")};
   std::unique_ptr<Server::Instance> server_;