test: remove the manual declarations of TestThread and figure it out from the platform.

source/common/common/thread.cc

@@ -20,15 +20,11 @@ namespace {
 // call-sites for isMainThread(), which might be a bit of work, but will make
 // tests more hermetic.
 struct ThreadIds {
-  // Determines whether we are currently running on the main-thread or
-  // test-thread. We need to allow for either one because we don't establish
-  // the full threading model in all unit tests.
-  bool inMainOrTestThread() const {
+  bool inMainThread() const {
     // We don't take the lock when testing the thread IDs, as they are atomic,
     // and are cleared when being released. All possible thread orderings
     // result in the correct result even without a lock.
-    std::thread::id id = std::this_thread::get_id();
-    return main_thread_id_ == id || test_thread_id_ == id;
+    return std::this_thread::get_id() == main_thread_id_;
   }
 
   bool isMainThreadActive() const {
@@ -53,20 +49,6 @@ struct ThreadIds {
     }
   }
 
-  // Call this when the TestThread exits. Nested semantics are supported, so
-  // that if multiple TestThread instances are declared, we unwind them
-  // properly.
-  void releaseTestThread() {
-    absl::MutexLock lock(&mutex_);
-    ASSERT(test_thread_use_count_ > 0);
-    ASSERT(std::this_thread::get_id() == test_thread_id_);
-    if (--test_thread_use_count_ == 0) {
-      // Clearing the thread ID when its use-count goes to zero allows us
-      // to read the atomic without taking a lock.
-      test_thread_id_ = std::thread::id{};
-    }
-  }
-
   // Declares current thread as the main one, or verifies that the current
   // thread matches any previous declarations.
   void registerMainThread() {
@@ -78,43 +60,52 @@ struct ThreadIds {
     }
   }
 
-  // Declares current thread as the test thread, or verifies that the current
-  // thread matches any previous declarations.
-  void registerTestThread() {
-    absl::MutexLock lock(&mutex_);
-    if (++test_thread_use_count_ > 1) {
-      ASSERT(std::this_thread::get_id() == test_thread_id_);
-    } else {
-      test_thread_id_ = std::this_thread::get_id();
-    }
-  }
+  // Methods to track how many SkipAssert objects are instantiated.
+  void incSkipAsserts() { ++skip_asserts_; }
+  void decSkipAsserts() { --skip_asserts_; }
+  bool skipAsserts() const { return skip_asserts_ > 0; }
 
 private:
   // The atomic thread IDs can be read without a mutex, but they are written
   // under a mutex so that they are consistent with their use_counts. this
   // avoids the possibility of two threads racing to claim being the main/test
   // thread.
   std::atomic<std::thread::id> main_thread_id_;
-  std::atomic<std::thread::id> test_thread_id_;
 
   int32_t main_thread_use_count_ GUARDED_BY(mutex_) = 0;
-  int32_t test_thread_use_count_ GUARDED_BY(mutex_) = 0;
   mutable absl::Mutex mutex_;
+
+  std::atomic<uint32_t> skip_asserts_{};
 };
 
 } // namespace
 
-bool MainThread::isMainOrTestThread() { return ThreadIds::get().inMainOrTestThread(); }
+bool MainThread::isMainThread() { return ThreadIds::get().inMainThread(); }
 
 bool MainThread::isMainThreadActive() { return ThreadIds::get().isMainThreadActive(); }
 
-TestThread::TestThread() { ThreadIds::get().registerTestThread(); }
-
-TestThread::~TestThread() { ThreadIds::get().releaseTestThread(); }
-
 MainThread::MainThread() { ThreadIds::get().registerMainThread(); }
 
 MainThread::~MainThread() { ThreadIds::get().releaseMainThread(); }
 
+#if TEST_THREAD_SUPPORTED
+bool TestThread::isTestThread() {
+  // Keep this implementation consistent with TEST_THREAD_SUPPORTED, defined in thread.h.
+  // https://stackoverflow.com/questions/4867839/how-can-i-tell-if-pthread-self-is-the-main-first-thread-in-the-process
+#ifdef __linux__
+  return getpid() == syscall(SYS_gettid);
+#elif defined(__APPLE__)
+  return pthread_main_np() != 0;
+#endif
+  // Note: final #else fallback omitted intentionally.
+}
+#endif
+
+SkipAsserts::SkipAsserts() { ThreadIds::get().incSkipAsserts(); }
+
+SkipAsserts::~SkipAsserts() { ThreadIds::get().decSkipAsserts(); }
+
+bool SkipAsserts::skip() { return ThreadIds::get().skipAsserts(); }
+
 } // namespace Thread
 } // namespace Envoy

source/common/common/thread.h

@@ -168,16 +168,29 @@ class AtomicPtr : private AtomicPtrArray<T, 1, alloc_mode> {
   T* get(const MakeObject& make_object) { return BaseClass::get(0, make_object); }
 };
 
-// RAII object to declare the TestThread. This should be declared in main() or
-// equivalent for any test binaries.
-//
-// Generally we expect TestThread to be instantiated only once on main() for
-// each test binary, though nested instantiations are allowed as long as the
-// thread ID does not change.
+// We use platform-specific functions to determine whether the current thread is
+// the "test thread". It is only valid to call isTestThread() on platforms where
+// these functions are available. Currently this is available only on apple and
+// linux.
+#if defined(__linux__) || defined(__APPLE__)
+#define TEST_THREAD_SUPPORTED 1
+#else
+#define TEST_THREAD_SUPPORTED 0
+#endif
+
+// Context for determining whether we are in the test thread.
 class TestThread {
 public:
-  TestThread();
-  ~TestThread();
+#if TEST_THREAD_SUPPORTED
+  /**
+   * @return whether the current thread is the test thread.
+   *
+   * Use of the macros ASSERT_IS_TEST_THREAD() and ASSERT_IS_NOT_TEST_THREAD()
+   * are preferred to avoid issues on platforms where detecting the test-thread
+   * is not supported.
+   */
+  static bool isTestThread();
+#endif
 };
 
 // RAII object to declare the MainThread. This should be declared in the thread
@@ -196,29 +209,110 @@ class MainThread {
   MainThread();
   ~MainThread();
 
+#if TEST_THREAD_SUPPORTED
   /**
    * @return whether the current thread is the main thread or test thread.
+   *
+   * Determines whether we are currently running on the main-thread or
+   * test-thread. We need to allow for either one because we don't establish
+   * the full threading model in all unit tests.
+   *
+   * Use of the macros ASSERT_IS_TEST_THREAD() and ASSERT_IS_NOT_TEST_THREAD()
+   * are preferred to avoid issues on platforms where detecting the test-thread
+   * is not supported.
    */
-  static bool isMainOrTestThread();
+  static bool isMainOrTestThread() { return isMainThread() || TestThread::isTestThread(); }
+#endif
+
+  /**
+   * @return whether the current thread is the main thread.
+   */
+  static bool isMainThread();
 
   /**
    * @return whether a MainThread has been instantiated.
    */
   static bool isMainThreadActive();
 };
 
-// To improve exception safety in data plane, we plan to forbid the use of raw try in the core code
-// base. This macros uses main thread assertion to make sure that exceptions aren't thrown from
-// worker thread.
-#define TRY_ASSERT_MAIN_THREAD                                                                     \
-  try {                                                                                            \
-    ASSERT(Thread::MainThread::isMainOrTestThread());
-
 #define END_TRY }
 
 // TODO(chaoqinli-1123): Remove this macros after we have removed all the exceptions from data
 // plane.
 #define TRY_NEEDS_AUDIT try
 
+// These convenience macros assert properties of the threading system, when
+// feasible. There is a platform-specific mechanism for determining whether the
+// current thread is from main(), which we call the "test thread", and if that
+// method is not available on the current platform we must skip the assertions.
+//
+// Note that the macros are all no-ops if there are any SkipAsserts instances
+// allocated.
+#ifdef NDEBUG
+
+#define ASSERT_IS_TEST_THREAD()
+#define ASSERT_IS_MAIN_OR_TEST_THREAD()
+#define ASSERT_IS_NOT_TEST_THREAD()
+#define ASSERT_IS_NOT_MAIN_OR_TEST_THREAD()
+
+#elif TEST_THREAD_SUPPORTED
+
+#define ASSERT_IS_TEST_THREAD()                                                                    \
+  ASSERT(Thread::SkipAsserts::skip() || Thread::TestThread::isTestThread())
+#define ASSERT_IS_MAIN_OR_TEST_THREAD()                                                            \
+  ASSERT(Thread::SkipAsserts::skip() || Thread::TestThread::isTestThread() ||                      \
+         Thread::MainThread::isMainThread())
+#define ASSERT_IS_NOT_TEST_THREAD()                                                                \
+  ASSERT(Thread::SkipAsserts::skip() || !Thread::TestThread::isTestThread())
+#define ASSERT_IS_NOT_MAIN_OR_TEST_THREAD()                                                        \
+  ASSERT(Thread::SkipAsserts::skip() ||                                                            \
+         (!Thread::MainThread::isMainThread() && !Thread::TestThread::isTestThread()))
+
+#else // !TEST_THREAD_SUPPORTED -- test-thread checks are skipped
+
+#define ASSERT_IS_TEST_THREAD()
+#define ASSERT_IS_MAIN_OR_TEST_THREAD()
+#define ASSERT_IS_NOT_TEST_THREAD()
+#define ASSERT_IS_NOT_MAIN_OR_TEST_THREAD() ASSERT(!Thread::MainThread::isMainThread())
+
+#endif
+
+/**
+ * To improve exception safety in data plane, we plan to forbid the use of raw
+ * try in the core code base. This macros uses main thread assertion to make
+ * sure that exceptions aren't thrown from worker thread.
+ */
+#define TRY_ASSERT_MAIN_THREAD                                                                     \
+  try {                                                                                            \
+    ASSERT_IS_MAIN_OR_TEST_THREAD();
+
+/**
+ * RAII class to override thread assertions checks in the macros:
+ *
+ *   TRY_ASSERT_MAIN_THREAD
+ *   ASSERT_IS_TEST_THREAD()
+ *   ASSERT_IS_MAIN_OR_TEST_THREAD()
+ *   ASSERT_IS_NOT_TEST_THREAD()
+ *   ASSERT_IS_NOT_MAIN_OR_TEST_THREAD()
+ *
+ * Those macros will be no-ops while there is a SkipAsserts object
+ * alive. SkipAsserts declarations can be nested.
+ *
+ * The state of the assertion-skipping can also be checked by calling static
+ * method SkipAsserts::skip().
+ *
+ * This class is intended to be instantiated on the stack in a limited scope.
+ */
+class SkipAsserts {
+public:
+  SkipAsserts();
+  ~SkipAsserts();
+
+  /**
+   * @return whether thread-related assertions should be skipped.
+   */
+  static bool skip();
+};
+
 } // namespace Thread
 } // namespace Envoy

source/common/config/context_provider_impl.h

@@ -20,7 +20,7 @@ class ContextProviderImpl : public ContextProvider {
   const xds::core::v3::ContextParams& nodeContext() const override { return node_context_; }
   const xds::core::v3::ContextParams&
   dynamicContext(absl::string_view resource_type_url) const override {
-    ASSERT(Thread::MainThread::isMainOrTestThread());
+    ASSERT_IS_MAIN_OR_TEST_THREAD();
     auto it = dynamic_context_.find(resource_type_url);
     if (it != dynamic_context_.end()) {
       return it->second;
@@ -29,22 +29,22 @@ class ContextProviderImpl : public ContextProvider {
   };
   void setDynamicContextParam(absl::string_view resource_type_url, absl::string_view key,
                               absl::string_view value) override {
-    ASSERT(Thread::MainThread::isMainOrTestThread());
+    ASSERT_IS_MAIN_OR_TEST_THREAD();
     (*dynamic_context_[resource_type_url]
           .mutable_params())[toStdStringView(key)] = // NOLINT(std::string_view)
         toStdStringView(value);                      // NOLINT(std::string_view)
     update_cb_helper_.runCallbacks(resource_type_url);
   }
   void unsetDynamicContextParam(absl::string_view resource_type_url,
                                 absl::string_view key) override {
-    ASSERT(Thread::MainThread::isMainOrTestThread());
+    ASSERT_IS_MAIN_OR_TEST_THREAD();
     dynamic_context_[resource_type_url].mutable_params()->erase(
         toStdStringView(key)); // NOLINT(std::string_view)
     update_cb_helper_.runCallbacks(resource_type_url);
   }
   ABSL_MUST_USE_RESULT Common::CallbackHandlePtr
   addDynamicContextUpdateCallback(UpdateNotificationCb callback) const override {
-    ASSERT(Thread::MainThread::isMainOrTestThread());
+    ASSERT_IS_MAIN_OR_TEST_THREAD();
     return update_cb_helper_.add(callback);
   };
 

source/common/config/utility.h

@@ -194,7 +194,7 @@ class Utility {
    */
   template <class Proto> static void checkTransportVersion(const Proto& api_config_source) {
     const auto transport_api_version = api_config_source.transport_api_version();
-    ASSERT(Thread::MainThread::isMainOrTestThread());
+    ASSERT_IS_MAIN_OR_TEST_THREAD();
     if (transport_api_version == envoy::config::core::v3::ApiVersion::AUTO ||
         transport_api_version == envoy::config::core::v3::ApiVersion::V2) {
       Runtime::LoaderSingleton::getExisting()->countDeprecatedFeatureUse();

source/common/network/dns_resolver/dns_factory_util.h

@@ -131,7 +131,7 @@ template <class ConfigType>
 Network::DnsResolverFactory& createDnsResolverFactoryFromProto(
     const ConfigType& config,
     envoy::config::core::v3::TypedExtensionConfig& typed_dns_resolver_config) {
-  ASSERT(Thread::MainThread::isMainOrTestThread());
+  ASSERT_IS_MAIN_OR_TEST_THREAD();
   typed_dns_resolver_config = makeDnsResolverConfig(config);
   return createDnsResolverFactoryFromTypedConfig(typed_dns_resolver_config);
 }

source/common/stats/custom_stat_namespaces_impl.cc

@@ -7,18 +7,18 @@ namespace Envoy {
 namespace Stats {
 
 bool CustomStatNamespacesImpl::registered(const absl::string_view name) const {
-  ASSERT(Thread::MainThread::isMainOrTestThread());
+  ASSERT_IS_MAIN_OR_TEST_THREAD();
   return namespaces_.find(name) != namespaces_.end();
 }
 
 void CustomStatNamespacesImpl::registerStatNamespace(const absl::string_view name) {
-  ASSERT(Thread::MainThread::isMainOrTestThread());
+  ASSERT_IS_MAIN_OR_TEST_THREAD();
   namespaces_.insert(std::string(name));
 };
 
 absl::optional<absl::string_view>
 CustomStatNamespacesImpl::stripRegisteredPrefix(const absl::string_view stat_name) const {
-  ASSERT(Thread::MainThread::isMainOrTestThread());
+  ASSERT_IS_MAIN_OR_TEST_THREAD();
   if (!namespaces_.empty()) {
     const auto pos = stat_name.find_first_of('.');
     if (pos != std::string::npos && registered(stat_name.substr(0, pos))) {