diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 72c5c6e9d87c..be0825a10619 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -419,6 +419,10 @@ def _com_google_absl(): name = "abseil_hash", actual = "@com_google_absl//absl/hash:hash", ) + native.bind( + name = "abseil_hash_testing", + actual = "@com_google_absl//absl/hash:hash_testing", + ) native.bind( name = "abseil_inlined_vector", actual = "@com_google_absl//absl/container:inlined_vector", diff --git a/include/envoy/server/guarddog.h b/include/envoy/server/guarddog.h index b17c55379aac..4386aa7f9051 100644 --- a/include/envoy/server/guarddog.h +++ b/include/envoy/server/guarddog.h @@ -26,9 +26,9 @@ class GuardDog { * to avoid triggering the GuardDog. If no longer needed use the * stopWatching() method to remove it from the list of watched objects. * - * @param thread_id a Thread::ThreadIdPtr containing the system thread id + * @param thread_id a Thread::ThreadId containing the system thread id */ - virtual WatchDogSharedPtr createWatchDog(Thread::ThreadIdPtr&& thread_id) PURE; + virtual WatchDogSharedPtr createWatchDog(Thread::ThreadId thread_id) PURE; /** * Tell the GuardDog to forget about this WatchDog. diff --git a/include/envoy/server/watchdog.h b/include/envoy/server/watchdog.h index f60755784582..d230ab48f6fb 100644 --- a/include/envoy/server/watchdog.h +++ b/include/envoy/server/watchdog.h @@ -36,7 +36,7 @@ class WatchDog { * This can be used if this is later used on a thread where there is no dispatcher. */ virtual void touch() PURE; - virtual const Thread::ThreadId& threadId() const PURE; + virtual Thread::ThreadId threadId() const PURE; virtual MonotonicTime lastTouchTime() const PURE; }; diff --git a/include/envoy/thread/thread.h b/include/envoy/thread/thread.h index 41dd7291ee07..8d630d8ac134 100644 --- a/include/envoy/thread/thread.h +++ b/include/envoy/thread/thread.h @@ -1,7 +1,9 @@ #pragma once #include +#include #include +#include #include "envoy/common/pure.h" @@ -10,16 +12,26 @@ namespace Envoy { namespace Thread { +/** + * An id for a thread. + */ class ThreadId { public: - virtual ~ThreadId() = default; - - virtual std::string debugString() const PURE; - virtual bool isCurrentThreadId() const PURE; + ThreadId() : id_(std::numeric_limits::min()) {} + explicit ThreadId(int64_t id) : id_(id) {} + + std::string debugString() const { return std::to_string(id_); } + bool isEmpty() const { return *this == ThreadId(); } + friend bool operator==(ThreadId lhs, ThreadId rhs) { return lhs.id_ == rhs.id_; } + friend bool operator!=(ThreadId lhs, ThreadId rhs) { return lhs.id_ != rhs.id_; } + template friend H AbslHashValue(H h, ThreadId id) { + return H::combine(std::move(h), id.id_); + } + +private: + int64_t id_; }; -using ThreadIdPtr = std::unique_ptr; - class Thread { public: virtual ~Thread() = default; @@ -48,7 +60,7 @@ class ThreadFactory { /** * Return the current system thread ID */ - virtual ThreadIdPtr currentThreadId() PURE; + virtual ThreadId currentThreadId() PURE; }; /** diff --git a/source/common/common/posix/thread_impl.cc b/source/common/common/posix/thread_impl.cc index 463ae4745ef7..324230ade176 100644 --- a/source/common/common/posix/thread_impl.cc +++ b/source/common/common/posix/thread_impl.cc @@ -24,14 +24,8 @@ int64_t getCurrentThreadId() { } // namespace -ThreadIdImplPosix::ThreadIdImplPosix(int64_t id) : id_(id) {} - -std::string ThreadIdImplPosix::debugString() const { return std::to_string(id_); } - -bool ThreadIdImplPosix::isCurrentThreadId() const { return id_ == getCurrentThreadId(); } - ThreadImplPosix::ThreadImplPosix(std::function thread_routine) - : thread_routine_(thread_routine) { + : thread_routine_(std::move(thread_routine)) { RELEASE_ASSERT(Logger::Registry::initialized(), ""); const int rc = pthread_create( &thread_handle_, nullptr, @@ -52,9 +46,7 @@ ThreadPtr ThreadFactoryImplPosix::createThread(std::function thread_rout return std::make_unique(thread_routine); } -ThreadIdPtr ThreadFactoryImplPosix::currentThreadId() { - return std::make_unique(getCurrentThreadId()); -} +ThreadId ThreadFactoryImplPosix::currentThreadId() { return ThreadId(getCurrentThreadId()); } } // namespace Thread } // namespace Envoy diff --git a/source/common/common/posix/thread_impl.h b/source/common/common/posix/thread_impl.h index aecc59e05b09..81c81d3be3fc 100755 --- a/source/common/common/posix/thread_impl.h +++ b/source/common/common/posix/thread_impl.h @@ -9,18 +9,6 @@ namespace Envoy { namespace Thread { -class ThreadIdImplPosix : public ThreadId { -public: - ThreadIdImplPosix(int64_t id); - - // Thread::ThreadId - std::string debugString() const override; - bool isCurrentThreadId() const override; - -private: - int64_t id_; -}; - /** * Wrapper for a pthread thread. We don't use std::thread because it eats exceptions and leads to * unusable stack traces. @@ -44,7 +32,7 @@ class ThreadFactoryImplPosix : public ThreadFactory { public: // Thread::ThreadFactory ThreadPtr createThread(std::function thread_routine) override; - ThreadIdPtr currentThreadId() override; + ThreadId currentThreadId() override; }; } // namespace Thread diff --git a/source/common/common/win32/thread_impl.cc b/source/common/common/win32/thread_impl.cc index bee7b9f2f979..1d3eca968957 100644 --- a/source/common/common/win32/thread_impl.cc +++ b/source/common/common/win32/thread_impl.cc @@ -6,12 +6,6 @@ namespace Envoy { namespace Thread { -ThreadIdImplWin32::ThreadIdImplWin32(DWORD id) : id_(id) {} - -std::string ThreadIdImplWin32::debugString() const { return std::to_string(id_); } - -bool ThreadIdImplWin32::isCurrentThreadId() const { return id_ == ::GetCurrentThreadId(); } - ThreadImplWin32::ThreadImplWin32(std::function thread_routine) : thread_routine_(thread_routine) { RELEASE_ASSERT(Logger::Registry::initialized(), ""); @@ -36,8 +30,9 @@ ThreadPtr ThreadFactoryImplWin32::createThread(std::function thread_rout return std::make_unique(thread_routine); } -ThreadIdPtr ThreadFactoryImplWin32::currentThreadId() { - return std::make_unique(::GetCurrentThreadId()); +ThreadId ThreadFactoryImplWin32::currentThreadId() { + // TODO(mhoran): test this in windows please. + return ThreadId(static_cast(::GetCurrentThreadId())); } } // namespace Thread diff --git a/source/common/common/win32/thread_impl.h b/source/common/common/win32/thread_impl.h index cd3821900f85..a8c74eb5d21a 100644 --- a/source/common/common/win32/thread_impl.h +++ b/source/common/common/win32/thread_impl.h @@ -13,18 +13,6 @@ namespace Envoy { namespace Thread { -class ThreadIdImplWin32 : public ThreadId { -public: - ThreadIdImplWin32(DWORD id); - - // Thread::ThreadId - std::string debugString() const override; - bool isCurrentThreadId() const override; - -private: - DWORD id_; -}; - /** * Wrapper for a win32 thread. We don't use std::thread because it eats exceptions and leads to * unusable stack traces. @@ -49,7 +37,7 @@ class ThreadFactoryImplWin32 : public ThreadFactory { public: // Thread::ThreadFactory ThreadPtr createThread(std::function thread_routine) override; - ThreadIdPtr currentThreadId() override; + ThreadId currentThreadId() override; }; } // namespace Thread diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 4ffcf6181a82..996ce9960471 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -48,7 +48,7 @@ void DispatcherImpl::initializeStats(Stats::Scope& scope, const std::string& pre stats_ = std::make_unique( DispatcherStats{ALL_DISPATCHER_STATS(POOL_HISTOGRAM_PREFIX(scope, stats_prefix_ + "."))}); base_scheduler_.initializeStats(stats_.get()); - ENVOY_LOG(debug, "running {} on thread {}", stats_prefix_, run_tid_->debugString()); + ENVOY_LOG(debug, "running {} on thread {}", stats_prefix_, run_tid_.debugString()); }); } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index b712f22d879e..ba95ee5684ea 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -72,12 +72,14 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { // Validate that an operation is thread safe, i.e. it's invoked on the same thread that the // dispatcher run loop is executing on. We allow run_tid_ == nullptr for tests where we don't // invoke run(). - bool isThreadSafe() const { return run_tid_ == nullptr || run_tid_->isCurrentThreadId(); } + bool isThreadSafe() const { + return run_tid_.isEmpty() || run_tid_ == api_.threadFactory().currentThreadId(); + } Api::Api& api_; std::string stats_prefix_; std::unique_ptr stats_; - Thread::ThreadIdPtr run_tid_; + Thread::ThreadId run_tid_; Buffer::WatermarkFactoryPtr buffer_factory_; LibeventScheduler base_scheduler_; SchedulerPtr scheduler_; diff --git a/source/common/singleton/BUILD b/source/common/singleton/BUILD index 3dad3f52a689..f3b1e651772d 100644 --- a/source/common/singleton/BUILD +++ b/source/common/singleton/BUILD @@ -21,6 +21,7 @@ envoy_cc_library( "//include/envoy/registry", "//include/envoy/singleton:manager_interface", "//source/common/common:assert_lib", + "//source/common/common:non_copyable", "//source/common/common:thread_lib", ], ) diff --git a/source/common/singleton/manager_impl.cc b/source/common/singleton/manager_impl.cc index 9023b2b744ca..e90d3b000c63 100644 --- a/source/common/singleton/manager_impl.cc +++ b/source/common/singleton/manager_impl.cc @@ -9,7 +9,8 @@ namespace Envoy { namespace Singleton { InstanceSharedPtr ManagerImpl::get(const std::string& name, SingletonFactoryCb cb) { - ASSERT(run_tid_->isCurrentThreadId()); + ASSERT(run_tid_ == thread_factory_.currentThreadId()); + if (nullptr == Registry::FactoryRegistry::getFactory(name)) { PANIC(fmt::format("invalid singleton name '{}'. Make sure it is registered.", name)); } diff --git a/source/common/singleton/manager_impl.h b/source/common/singleton/manager_impl.h index 9625087683d2..6f55ad3fadb2 100644 --- a/source/common/singleton/manager_impl.h +++ b/source/common/singleton/manager_impl.h @@ -5,6 +5,8 @@ #include "envoy/singleton/manager.h" #include "envoy/thread/thread.h" +#include "common/common/non_copyable.h" + namespace Envoy { namespace Singleton { @@ -13,16 +15,18 @@ namespace Singleton { * assumed the singleton manager is only used on the main thread so it is not thread safe. Asserts * verify that. */ -class ManagerImpl : public Manager { +class ManagerImpl : public Manager, NonCopyable { public: - ManagerImpl(Thread::ThreadIdPtr&& thread_id) : run_tid_(std::move(thread_id)) {} + explicit ManagerImpl(Thread::ThreadFactory& thread_factory) + : thread_factory_(thread_factory), run_tid_(thread_factory.currentThreadId()) {} // Singleton::Manager InstanceSharedPtr get(const std::string& name, SingletonFactoryCb cb) override; private: std::unordered_map> singletons_; - Thread::ThreadIdPtr run_tid_; + Thread::ThreadFactory& thread_factory_; + const Thread::ThreadId run_tid_; }; } // namespace Singleton diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 96faf5f81bd6..6e66d550227d 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -44,7 +44,7 @@ ValidationInstance::ValidationInstance(const Options& options, Event::TimeSystem : options_(options), stats_store_(store), api_(new Api::ValidationImpl(thread_factory, store, time_system, file_system)), dispatcher_(api_->allocateDispatcher()), - singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory().currentThreadId())), + singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory())), access_log_manager_(options.fileFlushIntervalMsec(), *api_, *dispatcher_, access_log_lock, store), mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), diff --git a/source/server/guarddog_impl.cc b/source/server/guarddog_impl.cc index 67b55ca00840..a9e3e583e318 100644 --- a/source/server/guarddog_impl.cc +++ b/source/server/guarddog_impl.cc @@ -103,7 +103,7 @@ void GuardDogImpl::step() { } } -WatchDogSharedPtr GuardDogImpl::createWatchDog(Thread::ThreadIdPtr&& thread_id) { +WatchDogSharedPtr GuardDogImpl::createWatchDog(Thread::ThreadId thread_id) { // Timer started by WatchDog will try to fire at 1/2 of the interval of the // minimum timeout specified. loop_interval_ is const so all shared state // accessed out of the locked section below is const (time_source_ has no diff --git a/source/server/guarddog_impl.h b/source/server/guarddog_impl.h index c1ba688e9ba0..5c0906ea55ad 100644 --- a/source/server/guarddog_impl.h +++ b/source/server/guarddog_impl.h @@ -87,7 +87,7 @@ class GuardDogImpl : public GuardDog { } // Server::GuardDog - WatchDogSharedPtr createWatchDog(Thread::ThreadIdPtr&& thread_id) override; + WatchDogSharedPtr createWatchDog(Thread::ThreadId thread_id) override; void stopWatching(WatchDogSharedPtr wd) override; private: diff --git a/source/server/server.cc b/source/server/server.cc index 8d2b5a58c8b6..dfd44d8f0f8c 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -61,7 +61,7 @@ InstanceImpl::InstanceImpl(const Options& options, Event::TimeSystem& time_syste start_time_(time(nullptr)), original_start_time_(start_time_), stats_store_(store), thread_local_(tls), api_(new Api::Impl(thread_factory, store, time_system, file_system)), dispatcher_(api_->allocateDispatcher()), - singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory().currentThreadId())), + singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory())), handler_(new ConnectionHandlerImpl(ENVOY_LOGGER(), *dispatcher_)), random_generator_(std::move(random_generator)), listener_component_factory_(*this), worker_factory_(thread_local_, *api_, hooks), diff --git a/source/server/watchdog_impl.h b/source/server/watchdog_impl.h index b4a4db1340af..ea4ea82e5c28 100644 --- a/source/server/watchdog_impl.h +++ b/source/server/watchdog_impl.h @@ -19,13 +19,12 @@ class WatchDogImpl : public WatchDog { /** * @param interval WatchDog timer interval (used after startWatchdog()) */ - WatchDogImpl(Thread::ThreadIdPtr&& thread_id, TimeSource& tsource, - std::chrono::milliseconds interval) - : thread_id_(std::move(thread_id)), time_source_(tsource), + WatchDogImpl(Thread::ThreadId thread_id, TimeSource& tsource, std::chrono::milliseconds interval) + : thread_id_(thread_id), time_source_(tsource), latest_touch_time_since_epoch_(tsource.monotonicTime().time_since_epoch()), timer_interval_(interval) {} - const Thread::ThreadId& threadId() const override { return *thread_id_; } + Thread::ThreadId threadId() const override { return thread_id_; } MonotonicTime lastTouchTime() const override { return MonotonicTime(latest_touch_time_since_epoch_.load()); } @@ -37,7 +36,7 @@ class WatchDogImpl : public WatchDog { } private: - Thread::ThreadIdPtr thread_id_; + const Thread::ThreadId thread_id_; TimeSource& time_source_; std::atomic latest_touch_time_since_epoch_; Event::TimerPtr timer_; diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 9d2d93940b54..d72fea4b6521 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -204,6 +204,16 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "thread_id_test", + srcs = ["thread_id_test.cc"], + external_deps = ["abseil_hash_testing"], + deps = [ + "//source/common/common:thread_lib", + "//test/test_common:thread_factory_for_test_lib", + ], +) + envoy_cc_test( name = "stl_helpers_test", srcs = ["stl_helpers_test.cc"], diff --git a/test/common/common/thread_id_test.cc b/test/common/common/thread_id_test.cc new file mode 100644 index 000000000000..7288b932cfc4 --- /dev/null +++ b/test/common/common/thread_id_test.cc @@ -0,0 +1,50 @@ +#include "common/common/thread.h" + +#include "test/test_common/thread_factory_for_test.h" + +#include "absl/hash/hash_testing.h" +#include "gmock/gmock.h" + +namespace Envoy { +namespace { + +TEST(ThreadId, Equality) { + auto& thread_factory = Thread::threadFactoryForTest(); + + Thread::ThreadId main_thread = thread_factory.currentThreadId(); + Thread::ThreadId background_thread; + Thread::ThreadId null_thread; + + Thread::threadFactoryForTest() + .createThread([&]() { background_thread = thread_factory.currentThreadId(); }) + ->join(); + + EXPECT_EQ(main_thread, main_thread); + EXPECT_EQ(background_thread, background_thread); + EXPECT_EQ(null_thread, null_thread); + + EXPECT_NE(main_thread, background_thread); + EXPECT_NE(main_thread, null_thread); + EXPECT_NE(background_thread, null_thread); +} + +TEST(ThreadId, Hashability) { + auto& thread_factory = Thread::threadFactoryForTest(); + + Thread::ThreadId main_thread = thread_factory.currentThreadId(); + Thread::ThreadId background_thread; + Thread::ThreadId null_thread; + + Thread::threadFactoryForTest() + .createThread([&]() { background_thread = thread_factory.currentThreadId(); }) + ->join(); + + EXPECT_TRUE(absl::VerifyTypeImplementsAbslHashCorrectly({ + null_thread, + main_thread, + background_thread, + })); +} + +} // namespace +} // namespace Envoy diff --git a/test/common/singleton/manager_impl_test.cc b/test/common/singleton/manager_impl_test.cc index 7fba6275cab5..14b3d9eacad2 100644 --- a/test/common/singleton/manager_impl_test.cc +++ b/test/common/singleton/manager_impl_test.cc @@ -12,7 +12,7 @@ namespace { // Must be a dedicated function so that TID is within the death test. static void deathTestWorker() { - ManagerImpl manager(Thread::threadFactoryForTest().currentThreadId()); + ManagerImpl manager(Thread::threadFactoryForTest()); manager.get("foo", [] { return nullptr; }); } @@ -35,7 +35,7 @@ class TestSingleton : public Instance { }; TEST(SingletonManagerImplTest, Basic) { - ManagerImpl manager(Thread::threadFactoryForTest().currentThreadId()); + ManagerImpl manager(Thread::threadFactoryForTest()); std::shared_ptr singleton = std::make_shared(); EXPECT_EQ(singleton, manager.get("test_singleton", [singleton] { return singleton; })); diff --git a/test/common/upstream/cluster_factory_impl_test.cc b/test/common/upstream/cluster_factory_impl_test.cc index 759faff9dd1f..85ed32e327c6 100644 --- a/test/common/upstream/cluster_factory_impl_test.cc +++ b/test/common/upstream/cluster_factory_impl_test.cc @@ -65,7 +65,7 @@ class ClusterFactoryTestBase { NiceMock runtime_; NiceMock random_; Stats::IsolatedStoreImpl stats_; - Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; NiceMock tls_; NiceMock validation_visitor_; Api::ApiPtr api_; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 69870436d991..75fa0ab06800 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -132,7 +132,7 @@ class TestClusterManagerFactory : public ClusterManagerFactory { NiceMock admin_; NiceMock secret_manager_; NiceMock log_manager_; - Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; NiceMock validation_visitor_; Api::ApiPtr api_; }; diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 9a92063f073f..78854abad4f0 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -108,7 +108,7 @@ class EdsTest : public testing::Test { NiceMock runtime_; NiceMock local_info_; NiceMock admin_; - Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; NiceMock tls_; NiceMock validation_visitor_; Api::ApiPtr api_; diff --git a/test/common/upstream/hds_test.cc b/test/common/upstream/hds_test.cc index 330f7db3279a..c7fe3af8d73e 100644 --- a/test/common/upstream/hds_test.cc +++ b/test/common/upstream/hds_test.cc @@ -126,7 +126,7 @@ class HdsTest : public testing::Test { NiceMock cm_; NiceMock local_info_; NiceMock admin_; - Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; NiceMock tls_; }; diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index 367d7afa55aa..03170a23af5a 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -214,7 +214,7 @@ class LogicalDnsClusterTest : public testing::Test { NiceMock dispatcher_; NiceMock local_info_; NiceMock admin_; - Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; NiceMock validation_visitor_; Api::ApiPtr api_; }; diff --git a/test/common/upstream/original_dst_cluster_test.cc b/test/common/upstream/original_dst_cluster_test.cc index f51042c28294..e3c5638dfef2 100644 --- a/test/common/upstream/original_dst_cluster_test.cc +++ b/test/common/upstream/original_dst_cluster_test.cc @@ -98,7 +98,7 @@ class OriginalDstClusterTest : public testing::Test { NiceMock random_; NiceMock local_info_; NiceMock admin_; - Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; NiceMock tls_; NiceMock validation_visitor_; Api::ApiPtr api_; diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 9001f60c7691..8fd0de99de76 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -58,7 +58,7 @@ class UpstreamImplTestBase { NiceMock runtime_; NiceMock random_; Stats::IsolatedStoreImpl stats_; - Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; NiceMock tls_; NiceMock validation_visitor_; Api::ApiPtr api_; @@ -1816,7 +1816,7 @@ class ClusterInfoImplTest : public testing::Test { NiceMock local_info_; NiceMock random_; NiceMock admin_; - Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; NiceMock tls_; ReadyWatcher initialized_; envoy::api::v2::Cluster cluster_config_; diff --git a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc index 568637c431a2..d15870581e24 100644 --- a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc +++ b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc @@ -94,7 +94,7 @@ class ClusterTest : public testing::Test, NiceMock dispatcher_; NiceMock local_info_; NiceMock admin_; - Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; NiceMock validation_visitor_; Api::ApiPtr api_{Api::createApiForTest(stats_store_)}; std::shared_ptr dns_cache_manager_{ diff --git a/test/extensions/clusters/redis/redis_cluster_test.cc b/test/extensions/clusters/redis/redis_cluster_test.cc index 7e69602477bc..8363be3b14a5 100644 --- a/test/extensions/clusters/redis/redis_cluster_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_test.cc @@ -455,7 +455,7 @@ class RedisClusterTest : public testing::Test, NiceMock dispatcher_; NiceMock local_info_; NiceMock admin_; - Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; NiceMock validation_visitor_; Api::ApiPtr api_; std::shared_ptr hosts_; diff --git a/test/extensions/transport_sockets/alts/config_test.cc b/test/extensions/transport_sockets/alts/config_test.cc index 91252b991308..34fcd34ae645 100644 --- a/test/extensions/transport_sockets/alts/config_test.cc +++ b/test/extensions/transport_sockets/alts/config_test.cc @@ -24,7 +24,7 @@ namespace { TEST(UpstreamAltsConfigTest, CreateSocketFactory) { MockTransportSocketFactoryContext factory_context; - Singleton::ManagerImpl singleton_manager{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager{Thread::threadFactoryForTest()}; EXPECT_CALL(factory_context, singletonManager()).WillRepeatedly(ReturnRef(singleton_manager)); UpstreamAltsTransportSocketConfigFactory factory; @@ -44,7 +44,7 @@ TEST(UpstreamAltsConfigTest, CreateSocketFactory) { TEST(DownstreamAltsConfigTest, CreateSocketFactory) { MockTransportSocketFactoryContext factory_context; - Singleton::ManagerImpl singleton_manager{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager{Thread::threadFactoryForTest()}; EXPECT_CALL(factory_context, singletonManager()).WillRepeatedly(ReturnRef(singleton_manager)); DownstreamAltsTransportSocketConfigFactory factory; diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 12e2ac5c3169..bfaa237f50d5 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -125,8 +125,8 @@ MockWorker::~MockWorker() = default; MockInstance::MockInstance() : secret_manager_(new Secret::SecretManagerImpl()), cluster_manager_(timeSource()), - ssl_context_manager_(timeSource()), singleton_manager_(new Singleton::ManagerImpl( - Thread::threadFactoryForTest().currentThreadId())), + ssl_context_manager_(timeSource()), + singleton_manager_(new Singleton::ManagerImpl(Thread::threadFactoryForTest())), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()) { ON_CALL(*this, threadLocal()).WillByDefault(ReturnRef(thread_local_)); ON_CALL(*this, stats()).WillByDefault(ReturnRef(stats_store_)); @@ -170,8 +170,7 @@ MockMain::MockMain(int wd_miss, int wd_megamiss, int wd_kill, int wd_multikill) MockMain::~MockMain() = default; MockFactoryContext::MockFactoryContext() - : singleton_manager_( - new Singleton::ManagerImpl(Thread::threadFactoryForTest().currentThreadId())), + : singleton_manager_(new Singleton::ManagerImpl(Thread::threadFactoryForTest())), grpc_context_(scope_.symbolTable()), http_context_(scope_.symbolTable()) { ON_CALL(*this, accessLogManager()).WillByDefault(ReturnRef(access_log_manager_)); ON_CALL(*this, clusterManager()).WillByDefault(ReturnRef(cluster_manager_)); diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index fde44c23ac7c..6ab3baac302c 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -54,7 +54,7 @@ class MockOptions : public Options { public: MockOptions() : MockOptions(std::string()) {} MockOptions(const std::string& config_path); - ~MockOptions(); + ~MockOptions() override; MOCK_CONST_METHOD0(baseId, uint64_t()); MOCK_CONST_METHOD0(concurrency, uint32_t()); @@ -102,7 +102,7 @@ class MockOptions : public Options { class MockConfigTracker : public ConfigTracker { public: MockConfigTracker(); - ~MockConfigTracker(); + ~MockConfigTracker() override; struct MockEntryOwner : public EntryOwner {}; @@ -120,7 +120,7 @@ class MockConfigTracker : public ConfigTracker { class MockAdmin : public Admin { public: MockAdmin(); - ~MockAdmin(); + ~MockAdmin() override; // Server::Admin MOCK_METHOD5(addHandler, bool(const std::string& prefix, const std::string& help_text, @@ -142,7 +142,7 @@ class MockAdmin : public Admin { class MockAdminStream : public AdminStream { public: MockAdminStream(); - ~MockAdminStream(); + ~MockAdminStream() override; MOCK_METHOD1(setEndStreamOnComplete, void(bool)); MOCK_METHOD1(addOnDestroyCallback, void(std::function)); @@ -155,7 +155,7 @@ class MockAdminStream : public AdminStream { class MockDrainManager : public DrainManager { public: MockDrainManager(); - ~MockDrainManager(); + ~MockDrainManager() override; // Server::DrainManager MOCK_CONST_METHOD0(drainClose, bool()); @@ -168,22 +168,22 @@ class MockDrainManager : public DrainManager { class MockWatchDog : public WatchDog { public: MockWatchDog(); - ~MockWatchDog(); + ~MockWatchDog() override; // Server::WatchDog MOCK_METHOD1(startWatchdog, void(Event::Dispatcher& dispatcher)); MOCK_METHOD0(touch, void()); - MOCK_CONST_METHOD0(threadId, const Thread::ThreadId&()); + MOCK_CONST_METHOD0(threadId, Thread::ThreadId()); MOCK_CONST_METHOD0(lastTouchTime, MonotonicTime()); }; class MockGuardDog : public GuardDog { public: MockGuardDog(); - ~MockGuardDog(); + ~MockGuardDog() override; // Server::GuardDog - MOCK_METHOD1(createWatchDog, WatchDogSharedPtr(Thread::ThreadIdPtr&&)); + MOCK_METHOD1(createWatchDog, WatchDogSharedPtr(Thread::ThreadId)); MOCK_METHOD1(stopWatching, void(WatchDogSharedPtr wd)); std::shared_ptr watch_dog_; @@ -192,7 +192,7 @@ class MockGuardDog : public GuardDog { class MockHotRestart : public HotRestart { public: MockHotRestart(); - ~MockHotRestart(); + ~MockHotRestart() override; // Server::HotRestart MOCK_METHOD0(drainParentListeners, void()); @@ -218,7 +218,7 @@ class MockHotRestart : public HotRestart { class MockListenerComponentFactory : public ListenerComponentFactory { public: MockListenerComponentFactory(); - ~MockListenerComponentFactory(); + ~MockListenerComponentFactory() override; DrainManagerPtr createDrainManager(envoy::api::v2::Listener::DrainType drain_type) override { return DrainManagerPtr{createDrainManager_(drain_type)}; @@ -254,7 +254,7 @@ class MockListenerComponentFactory : public ListenerComponentFactory { class MockListenerManager : public ListenerManager { public: MockListenerManager(); - ~MockListenerManager(); + ~MockListenerManager() override; MOCK_METHOD3(addOrUpdateListener, bool(const envoy::api::v2::Listener& config, const std::string& version_info, bool modifiable)); @@ -270,7 +270,7 @@ class MockListenerManager : public ListenerManager { class MockServerLifecycleNotifier : public ServerLifecycleNotifier { public: MockServerLifecycleNotifier(); - ~MockServerLifecycleNotifier(); + ~MockServerLifecycleNotifier() override; MOCK_METHOD2(registerCallback, ServerLifecycleNotifier::HandlePtr(Stage, StageCallback)); MOCK_METHOD2(registerCallback, @@ -280,7 +280,7 @@ class MockServerLifecycleNotifier : public ServerLifecycleNotifier { class MockWorkerFactory : public WorkerFactory { public: MockWorkerFactory(); - ~MockWorkerFactory(); + ~MockWorkerFactory() override; // Server::WorkerFactory WorkerPtr createWorker(OverloadManager&) override { return WorkerPtr{createWorker_()}; } @@ -291,7 +291,7 @@ class MockWorkerFactory : public WorkerFactory { class MockWorker : public Worker { public: MockWorker(); - ~MockWorker(); + ~MockWorker() override; void callAddCompletion(bool success) { EXPECT_NE(nullptr, add_listener_completion_); @@ -324,7 +324,7 @@ class MockWorker : public Worker { class MockOverloadManager : public OverloadManager { public: MockOverloadManager(); - ~MockOverloadManager(); + ~MockOverloadManager() override; // OverloadManager MOCK_METHOD0(start, void()); @@ -338,7 +338,7 @@ class MockOverloadManager : public OverloadManager { class MockInstance : public Instance { public: MockInstance(); - ~MockInstance(); + ~MockInstance() override; Secret::SecretManager& secretManager() override { return *(secret_manager_.get()); } @@ -414,7 +414,7 @@ class MockMain : public Main { public: MockMain() : MockMain(0, 0, 0, 0) {} MockMain(int wd_miss, int wd_megamiss, int wd_kill, int wd_multikill); - ~MockMain(); + ~MockMain() override; MOCK_METHOD0(clusterManager, Upstream::ClusterManager*()); MOCK_METHOD0(httpTracer, Tracing::HttpTracer&()); @@ -434,7 +434,7 @@ class MockMain : public Main { class MockFactoryContext : public virtual FactoryContext { public: MockFactoryContext(); - ~MockFactoryContext(); + ~MockFactoryContext() override; MOCK_METHOD0(accessLogManager, AccessLog::AccessLogManager&()); MOCK_METHOD0(clusterManager, Upstream::ClusterManager&()); @@ -487,7 +487,7 @@ class MockFactoryContext : public virtual FactoryContext { class MockTransportSocketFactoryContext : public TransportSocketFactoryContext { public: MockTransportSocketFactoryContext(); - ~MockTransportSocketFactoryContext(); + ~MockTransportSocketFactoryContext() override; Secret::SecretManager& secretManager() override { return *(secret_manager_.get()); } @@ -514,7 +514,7 @@ class MockTransportSocketFactoryContext : public TransportSocketFactoryContext { class MockListenerFactoryContext : public MockFactoryContext, public ListenerFactoryContext { public: MockListenerFactoryContext(); - ~MockListenerFactoryContext(); + ~MockListenerFactoryContext() override; void addListenSocketOption(const Network::Socket::OptionConstSharedPtr& option) override { addListenSocketOption_(option); @@ -533,7 +533,7 @@ class MockListenerFactoryContext : public MockFactoryContext, public ListenerFac class MockHealthCheckerFactoryContext : public virtual HealthCheckerFactoryContext { public: MockHealthCheckerFactoryContext(); - ~MockHealthCheckerFactoryContext(); + ~MockHealthCheckerFactoryContext() override; MOCK_METHOD0(cluster, Upstream::Cluster&()); MOCK_METHOD0(dispatcher, Event::Dispatcher&()); diff --git a/test/server/config_validation/cluster_manager_test.cc b/test/server/config_validation/cluster_manager_test.cc index 2b2406725330..ce734f717301 100644 --- a/test/server/config_validation/cluster_manager_test.cc +++ b/test/server/config_validation/cluster_manager_test.cc @@ -43,7 +43,7 @@ TEST(ValidationClusterManagerTest, MockedMethods) { NiceMock admin; Http::ContextImpl http_context(stats_store.symbolTable()); AccessLog::MockAccessLogManager log_manager; - Singleton::ManagerImpl singleton_manager{Thread::threadFactoryForTest().currentThreadId()}; + Singleton::ManagerImpl singleton_manager{Thread::threadFactoryForTest()}; ValidationClusterManagerFactory factory(admin, runtime, stats_store, tls, random, dns_resolver, ssl_context_manager, dispatcher, local_info, diff --git a/test/server/guarddog_impl_test.cc b/test/server/guarddog_impl_test.cc index c1e00ef03724..77b3397ffd59 100644 --- a/test/server/guarddog_impl_test.cc +++ b/test/server/guarddog_impl_test.cc @@ -316,7 +316,7 @@ TEST_P(GuardDogTestBase, WatchDogThreadIdTest) { initGuardDog(stats, config); auto watched_dog = guard_dog_->createWatchDog(api_->threadFactory().currentThreadId()); EXPECT_EQ(watched_dog->threadId().debugString(), - api_->threadFactory().currentThreadId()->debugString()); + api_->threadFactory().currentThreadId().debugString()); guard_dog_->stopWatching(watched_dog); } diff --git a/test/test_common/only_one_thread.cc b/test/test_common/only_one_thread.cc index a6a5869b7eda..06cc9855446d 100644 --- a/test/test_common/only_one_thread.cc +++ b/test/test_common/only_one_thread.cc @@ -13,10 +13,10 @@ OnlyOneThread::OnlyOneThread() : thread_factory_(threadFactoryForTest()) {} void OnlyOneThread::checkOneThread() { LockGuard lock(mutex_); - if (thread_advancing_time_ == nullptr) { + if (thread_advancing_time_.isEmpty()) { thread_advancing_time_ = thread_factory_.currentThreadId(); } else { - RELEASE_ASSERT(thread_advancing_time_->isCurrentThreadId(), + RELEASE_ASSERT(thread_advancing_time_ == thread_factory_.currentThreadId(), "time should only be advanced on one thread in the context of a test"); } } diff --git a/test/test_common/only_one_thread.h b/test/test_common/only_one_thread.h index 1051c632e78a..678e40b319a0 100644 --- a/test/test_common/only_one_thread.h +++ b/test/test_common/only_one_thread.h @@ -20,7 +20,7 @@ class OnlyOneThread { private: ThreadFactory& thread_factory_; - ThreadIdPtr thread_advancing_time_ GUARDED_BY(mutex_); + ThreadId thread_advancing_time_ GUARDED_BY(mutex_); mutable MutexBasicLockable mutex_; };