Mark Server::InstanceImpl final (envoyproxy#8510)

Signed-off-by: Yuchen Dai [email protected] Description: Refactor InstanceImpl and remove the only derived test class of InstanceImpl Mark entire Server::InstanceImpl as final to avoid accidentally invoke virtual function in InstanceImpl constructor. Massage clang-tidy. Fixes envoyproxy#8509 Signed-off-by: Yuchen Dai <[email protected]>
nandu-vinodan · Oct 17, 2019 · ab33dd7 · ab33dd7
1 parent 9d53ed2
commit ab33dd7
Show file tree

Hide file tree

Showing 7 changed files with 38 additions and 63 deletions.
diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc
@@ -77,9 +77,9 @@ MainCommonBase::MainCommonBase(const OptionsImpl& options, Event::TimeSystem& ti
     stats_store_ = std::make_unique<Stats::ThreadLocalStoreImpl>(stats_allocator_);
 
     server_ = std::make_unique<Server::InstanceImpl>(
-        options_, time_system, local_address, listener_hooks, *restarter_, *stats_store_,
-        access_log_lock, component_factory, std::move(random_generator), *tls_, thread_factory_,
-        file_system_, std::move(process_context));
+        *init_manager_, options_, time_system, local_address, listener_hooks, *restarter_,
+        *stats_store_, access_log_lock, component_factory, std::move(random_generator), *tls_,
+        thread_factory_, file_system_, std::move(process_context));
 
     break;
   }

diff --git a/source/exe/main_common.h b/source/exe/main_common.h
@@ -82,6 +82,7 @@ class MainCommonBase {
   std::unique_ptr<Server::HotRestart> restarter_;
   std::unique_ptr<Stats::ThreadLocalStoreImpl> stats_store_;
   std::unique_ptr<Logger::Context> logging_context_;
+  std::unique_ptr<Init::Manager> init_manager_{std::make_unique<Init::ManagerImpl>("Server")};
   std::unique_ptr<Server::InstanceImpl> server_;
 
 private:

diff --git a/source/server/server.cc b/source/server/server.cc
@@ -47,16 +47,14 @@
 namespace Envoy {
 namespace Server {
 
-InstanceImpl::InstanceImpl(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,
-                           Runtime::RandomGeneratorPtr&& random_generator,
-                           ThreadLocal::Instance& tls, Thread::ThreadFactory& thread_factory,
-                           Filesystem::Instance& file_system,
-                           std::unique_ptr<ProcessContext> process_context)
-    : workers_started_(false), shutdown_(false), options_(options),
+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, Runtime::RandomGeneratorPtr&& random_generator,
+    ThreadLocal::Instance& tls, Thread::ThreadFactory& thread_factory,
+    Filesystem::Instance& file_system, std::unique_ptr<ProcessContext> process_context)
+    : init_manager_(init_manager), workers_started_(false), shutdown_(false), options_(options),
       validation_context_(options_.allowUnknownStaticFields(),
                           !options.rejectUnknownDynamicFields()),
       time_source_(time_system), restarter_(restarter), start_time_(time(nullptr)),

diff --git a/source/server/server.h b/source/server/server.h
@@ -144,14 +144,14 @@ class RunHelper : Logger::Loggable<Logger::Id::main> {
 /**
  * This is the actual full standalone server which stitches together various common components.
  */
-class InstanceImpl : Logger::Loggable<Logger::Id::main>,
-                     public Instance,
-                     public ServerLifecycleNotifier {
+class InstanceImpl final : Logger::Loggable<Logger::Id::main>,
+                           public Instance,
+                           public ServerLifecycleNotifier {
 public:
   /**
    * @throw EnvoyException if initialization fails.
    */
-  InstanceImpl(const Options& options, Event::TimeSystem& time_system,
+  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,
@@ -227,7 +227,7 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>,
   // init_manager_ must come before any member that participates in initialization, and destructed
   // only after referencing members are gone, since initialization continuation can potentially
   // occur at any point during member lifetime. This init manager is populated with LdsApi targets.
-  Init::ManagerImpl init_manager_{"Server"};
+  Init::Manager& init_manager_;
   // secret_manager_ must come before listener_manager_, config_ and dispatcher_, and destructed
   // only after these members can no longer reference it, since:
   // - There may be active filter chains referencing it in listener_manager_.

diff --git a/test/integration/server.cc b/test/integration/server.cc
@@ -188,6 +188,7 @@ void IntegrationTestServerImpl::createAndRunEnvoyServer(
     Runtime::RandomGeneratorPtr&& random_generator,
     absl::optional<std::reference_wrapper<ProcessObject>> process_object) {
   {
+    Init::ManagerImpl init_manager{"Server"};
     Stats::SymbolTablePtr symbol_table = Stats::SymbolTableCreator::makeSymbolTable();
     Server::HotRestartNopImpl restarter;
     ThreadLocal::InstanceImpl tls;
@@ -197,9 +198,9 @@ void IntegrationTestServerImpl::createAndRunEnvoyServer(
     if (process_object.has_value()) {
       process_context = std::make_unique<ProcessContextImpl>(process_object->get());
     }
-    Server::InstanceImpl server(options, time_system, local_address, hooks, restarter, stat_store,
-                                access_log_lock, component_factory, std::move(random_generator),
-                                tls, Thread::threadFactoryForTest(),
+    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));
     // This is technically thread unsafe (assigning to a shared_ptr accessed
     // across threads), but because we synchronize below through serverReady(), the only

diff --git a/test/server/server_fuzz_test.cc b/test/server/server_fuzz_test.cc
@@ -72,6 +72,7 @@ DEFINE_PROTO_FUZZER(const envoy::config::bootstrap::v2::Bootstrap& input) {
   ThreadLocal::InstanceImpl thread_local_instance;
   DangerousDeprecatedTestTime test_time;
   Fuzz::PerTestEnvironment test_env;
+  Init::ManagerImpl init_manager{"Server"};
 
   {
     const std::string bootstrap_path = test_env.temporaryPath("bootstrap.pb_text");
@@ -84,7 +85,7 @@ DEFINE_PROTO_FUZZER(const envoy::config::bootstrap::v2::Bootstrap& input) {
   std::unique_ptr<InstanceImpl> server;
   try {
     server = std::make_unique<InstanceImpl>(
-        options, test_time.timeSystem(),
+        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<Runtime::RandomGeneratorImpl>(),
         thread_local_instance, Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(),

diff --git a/test/server/server_test.cc b/test/server/server_test.cc
@@ -141,27 +141,6 @@ class InitializingInitManager : public Init::ManagerImpl {
   State state() const override { return State::Initializing; }
 };
 
-class InitializingInstanceImpl : public InstanceImpl {
-private:
-  InitializingInitManager init_manager_{"Server"};
-
-public:
-  InitializingInstanceImpl(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,
-                           Runtime::RandomGeneratorPtr&& random_generator,
-                           ThreadLocal::Instance& tls, Thread::ThreadFactory& thread_factory,
-                           Filesystem::Instance& file_system,
-                           std::unique_ptr<ProcessContext> process_context)
-      : InstanceImpl(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)) {}
-
-  Init::Manager& initManager() override { return init_manager_; }
-};
-
 // Class creates minimally viable server instance for testing.
 class ServerInstanceImplTestBase {
 protected:
@@ -176,25 +155,16 @@ class ServerInstanceImplTestBase {
     if (process_object_ != nullptr) {
       process_context_ = std::make_unique<ProcessContextImpl>(*process_object_);
     }
-    if (use_intializing_instance) {
-      server_ = std::make_unique<InitializingInstanceImpl>(
-          options_, test_time_.timeSystem(),
-          Network::Address::InstanceConstSharedPtr(new Network::Address::Ipv4Instance("127.0.0.1")),
-          hooks_, restart_, stats_store_, fakelock_, component_factory_,
-          std::make_unique<NiceMock<Runtime::MockRandomGenerator>>(), *thread_local_,
-          Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(),
-          std::move(process_context_));
-
-    } else {
-      server_ = std::make_unique<InstanceImpl>(
-          options_, test_time_.timeSystem(),
-          Network::Address::InstanceConstSharedPtr(new Network::Address::Ipv4Instance("127.0.0.1")),
-          hooks_, restart_, stats_store_, fakelock_, component_factory_,
-          std::make_unique<NiceMock<Runtime::MockRandomGenerator>>(), *thread_local_,
-          Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(),
-          std::move(process_context_));
-    }
+    init_manager_ = use_intializing_instance ? std::make_unique<InitializingInitManager>("Server")
+                                             : std::make_unique<Init::ManagerImpl>("Server");
 
+    server_ = std::make_unique<InstanceImpl>(
+        *init_manager_, options_, test_time_.timeSystem(),
+        Network::Address::InstanceConstSharedPtr(new Network::Address::Ipv4Instance("127.0.0.1")),
+        hooks_, restart_, stats_store_, fakelock_, component_factory_,
+        std::make_unique<NiceMock<Runtime::MockRandomGenerator>>(), *thread_local_,
+        Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(),
+        std::move(process_context_));
     EXPECT_TRUE(server_->api().fileSystem().fileExists("/dev/null"));
   }
 
@@ -206,8 +176,9 @@ class ServerInstanceImplTestBase {
          {"health_check_interval", fmt::format("{}", interval).c_str()}},
         TestEnvironment::PortMap{}, version_);
     thread_local_ = std::make_unique<ThreadLocal::InstanceImpl>();
+    init_manager_ = std::make_unique<Init::ManagerImpl>("Server");
     server_ = std::make_unique<InstanceImpl>(
-        options_, test_time_.timeSystem(),
+        *init_manager_, options_, test_time_.timeSystem(),
         Network::Address::InstanceConstSharedPtr(new Network::Address::Ipv4Instance("127.0.0.1")),
         hooks_, restart_, stats_store_, fakelock_, component_factory_,
         std::make_unique<NiceMock<Runtime::MockRandomGenerator>>(), *thread_local_,
@@ -256,6 +227,8 @@ class ServerInstanceImplTestBase {
   DangerousDeprecatedTestTime test_time_;
   ProcessObject* process_object_ = nullptr;
   std::unique_ptr<ProcessContextImpl> process_context_;
+  std::unique_ptr<Init::Manager> init_manager_;
+
   std::unique_ptr<InstanceImpl> server_;
 };
 
@@ -781,9 +754,10 @@ TEST_P(ServerInstanceImplTest, LogToFileError) {
 // an empty config.
 TEST_P(ServerInstanceImplTest, NoOptionsPassed) {
   thread_local_ = std::make_unique<ThreadLocal::InstanceImpl>();
+  init_manager_ = std::make_unique<Init::ManagerImpl>("Server");
   EXPECT_THROW_WITH_MESSAGE(
       server_.reset(new InstanceImpl(
-          options_, test_time_.timeSystem(),
+          *init_manager_, options_, test_time_.timeSystem(),
           Network::Address::InstanceConstSharedPtr(new Network::Address::Ipv4Instance("127.0.0.1")),
           hooks_, restart_, stats_store_, fakelock_, component_factory_,
           std::make_unique<NiceMock<Runtime::MockRandomGenerator>>(), *thread_local_,