From d0ed5a1b23a34f93842eac457474e709b1d4f9b2 Mon Sep 17 00:00:00 2001
From: Andres Guedez <aguedez@google.com>
Date: Fri, 19 Apr 2019 15:11:45 -0400
Subject: [PATCH 1/2] Remove ThreadFactorySingleton.

It is no longer needed since Api::Api is plumbed ubiquitiously throughout Envoy's core.

The only user of the factory, QuicThreadImpl, has been modified to take the
Envoy::Thread::ThreadFactory via QuicThreadImpl::setThreadFactory().

Signed-off-by: Andres Guedez <aguedez@google.com>
---
 include/envoy/thread/thread.h                 | 28 ---------------
 source/common/thread/BUILD                    | 18 ----------
 .../common/thread/thread_factory_singleton.cc | 21 -----------
 source/exe/BUILD                              |  1 -
 source/exe/main_common.cc                     |  6 +---
 .../quiche/platform/quic_thread_impl.h        |  8 ++++-
 test/BUILD                                    |  1 -
 test/common/thread/BUILD                      | 18 ----------
 .../thread/thread_factory_singleton_test.cc   | 35 -------------------
 test/exe/main_common_test.cc                  | 15 +-------
 .../quic_listeners/quiche/platform/BUILD      |  3 +-
 .../quiche/platform/quic_platform_test.cc     | 16 ++++++---
 test/main.cc                                  |  1 -
 13 files changed, 22 insertions(+), 149 deletions(-)
 delete mode 100644 source/common/thread/BUILD
 delete mode 100644 source/common/thread/thread_factory_singleton.cc
 delete mode 100644 test/common/thread/BUILD
 delete mode 100644 test/common/thread/thread_factory_singleton_test.cc

diff --git a/include/envoy/thread/thread.h b/include/envoy/thread/thread.h
index 6bde21178f849..e9078afa476ba 100644
--- a/include/envoy/thread/thread.h
+++ b/include/envoy/thread/thread.h
@@ -51,34 +51,6 @@ class ThreadFactory {
   virtual ThreadIdPtr currentThreadId() PURE;
 };
 
-/**
- * A static singleton to the ThreadFactory corresponding to the build platform.
- *
- * The singleton must be initialized via set() early in main() with the appropriate ThreadFactory
- * (see source/exe/{posix,win32}/platform_impl.h).
- *
- * This static singleton is an exception to Envoy's established practice for handling of singletons,
- * which are typically registered with and accessed via the Envoy::Singleton::Manager. Reasons for
- * the exception include drastic simplification of thread safety assertions; e.g.:
- *   ASSERT(ThreadFactorySingleton::get()->currentThreadId() == original_thread_id_);
- */
-class ThreadFactorySingleton {
-public:
-  /**
-   * Returns a reference to the platform dependent ThreadFactory.
-   */
-  static ThreadFactory& get() { return *thread_factory_; }
-
-  /**
-   * Sets the singleton to the supplied thread_factory.
-   * @param thread_factory the ThreadFactory instance to be pointed to by this singleton.
-   */
-  static void set(ThreadFactory* thread_factory);
-
-private:
-  static ThreadFactory* thread_factory_;
-};
-
 /**
  * Like the C++11 "basic lockable concept" but a pure virtual interface vs. a template, and
  * with thread annotations.
diff --git a/source/common/thread/BUILD b/source/common/thread/BUILD
deleted file mode 100644
index 8ba062af10205..0000000000000
--- a/source/common/thread/BUILD
+++ /dev/null
@@ -1,18 +0,0 @@
-licenses(["notice"])  # Apache 2
-
-load(
-    "//bazel:envoy_build_system.bzl",
-    "envoy_cc_library",
-    "envoy_package",
-)
-
-envoy_package()
-
-envoy_cc_library(
-    name = "thread_factory_singleton_lib",
-    srcs = ["thread_factory_singleton.cc"],
-    deps = [
-        "//include/envoy/thread:thread_interface",
-        "//source/common/common:assert_lib",
-    ],
-)
diff --git a/source/common/thread/thread_factory_singleton.cc b/source/common/thread/thread_factory_singleton.cc
deleted file mode 100644
index c3c8d8a62e883..0000000000000
--- a/source/common/thread/thread_factory_singleton.cc
+++ /dev/null
@@ -1,21 +0,0 @@
-#include "envoy/thread/thread.h"
-
-#include "common/common/assert.h"
-
-namespace Envoy {
-namespace Thread {
-
-ThreadFactory* ThreadFactorySingleton::thread_factory_{nullptr};
-
-// This function can not be inlined in the thread.h header due to the use of ASSERT() creating a
-// circular dependency with assert.h.
-void ThreadFactorySingleton::set(ThreadFactory* thread_factory) {
-  // Verify that either the singleton is uninitialized (i.e., thread_factory_ == nullptr) OR it's
-  // being reset to the uninitialized state (i.e., thread_factory == nullptr), but _not_ both. The
-  // use of XOR complicates tests but improves our ability to catch init/cleanup errors.
-  ASSERT((thread_factory == nullptr) != (thread_factory_ == nullptr));
-  thread_factory_ = thread_factory;
-}
-
-} // namespace Thread
-} // namespace Envoy
diff --git a/source/exe/BUILD b/source/exe/BUILD
index bcce85238a6b8..7e4d98a84b030 100644
--- a/source/exe/BUILD
+++ b/source/exe/BUILD
@@ -71,7 +71,6 @@ envoy_cc_library(
         "//source/common/http/http2:codec_lib",
         "//source/common/common:perf_annotation_lib",
         "//source/common/stats:fake_symbol_table_lib",
-        "//source/common/thread:thread_factory_singleton_lib",
         "//source/server:hot_restart_lib",
         "//source/server:hot_restart_nop_lib",
         "//source/server:proto_descriptors_lib",
diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc
index 53f828142c2e5..2dd60ef2033e8 100644
--- a/source/exe/main_common.cc
+++ b/source/exe/main_common.cc
@@ -49,7 +49,6 @@ MainCommonBase::MainCommonBase(const OptionsImpl& options, Event::TimeSystem& ti
                                Filesystem::Instance& file_system)
     : options_(options), component_factory_(component_factory), thread_factory_(thread_factory),
       file_system_(file_system) {
-  Thread::ThreadFactorySingleton::set(&thread_factory_);
   ares_library_init(ARES_LIB_INIT_ALL);
   Event::Libevent::Global::initialize();
   RELEASE_ASSERT(Envoy::Server::validateProtoDescriptors(), "");
@@ -98,10 +97,7 @@ MainCommonBase::MainCommonBase(const OptionsImpl& options, Event::TimeSystem& ti
   }
 }
 
-MainCommonBase::~MainCommonBase() {
-  Thread::ThreadFactorySingleton::set(nullptr);
-  ares_library_cleanup();
-}
+MainCommonBase::~MainCommonBase() { ares_library_cleanup(); }
 
 void MainCommonBase::configureComponentLogLevels() {
   for (auto& component_log_level : options_.componentLogLevels()) {
diff --git a/source/extensions/quic_listeners/quiche/platform/quic_thread_impl.h b/source/extensions/quic_listeners/quiche/platform/quic_thread_impl.h
index 42e84bb27c7dc..3fecc809ee59a 100644
--- a/source/extensions/quic_listeners/quiche/platform/quic_thread_impl.h
+++ b/source/extensions/quic_listeners/quiche/platform/quic_thread_impl.h
@@ -30,10 +30,11 @@ class QuicThreadImpl {
   }
 
   void Start() {
+    ASSERT(thread_factory_ != nullptr);
     if (thread_ != nullptr || thread_is_set_.HasBeenNotified()) {
       PANIC("QuicThread can only be started once.");
     }
-    thread_ = Envoy::Thread::ThreadFactorySingleton::get().createThread([this]() {
+    thread_ = thread_factory_->createThread([this]() {
       thread_is_set_.WaitForNotification();
       this->Run();
     });
@@ -48,6 +49,10 @@ class QuicThreadImpl {
     thread_ = nullptr;
   }
 
+  void setThreadFactory(Envoy::Thread::ThreadFactory& thread_factory) {
+    thread_factory_ = &thread_factory;
+  }
+
 protected:
   virtual void Run() {
     // We don't want this function to be pure virtual, because it will be called if:
@@ -61,6 +66,7 @@ class QuicThreadImpl {
 
 private:
   Envoy::Thread::ThreadPtr thread_;
+  Envoy::Thread::ThreadFactory* thread_factory_;
   absl::Notification thread_is_set_; // Whether |thread_| is set in parent.
 };
 
diff --git a/test/BUILD b/test/BUILD
index 9e194a76dcf2d..6584424aaa4f2 100644
--- a/test/BUILD
+++ b/test/BUILD
@@ -30,7 +30,6 @@ envoy_cc_test_library(
         "//source/common/common:thread_lib",
         "//source/common/event:libevent_lib",
         "//source/common/http/http2:codec_lib",
-        "//source/common/thread:thread_factory_singleton_lib",
         "//test/common/runtime:utility_lib",
         "//test/mocks/access_log:access_log_mocks",
         "//test/test_common:environment_lib",
diff --git a/test/common/thread/BUILD b/test/common/thread/BUILD
deleted file mode 100644
index 382cca7904df0..0000000000000
--- a/test/common/thread/BUILD
+++ /dev/null
@@ -1,18 +0,0 @@
-licenses(["notice"])  # Apache 2
-
-load(
-    "//bazel:envoy_build_system.bzl",
-    "envoy_cc_test",
-    "envoy_package",
-)
-
-envoy_package()
-
-envoy_cc_test(
-    name = "thread_factory_singleton_test",
-    srcs = ["thread_factory_singleton_test.cc"],
-    deps = [
-        "//source/common/common:assert_lib",
-        "//source/common/thread:thread_factory_singleton_lib",
-    ],
-)
diff --git a/test/common/thread/thread_factory_singleton_test.cc b/test/common/thread/thread_factory_singleton_test.cc
deleted file mode 100644
index d50fc69ee417c..0000000000000
--- a/test/common/thread/thread_factory_singleton_test.cc
+++ /dev/null
@@ -1,35 +0,0 @@
-#include "envoy/thread/thread.h"
-
-#include "common/common/assert.h"
-
-#include "gmock/gmock.h"
-#include "gtest/gtest.h"
-
-namespace Envoy {
-namespace Thread {
-namespace {
-
-class ThreadFactorySingletonTest : public testing::Test {
-protected:
-  ThreadFactorySingletonTest()
-      : run_tid_(Envoy::Thread::ThreadFactorySingleton::get().currentThreadId()) {}
-
-  bool checkThreadId() const { return run_tid_->isCurrentThreadId(); };
-
-  ThreadIdPtr run_tid_;
-};
-
-// Verify that Thread::threadFactorySingleton is defined and initialized for tests.
-TEST_F(ThreadFactorySingletonTest, IsCurrentThread) {
-  // Use std::thread instead of the ThreadFactory's createThread() to avoid the dependency on the
-  // code under test.
-  bool is_current = checkThreadId();
-  EXPECT_TRUE(is_current);
-  std::thread thread([this, &is_current]() { is_current = checkThreadId(); });
-  thread.join();
-  EXPECT_FALSE(is_current) << "run_tid_->isCurrentThreadId() from inside another thread";
-}
-
-} // namespace
-} // namespace Thread
-} // namespace Envoy
diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc
index 2b1aee544fec3..44c693a91f8e6 100644
--- a/test/exe/main_common_test.cc
+++ b/test/exe/main_common_test.cc
@@ -40,20 +40,7 @@ class MainCommonTest : public testing::TestWithParam<Network::Address::IpVersion
             TestEnvironment::PortMap(), GetParam())),
         random_string_(fmt::format("{}", computeBaseId())),
         argv_({"envoy-static", "--base-id", random_string_.c_str(), "-c", config_file_.c_str(),
-               nullptr}) {
-    // The test main() sets the ThreadFactorySingleton since it is required by all other tests not
-    // instantiating their own MainCommon.
-    // Reset the singleton to a nullptr to avoid triggering an assertion when MainCommonBase() calls
-    // set() in the tests below.
-    Thread::ThreadFactorySingleton::set(nullptr);
-  }
-
-  ~MainCommonTest() override {
-    // This is ugly, but necessary to enable a stronger ASSERT() in ThreadFactorySingleton::set().
-    // The singleton needs to be reset to a non nullptr value such that when the constructor runs
-    // again, the ThreadFactorySingleton::set(nullptr) does not trigger the assertion.
-    Thread::ThreadFactorySingleton::set(&Thread::threadFactoryForTest());
-  }
+               nullptr}) {}
 
   /**
    * Computes a numeric ID to incorporate into the names of
diff --git a/test/extensions/quic_listeners/quiche/platform/BUILD b/test/extensions/quic_listeners/quiche/platform/BUILD
index d2ec6aa11f9b1..0f7a8b80c6a72 100644
--- a/test/extensions/quic_listeners/quiche/platform/BUILD
+++ b/test/extensions/quic_listeners/quiche/platform/BUILD
@@ -3,6 +3,7 @@ licenses(["notice"])  # Apache 2
 load(
     "//bazel:envoy_build_system.bzl",
     "envoy_cc_fuzz_test",
+    "envoy_cc_platform_dep",
     "envoy_cc_test",
     "envoy_cc_test_binary",
     "envoy_cc_test_library",
@@ -40,7 +41,7 @@ envoy_cc_test(
         "//test/test_common:utility_lib",
         "@com_googlesource_quiche//:quic_platform_port_utils",
         "@com_googlesource_quiche//:quic_platform_sleep",
-    ],
+    ] + envoy_cc_platform_dep("//source/exe:platform_impl_lib"),
 )
 
 envoy_cc_test(
diff --git a/test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc b/test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc
index df87edca85dd4..bf341723b94a0 100644
--- a/test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc
+++ b/test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc
@@ -12,6 +12,8 @@
 #include "common/memory/stats.h"
 #include "common/network/utility.h"
 
+#include "exe/platform_impl.h"
+
 #include "test/common/stats/stat_test_utility.h"
 #include "test/extensions/transport_sockets/tls/ssl_test_utility.h"
 #include "test/mocks/api/mocks.h"
@@ -252,10 +254,14 @@ TEST_F(QuicPlatformTest, QuicStringPiece) {
 }
 
 TEST_F(QuicPlatformTest, QuicThread) {
+  Envoy::PlatformImpl platform_impl;
+
   class AdderThread : public QuicThread {
   public:
-    AdderThread(int* value, int increment)
-        : QuicThread("adder_thread"), value_(value), increment_(increment) {}
+    AdderThread(int* value, int increment, Envoy::Thread::ThreadFactory& thread_factory)
+        : QuicThread("adder_thread"), value_(value), increment_(increment) {
+      setThreadFactory(thread_factory);
+    }
 
     ~AdderThread() override = default;
 
@@ -270,19 +276,19 @@ TEST_F(QuicPlatformTest, QuicThread) {
   int value = 0;
 
   // A QuicThread that is never started, which is ok.
-  { AdderThread t0(&value, 1); }
+  { AdderThread t0(&value, 1, platform_impl.threadFactory()); }
   EXPECT_EQ(0, value);
 
   // A QuicThread that is started and joined as usual.
   {
-    AdderThread t1(&value, 1);
+    AdderThread t1(&value, 1, platform_impl.threadFactory());
     t1.Start();
     t1.Join();
   }
   EXPECT_EQ(1, value);
 
   // QuicThread will panic if it's started but not joined.
-  EXPECT_DEATH_LOG_TO_STDERR({ AdderThread(&value, 2).Start(); },
+  EXPECT_DEATH_LOG_TO_STDERR({ AdderThread(&value, 2, platform_impl.threadFactory()).Start(); },
                              "QuicThread should be joined before destruction");
 }
 
diff --git a/test/main.cc b/test/main.cc
index caecf19b1f625..83a9d2b72c9f2 100644
--- a/test/main.cc
+++ b/test/main.cc
@@ -21,7 +21,6 @@ int main(int argc, char** argv) {
   // Enabled by default. Control with "bazel --define=signal_trace=disabled"
   Envoy::SignalAction handle_sigs;
 #endif
-  Envoy::Thread::ThreadFactorySingleton::set(&Envoy::Thread::threadFactoryForTest());
 
   Envoy::TestEnvironment::setEnvVar("TEST_RUNDIR",
                                     (Envoy::TestEnvironment::getCheckedEnvVar("TEST_SRCDIR") + "/" +

From 2383bd936945b56ab32a705290bd9c81d4b174a8 Mon Sep 17 00:00:00 2001
From: Andres Guedez <aguedez@google.com>
Date: Fri, 19 Apr 2019 16:32:07 -0400
Subject: [PATCH 2/2] Add comment clarifying dependency injection via member
 function.

Signed-off-by: Andres Guedez <aguedez@google.com>
---
 .../quic_listeners/quiche/platform/quic_thread_impl.h         | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/source/extensions/quic_listeners/quiche/platform/quic_thread_impl.h b/source/extensions/quic_listeners/quiche/platform/quic_thread_impl.h
index 3fecc809ee59a..bf2b63419a5b4 100644
--- a/source/extensions/quic_listeners/quiche/platform/quic_thread_impl.h
+++ b/source/extensions/quic_listeners/quiche/platform/quic_thread_impl.h
@@ -49,6 +49,10 @@ class QuicThreadImpl {
     thread_ = nullptr;
   }
 
+  // Sets the thread factory to use.
+  // NOTE: The factory can not be passed via a constructor argument because this class is itself a
+  // dependency of an external library that derives from it and expects a single argument
+  // constructor.
   void setThreadFactory(Envoy::Thread::ThreadFactory& thread_factory) {
     thread_factory_ = &thread_factory;
   }