main: use custom main common to disable listening for termination sig…

…nals (#835)

Description: Disabling signal handling in the Server::Options makes it so that the server's event dispatcher does not listen for termination signals such as SIGTERM, SIGINT, etc. Previous crashes in iOS were experienced due to out-of-band event loop exit as described in #831. Ignoring termination signals makes it more likely that the event loop will only exit due to Engine destruction.

This PR introduces Envoy::MobileMainCommon, as this is the canonical way to customize how main runs, and options setup per #3424. The new Envoy::MobileMainCommon also does away with other logic in Envoy::MainCommon that does not apply to Envoy Mobile.

Risk Level: med - low-level change in termination handling
Testing: unit test to assert option is correctly set. End to end test with iOS and Android devices to ensure clean exit when the app using envoy mobile exits (and thus destructs the engine). Moreover, there is no event loop exit any longer when the simulator app receives a SIGTERM, i.e., the event dispatcher is no longer listening to SIGTERM and exiting due to them.

Potentially fixes #831. Will need to verify with wider client release.

Signed-off-by: Jose Nino jnino@lyft.com
Signed-off-by: JP Simard <jp@jpsim.com>

mobile/library/common/BUILD

@@ -16,11 +16,23 @@ envoy_cc_library(
     hdrs = ["main_interface.h"],
     repository = "@envoy",
     deps = [
+        ":envoy_mobile_main_common_lib",
         "//library/common/buffer:utility_lib",
         "//library/common/http:dispatcher_lib",
         "//library/common/http:header_utility_lib",
         "//library/common/types:c_types_lib",
         "@envoy//include/envoy/server:lifecycle_notifier_interface",
+    ],
+)
+
+envoy_cc_library(
+    name = "envoy_mobile_main_common_lib",
+    srcs = ["envoy_mobile_main_common.cc"],
+    hdrs = ["envoy_mobile_main_common.h"],
+    repository = "@envoy",
+    deps = [
+        "@envoy//source/common/runtime:runtime_lib",
+        "@envoy//source/exe:envoy_common_lib",
         "@envoy//source/exe:envoy_main_common_lib",
     ],
 )

mobile/library/common/engine.cc

@@ -44,7 +44,7 @@ envoy_status_t Engine::run(std::string config, std::string log_level) {
       char* envoy_argv[] = {strdup("envoy"), strdup("--config-yaml"),   strdup(config.c_str()),
                             strdup("-l"),    strdup(log_level.c_str()), nullptr};
 
-      main_common_ = std::make_unique<Envoy::MainCommon>(5, envoy_argv);
+      main_common_ = std::make_unique<MobileMainCommon>(5, envoy_argv);
       event_dispatcher_ = &main_common_->server()->dispatcher();
       cv_.notifyOne();
     } catch (const Envoy::NoServingException& e) {

mobile/library/common/engine.h

@@ -4,8 +4,6 @@
 
 #include "common/upstream/logical_dns_cluster.h"
 
-#include "exe/main_common.h"
-
 #include "extensions/clusters/dynamic_forward_proxy/cluster.h"
 #include "extensions/filters/http/dynamic_forward_proxy/config.h"
 #include "extensions/filters/http/router/config.h"
@@ -14,6 +12,7 @@
 #include "extensions/transport_sockets/tls/config.h"
 
 #include "absl/base/call_once.h"
+#include "library/common/envoy_mobile_main_common.h"
 #include "library/common/http/dispatcher.h"
 #include "library/common/types/c_types.h"
 
@@ -57,7 +56,7 @@ class Engine {
   Thread::CondVar cv_;
   std::thread main_thread_;
   std::unique_ptr<Http::Dispatcher> http_dispatcher_;
-  std::unique_ptr<MainCommon> main_common_ GUARDED_BY(mutex_);
+  std::unique_ptr<MobileMainCommon> main_common_ GUARDED_BY(mutex_);
   Server::Instance* server_{};
   Server::ServerLifecycleNotifier::HandlePtr postinit_callback_handler_;
   Event::Dispatcher* event_dispatcher_;

mobile/library/common/envoy_mobile_main_common.cc

@@ -0,0 +1,22 @@
+#include "library/common/envoy_mobile_main_common.h"
+
+#include "common/runtime/runtime_impl.h"
+
+namespace Envoy {
+
+MobileMainCommon::MobileMainCommon(int argc, const char* const* argv)
+    : options_(argc, argv, &MainCommon::hotRestartVersion, spdlog::level::info),
+      base_(options_, real_time_system_, default_listener_hooks_, prod_component_factory_,
+            std::make_unique<Runtime::RandomGeneratorImpl>(), platform_impl_.threadFactory(),
+            platform_impl_.fileSystem(), nullptr) {
+  // 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).
+  // Previous crashes in iOS were experienced due to early event loop exit as described in
+  // https://github.com/lyft/envoy-mobile/issues/831. Ignoring termination signals makes it more
+  // likely that the event loop will only exit due to Engine destruction
+  // https://github.com/lyft/envoy-mobile/blob/a72a51e64543882ea05fba3c76178b5784d39cdc/library/common/engine.cc#L105.
+  options_.setSignalHandling(false);
+}
+
+} // namespace Envoy

mobile/library/common/envoy_mobile_main_common.h

@@ -0,0 +1,40 @@
+#pragma once
+
+#include "envoy/event/timer.h"
+#include "envoy/server/instance.h"
+
+#include "common/event/real_time_system.h"
+
+#include "exe/main_common.h"
+#include "exe/platform_impl.h"
+
+#include "server/listener_hooks.h"
+#include "server/options_impl.h"
+
+namespace Envoy {
+
+/**
+ * This class is used instead of Envoy::MainCommon to customize logic for the Envoy Mobile setting.
+ * It largely leverages Envoy::MainCommonBase.
+ */
+class MobileMainCommon {
+public:
+  MobileMainCommon(int argc, const char* const* argv);
+  bool run() { return base_.run(); }
+
+  /**
+   * @return a pointer to the server instance, or nullptr if initialized into
+   *         validation mode.
+   */
+  Server::Instance* server() { return base_.server(); }
+
+private:
+  PlatformImpl platform_impl_;
+  Envoy::OptionsImpl options_;
+  Event::RealTimeSystem real_time_system_; // NO_CHECK_FORMAT(real_time)
+  DefaultListenerHooks default_listener_hooks_;
+  ProdComponentFactory prod_component_factory_;
+  MainCommonBase base_;
+};
+
+} // namespace Envoy

mobile/test/common/BUILD

@@ -0,0 +1,14 @@
+licenses(["notice"])  # Apache 2
+
+load("@envoy//bazel:envoy_build_system.bzl", "envoy_cc_test", "envoy_package")
+
+envoy_package()
+
+envoy_cc_test(
+    name = "envoy_mobile_main_common_test",
+    srcs = ["envoy_mobile_main_common_test.cc"],
+    repository = "@envoy",
+    deps = [
+        "//library/common:envoy_mobile_main_common_lib",
+    ],
+)

mobile/test/common/envoy_mobile_main_common_test.cc

@@ -0,0 +1,12 @@
+#include "gtest/gtest.h"
+#include "library/common/envoy_mobile_main_common.h"
+
+namespace Envoy {
+
+TEST(MobileMainCommonTest, SignalHandlingFalse) {
+  std::vector<const char*> envoy_argv{"envoy", "--config-yaml", "{}", nullptr};
+  MobileMainCommon main_common{3, &envoy_argv[0]};
+  ASSERT_FALSE(main_common.server()->options().signalHandlingEnabled());
+}
+
+} // namespace Envoy