server: make initialize non-virtual (envoyproxy#30531)

Risk Level: low (any overrides must be updated due to constructor change) Testing: updated tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <[email protected]>
kyessenov · Nov 2, 2023 · 4991c0f · 4991c0f
1 parent 3d04be9
commit 4991c0f
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 80 deletions.
diff --git a/mobile/library/common/engine_common.cc b/mobile/library/common/engine_common.cc
@@ -71,10 +71,12 @@ EngineCommon::EngineCommon(std::unique_ptr<Envoy::OptionsImpl>&& options)
          Buffer::WatermarkFactorySharedPtr watermark_factory) {
         // TODO(alyssawilk) use InstanceLite not InstanceImpl.
         auto local_address = Network::Utility::getLocalAddress(options.localAddressIpVersion());
-        return std::make_unique<Server::InstanceImpl>(
-            init_manager, options, time_system, local_address, hooks, restarter, store,
-            access_log_lock, component_factory, std::move(random_generator), tls, thread_factory,
-            file_system, std::move(process_context), watermark_factory);
+        auto server = std::make_unique<Server::InstanceImpl>(
+            init_manager, options, time_system, hooks, restarter, store, access_log_lock,
+            std::move(random_generator), tls, thread_factory, file_system,
+            std::move(process_context), watermark_factory);
+        server->initialize(local_address, component_factory);
+        return server;
       };
   base_ = std::make_unique<StrippedMainBase>(
       *options_, real_time_system_, default_listener_hooks_, prod_component_factory_,

diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc
@@ -39,10 +39,12 @@ StrippedMainBase::CreateInstanceFunction createFunction() {
          Filesystem::Instance& file_system, std::unique_ptr<ProcessContext> process_context,
          Buffer::WatermarkFactorySharedPtr watermark_factory) {
         auto local_address = Network::Utility::getLocalAddress(options.localAddressIpVersion());
-        return std::make_unique<Server::InstanceImpl>(
-            init_manager, options, time_system, local_address, hooks, restarter, store,
-            access_log_lock, component_factory, std::move(random_generator), tls, thread_factory,
-            file_system, std::move(process_context), watermark_factory);
+        auto server = std::make_unique<Server::InstanceImpl>(
+            init_manager, options, time_system, hooks, restarter, store, access_log_lock,
+            std::move(random_generator), tls, thread_factory, file_system,
+            std::move(process_context), watermark_factory);
+        server->initialize(local_address, component_factory);
+        return server;
       };
 }
 

diff --git a/source/server/server.cc b/source/server/server.cc
@@ -74,14 +74,15 @@ std::unique_ptr<ConnectionHandler> getHandler(Event::Dispatcher& dispatcher) {
 
 } // namespace
 
-InstanceImpl::InstanceImpl(
-    Init::Manager& init_manager, const Options& options, Event::TimeSystem& time_system,
-    Network::Address::InstanceConstSharedPtr local_address, ListenerHooks& hooks,
-    HotRestart& restarter, Stats::StoreRoot& store, Thread::BasicLockable& access_log_lock,
-    ComponentFactory& component_factory, Random::RandomGeneratorPtr&& random_generator,
-    ThreadLocal::Instance& tls, Thread::ThreadFactory& thread_factory,
-    Filesystem::Instance& file_system, std::unique_ptr<ProcessContext> process_context,
-    Buffer::WatermarkFactorySharedPtr watermark_factory)
+InstanceImpl::InstanceImpl(Init::Manager& init_manager, const Options& options,
+                           Event::TimeSystem& time_system, ListenerHooks& hooks,
+                           HotRestart& restarter, Stats::StoreRoot& store,
+                           Thread::BasicLockable& access_log_lock,
+                           Random::RandomGeneratorPtr&& random_generator,
+                           ThreadLocal::Instance& tls, Thread::ThreadFactory& thread_factory,
+                           Filesystem::Instance& file_system,
+                           std::unique_ptr<ProcessContext> process_context,
+                           Buffer::WatermarkFactorySharedPtr watermark_factory)
     : init_manager_(init_manager), live_(false), options_(options),
       validation_context_(options_.allowUnknownStaticFields(),
                           !options.rejectUnknownDynamicFields(),
@@ -103,43 +104,7 @@ InstanceImpl::InstanceImpl(
       grpc_context_(store.symbolTable()), http_context_(store.symbolTable()),
       router_context_(store.symbolTable()), process_context_(std::move(process_context)),
       hooks_(hooks), quic_stat_names_(store.symbolTable()), server_contexts_(*this),
-      enable_reuse_port_default_(true), stats_flush_in_progress_(false) {
-  std::function set_up_logger = [&] {
-    TRY_ASSERT_MAIN_THREAD {
-      file_logger_ = std::make_unique<Logger::FileSinkDelegate>(
-          options.logPath(), access_log_manager_, Logger::Registry::getSink());
-    }
-    END_TRY
-    CATCH(const EnvoyException& e, {
-      throw EnvoyException(
-          fmt::format("Failed to open log-file '{}'. e.what(): {}", options.logPath(), e.what()));
-    });
-  };
-
-  TRY_ASSERT_MAIN_THREAD {
-    if (!options.logPath().empty()) {
-      set_up_logger();
-    }
-    restarter_.initialize(*dispatcher_, *this);
-    drain_manager_ = component_factory.createDrainManager(*this);
-    initialize(std::move(local_address), component_factory);
-  }
-  END_TRY
-  MULTI_CATCH(
-      const EnvoyException& e,
-      {
-        ENVOY_LOG(critical, "error initializing config '{} {} {}': {}",
-                  options.configProto().DebugString(), options.configYaml(), options.configPath(),
-                  e.what());
-        terminate();
-        throw;
-      },
-      {
-        ENVOY_LOG(critical, "error initializing due to unknown exception");
-        terminate();
-        throw;
-      });
-}
+      enable_reuse_port_default_(true), stats_flush_in_progress_(false) {}
 
 InstanceImpl::~InstanceImpl() {
   terminate();
@@ -422,6 +387,45 @@ void InstanceUtil::loadBootstrapConfig(envoy::config::bootstrap::v3::Bootstrap&
 
 void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_address,
                               ComponentFactory& component_factory) {
+  std::function set_up_logger = [&] {
+    TRY_ASSERT_MAIN_THREAD {
+      file_logger_ = std::make_unique<Logger::FileSinkDelegate>(
+          options_.logPath(), access_log_manager_, Logger::Registry::getSink());
+    }
+    END_TRY
+    CATCH(const EnvoyException& e, {
+      throw EnvoyException(
+          fmt::format("Failed to open log-file '{}'. e.what(): {}", options_.logPath(), e.what()));
+    });
+  };
+
+  TRY_ASSERT_MAIN_THREAD {
+    if (!options_.logPath().empty()) {
+      set_up_logger();
+    }
+    restarter_.initialize(*dispatcher_, *this);
+    drain_manager_ = component_factory.createDrainManager(*this);
+    initializeOrThrow(std::move(local_address), component_factory);
+  }
+  END_TRY
+  MULTI_CATCH(
+      const EnvoyException& e,
+      {
+        ENVOY_LOG(critical, "error initializing config '{} {} {}': {}",
+                  options_.configProto().DebugString(), options_.configYaml(),
+                  options_.configPath(), e.what());
+        terminate();
+        throw;
+      },
+      {
+        ENVOY_LOG(critical, "error initializing due to unknown exception");
+        terminate();
+        throw;
+      });
+}
+
+void InstanceImpl::initializeOrThrow(Network::Address::InstanceConstSharedPtr local_address,
+                                     ComponentFactory& component_factory) {
   ENVOY_LOG(info, "initializing epoch {} (base id={}, hot restart version={})",
             options_.restartEpoch(), restarter_.baseId(), restarter_.version());
 

diff --git a/source/server/server.h b/source/server/server.h
@@ -233,14 +233,16 @@ class InstanceImpl final : Logger::Loggable<Logger::Id::main>,
    * @throw EnvoyException if initialization fails.
    */
   InstanceImpl(Init::Manager& init_manager, const Options& options, Event::TimeSystem& time_system,
-               Network::Address::InstanceConstSharedPtr local_address, ListenerHooks& hooks,
-               HotRestart& restarter, Stats::StoreRoot& store,
-               Thread::BasicLockable& access_log_lock, ComponentFactory& component_factory,
+               ListenerHooks& hooks, HotRestart& restarter, Stats::StoreRoot& store,
+               Thread::BasicLockable& access_log_lock,
                Random::RandomGeneratorPtr&& random_generator, ThreadLocal::Instance& tls,
                Thread::ThreadFactory& thread_factory, Filesystem::Instance& file_system,
                std::unique_ptr<ProcessContext> process_context,
                Buffer::WatermarkFactorySharedPtr watermark_factory = nullptr);
 
+  // initialize the server. This must be called before run().
+  void initialize(Network::Address::InstanceConstSharedPtr local_address,
+                  ComponentFactory& component_factory);
   ~InstanceImpl() override;
 
   void run() override;
@@ -313,8 +315,10 @@ class InstanceImpl final : Logger::Loggable<Logger::Id::main>,
   ProtobufTypes::MessagePtr dumpBootstrapConfig();
   void flushStatsInternal();
   void updateServerStats();
-  void initialize(Network::Address::InstanceConstSharedPtr local_address,
-                  ComponentFactory& component_factory);
+  // This does most of the work of initialization, but can throw errors caught
+  // by initialize().
+  void initializeOrThrow(Network::Address::InstanceConstSharedPtr local_address,
+                         ComponentFactory& component_factory);
   void loadServerFlags(const absl::optional<std::string>& flags_path);
   void startWorkers();
   void terminate();

diff --git a/test/integration/server.cc b/test/integration/server.cc
@@ -232,11 +232,11 @@ void IntegrationTestServerImpl::createAndRunEnvoyServer(
     if (process_object.has_value()) {
       process_context = std::make_unique<ProcessContextImpl>(process_object->get());
     }
-    Server::InstanceImpl server(init_manager, options, time_system, local_address, hooks, restarter,
-                                stat_store, access_log_lock, component_factory,
-                                std::move(random_generator), tls, Thread::threadFactoryForTest(),
-                                Filesystem::fileSystemForTest(), std::move(process_context),
-                                watermark_factory);
+    Server::InstanceImpl server(init_manager, options, time_system, hooks, restarter, stat_store,
+                                access_log_lock, std::move(random_generator), tls,
+                                Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(),
+                                std::move(process_context), watermark_factory);
+    server.initialize(local_address, component_factory);
     // This is technically thread unsafe (assigning to a shared_ptr accessed
     // across threads), but because we synchronize below through serverReady(), the only
     // consumer on the main test thread in ~IntegrationTestServerImpl will not race.

diff --git a/test/server/server_fuzz_test.cc b/test/server/server_fuzz_test.cc
@@ -152,11 +152,11 @@ DEFINE_PROTO_FUZZER(const envoy::config::bootstrap::v3::Bootstrap& input) {
   std::unique_ptr<InstanceImpl> server;
   try {
     server = std::make_unique<InstanceImpl>(
-        init_manager, options, test_time.timeSystem(),
-        std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1"), hooks, restart, stats_store,
-        fakelock, component_factory, std::make_unique<Random::RandomGeneratorImpl>(),
-        thread_local_instance, Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(),
-        nullptr);
+        init_manager, options, test_time.timeSystem(), hooks, restart, stats_store, fakelock,
+        std::make_unique<Random::RandomGeneratorImpl>(), thread_local_instance,
+        Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(), nullptr);
+    server->initialize(std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1"),
+                       component_factory);
   } catch (const EnvoyException& ex) {
     ENVOY_LOG_MISC(debug, "Controlled EnvoyException exit: {}", ex.what());
     return;

diff --git a/test/server/server_test.cc b/test/server/server_test.cc
@@ -258,12 +258,12 @@ class ServerInstanceImplTestBase {
                                              : std::make_unique<Init::ManagerImpl>("Server");
 
     server_ = std::make_unique<InstanceImpl>(
-        *init_manager_, options_, time_system_,
-        std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1"), hooks, restart_,
-        stats_store_, fakelock_, component_factory_,
+        *init_manager_, options_, time_system_, hooks, restart_, stats_store_, fakelock_,
         std::make_unique<NiceMock<Random::MockRandomGenerator>>(), *thread_local_,
         Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(),
         std::move(process_context_));
+    server_->initialize(std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1"),
+                        component_factory_);
     EXPECT_TRUE(server_->api().fileSystem().fileExists(std::string(Platform::null_device_path)));
   }
 
@@ -277,11 +277,11 @@ class ServerInstanceImplTestBase {
     thread_local_ = std::make_unique<ThreadLocal::InstanceImpl>();
     init_manager_ = std::make_unique<Init::ManagerImpl>("Server");
     server_ = std::make_unique<InstanceImpl>(
-        *init_manager_, options_, time_system_,
-        std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1"), hooks_, restart_,
-        stats_store_, fakelock_, component_factory_,
+        *init_manager_, options_, time_system_, hooks_, restart_, stats_store_, fakelock_,
         std::make_unique<NiceMock<Random::MockRandomGenerator>>(), *thread_local_,
         Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(), nullptr);
+    server_->initialize(std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1"),
+                        component_factory_);
 
     EXPECT_TRUE(server_->api().fileSystem().fileExists(std::string(Platform::null_device_path)));
   }
@@ -1302,13 +1302,13 @@ TEST_P(ServerInstanceImplTest, LogToFileError) {
 TEST_P(ServerInstanceImplTest, NoOptionsPassed) {
   thread_local_ = std::make_unique<ThreadLocal::InstanceImpl>();
   init_manager_ = std::make_unique<Init::ManagerImpl>("Server");
+  server_.reset(new InstanceImpl(
+      *init_manager_, options_, time_system_, hooks_, restart_, stats_store_, fakelock_,
+      std::make_unique<NiceMock<Random::MockRandomGenerator>>(), *thread_local_,
+      Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(), nullptr));
   EXPECT_THROW_WITH_MESSAGE(
-      server_.reset(new InstanceImpl(*init_manager_, options_, time_system_,
-                                     std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1"),
-                                     hooks_, restart_, stats_store_, fakelock_, component_factory_,
-                                     std::make_unique<NiceMock<Random::MockRandomGenerator>>(),
-                                     *thread_local_, Thread::threadFactoryForTest(),
-                                     Filesystem::fileSystemForTest(), nullptr)),
+      server_->initialize(std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1"),
+                          component_factory_),
       EnvoyException,
       "At least one of --config-path or --config-yaml or Options::configProto() should be "
       "non-empty");