envoyproxy · mattklein123 · Jul 1, 2019 · Jun 20, 2019 · Jun 20, 2019 · Jun 20, 2019
diff --git 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
@@ -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
@@ -1,7 +1,9 @@
 #pragma once
 
 #include <functional>
+#include <limits>
 #include <memory>
+#include <string>
 
 #include "envoy/common/pure.h"
 
@@ -10,16 +12,27 @@
 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<int64_t>::min()) {}
+  ThreadId(int64_t id) : id_(id) {}
+  ~ThreadId() = default;
+
+  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 <typename H> friend H AbslHashValue(H h, ThreadId id) {
+    return H::combine(std::move(h), id.id_);
+  }
+
+private:
+  int64_t id_;
 };
 
-using ThreadIdPtr = std::unique_ptr<ThreadId>;
-
 class Thread {
 public:
   virtual ~Thread() = default;
@@ -48,7 +61,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
@@ -24,12 +24,6 @@ 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<void()> thread_routine)
     : thread_routine_(thread_routine) {
   RELEASE_ASSERT(Logger::Registry::initialized(), "");
@@ -52,9 +46,7 @@ ThreadPtr ThreadFactoryImplPosix::createThread(std::function<void()> thread_rout
   return std::make_unique<ThreadImplPosix>(thread_routine);
 }
 
-ThreadIdPtr ThreadFactoryImplPosix::currentThreadId() {
-  return std::make_unique<ThreadIdImplPosix>(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
@@ -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<void()> 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
@@ -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<void()> thread_routine)
     : thread_routine_(thread_routine) {
   RELEASE_ASSERT(Logger::Registry::initialized(), "");
@@ -36,8 +30,8 @@ ThreadPtr ThreadFactoryImplWin32::createThread(std::function<void()> thread_rout
   return std::make_unique<ThreadImplWin32>(thread_routine);
 }
 
-ThreadIdPtr ThreadFactoryImplWin32::currentThreadId() {
-  return std::make_unique<ThreadIdImplWin32>(::GetCurrentThreadId());
+ThreadId ThreadFactoryImplWin32::currentThreadId() {
+  return ThreadId(static_cast<int64_t>(::GetCurrentThreadId()));
 }
 
 } // namespace Thread

diff --git 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<void()> 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
@@ -48,7 +48,7 @@ void DispatcherImpl::initializeStats(Stats::Scope& scope, const std::string& pre
     stats_ = std::make_unique<DispatcherStats>(
         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
@@ -72,12 +72,14 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>, 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<DispatcherStats> stats_;
-  Thread::ThreadIdPtr run_tid_;
+  Thread::ThreadId run_tid_;
   Buffer::WatermarkFactoryPtr buffer_factory_;
   LibeventScheduler base_scheduler_;
   SchedulerPtr scheduler_;

diff --git 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<Registration>::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
@@ -15,14 +15,18 @@ namespace Singleton {
  */
 class ManagerImpl : public Manager {
 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()) {}
+  ManagerImpl(const ManagerImpl&) = delete;
+  ManagerImpl& operator=(const ManagerImpl&) = delete;
 
   // Singleton::Manager
   InstanceSharedPtr get(const std::string& name, SingletonFactoryCb cb) override;
 
 private:
   std::unordered_map<std::string, std::weak_ptr<Instance>> singletons_;
-  Thread::ThreadIdPtr run_tid_;
+  Thread::ThreadFactory& thread_factory_;
+  Thread::ThreadId run_tid_;
 };
 
 } // namespace Singleton

diff --git 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
@@ -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
@@ -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
@@ -60,7 +60,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
@@ -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)
+  WatchDogImpl(Thread::ThreadId thread_id, TimeSource& tsource, std::chrono::milliseconds interval)
       : thread_id_(std::move(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_;
+  Thread::ThreadId thread_id_;
   TimeSource& time_source_;
   std::atomic<std::chrono::steady_clock::duration> latest_touch_time_since_epoch_;
   Event::TimerPtr timer_;

diff --git 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<TestSingleton> singleton = std::make_shared<TestSingleton>();
   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
@@ -65,7 +65,7 @@ class ClusterFactoryTestBase {
   NiceMock<Runtime::MockLoader> runtime_;
   NiceMock<Runtime::MockRandomGenerator> random_;
   Stats::IsolatedStoreImpl stats_;
-  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()};
+  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()};
   NiceMock<ThreadLocal::MockInstance> tls_;
   NiceMock<ProtobufMessage::MockValidationVisitor> 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
@@ -132,7 +132,7 @@ class TestClusterManagerFactory : public ClusterManagerFactory {
   NiceMock<Server::MockAdmin> admin_;
   NiceMock<Secret::MockSecretManager> secret_manager_;
   NiceMock<AccessLog::MockAccessLogManager> log_manager_;
-  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()};
+  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()};
   NiceMock<ProtobufMessage::MockValidationVisitor> validation_visitor_;
   Api::ApiPtr api_;
 };

diff --git 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::MockLoader> runtime_;
   NiceMock<LocalInfo::MockLocalInfo> local_info_;
   NiceMock<Server::MockAdmin> admin_;
-  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()};
+  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()};
   NiceMock<ThreadLocal::MockInstance> tls_;
   NiceMock<ProtobufMessage::MockValidationVisitor> validation_visitor_;
   Api::ApiPtr api_;

diff --git a/test/common/upstream/hds_test.cc b/test/common/upstream/hds_test.cc
@@ -126,7 +126,7 @@ class HdsTest : public testing::Test {
   NiceMock<Upstream::MockClusterManager> cm_;
   NiceMock<LocalInfo::MockLocalInfo> local_info_;
   NiceMock<Server::MockAdmin> admin_;
-  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()};
+  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()};
   NiceMock<ThreadLocal::MockInstance> tls_;
 };
 

diff --git 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<Event::MockDispatcher> dispatcher_;
   NiceMock<LocalInfo::MockLocalInfo> local_info_;
   NiceMock<Server::MockAdmin> admin_;
-  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()};
+  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()};
   NiceMock<ProtobufMessage::MockValidationVisitor> 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
@@ -98,7 +98,7 @@ class OriginalDstClusterTest : public testing::Test {
   NiceMock<Runtime::MockRandomGenerator> random_;
   NiceMock<LocalInfo::MockLocalInfo> local_info_;
   NiceMock<Server::MockAdmin> admin_;
-  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()};
+  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()};
   NiceMock<ThreadLocal::MockInstance> tls_;
   NiceMock<ProtobufMessage::MockValidationVisitor> validation_visitor_;
   Api::ApiPtr api_;

diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc
@@ -58,7 +58,7 @@ class UpstreamImplTestBase {
   NiceMock<Runtime::MockLoader> runtime_;
   NiceMock<Runtime::MockRandomGenerator> random_;
   Stats::IsolatedStoreImpl stats_;
-  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()};
+  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()};
   NiceMock<ThreadLocal::MockInstance> tls_;
   NiceMock<ProtobufMessage::MockValidationVisitor> validation_visitor_;
   Api::ApiPtr api_;
@@ -1816,7 +1816,7 @@ class ClusterInfoImplTest : public testing::Test {
   NiceMock<LocalInfo::MockLocalInfo> local_info_;
   NiceMock<Runtime::MockRandomGenerator> random_;
   NiceMock<Server::MockAdmin> admin_;
-  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()};
+  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()};
   NiceMock<ThreadLocal::MockInstance> tls_;
   ReadyWatcher initialized_;
   envoy::api::v2::Cluster cluster_config_;

diff --git 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<Event::MockDispatcher> dispatcher_;
   NiceMock<LocalInfo::MockLocalInfo> local_info_;
   NiceMock<Server::MockAdmin> admin_;
-  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest().currentThreadId()};
+  Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()};
   NiceMock<ProtobufMessage::MockValidationVisitor> validation_visitor_;
   Api::ApiPtr api_;
   std::shared_ptr<Upstream::MockClusterMockPrioritySet> hosts_;