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..bf2b63419a5b4 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,14 @@ 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; + } + protected: virtual void Run() { // We don't want this function to be pure virtual, because it will be called if: @@ -61,6 +70,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