test: Make the creation of a test time system threadsafe

test/common/stats/symbol_table_impl_test.cc

@@ -367,6 +367,8 @@ TEST_P(StatNameTest, JoinAllEmpty) {
   EXPECT_EQ("", table_->toString(StatName(joined.get())));
 }
 
+// Validates that we don't get tsan or other errors when concurrently creating
+// a large number of stats.
 TEST_P(StatNameTest, RacingSymbolCreation) {
   Thread::ThreadFactory& thread_factory = Thread::threadFactoryForTest();
   MutexTracerImpl& mutex_tracer = MutexTracerImpl::getOrCreateTracer();
@@ -406,21 +408,9 @@ TEST_P(StatNameTest, RacingSymbolCreation) {
   int64_t create_contentions = mutex_tracer.numContentions();
   ENVOY_LOG_MISC(info, "Number of contentions: {}", create_contentions);
 
-  // But when we access the already-existing symbols, we guarantee that no
-  // further mutex contentions occur.
-  int64_t start_contentions = mutex_tracer.numContentions();
   access.setReady();
   accesses.Wait();
 
-  // With fake symbol tables, there are no contentions as there are is no state
-  // in the symbol table to lock. This is why fake symbol tables are a safe way
-  // to transition the codebase to use the SymbolTable API without impacting
-  // hot-path performance.
-  if (GetParam() == SymbolTableType::Fake) {
-    EXPECT_EQ(create_contentions, mutex_tracer.numContentions());
-    EXPECT_EQ(start_contentions, create_contentions);
-  }
-
   // In a perfect world, we could use reader-locks in the SymbolTable
   // implementation, and there should be zero additional contentions
   // after latching 'create_contentions' above. And we can definitely

test/test_common/BUILD

@@ -179,6 +179,7 @@ envoy_cc_test_library(
 
 envoy_cc_test_library(
     name = "test_time_system_interface",
+    srcs = ["test_time_system.cc"],
     hdrs = ["test_time_system.h"],
     deps = [
         ":global_lib",

test/test_common/test_time.cc

@@ -10,6 +10,14 @@ DangerousDeprecatedTestTime::DangerousDeprecatedTestTime() {}
 
 namespace Event {
 
+TestTimeSystem& GlobalTimeSystem::timeSystem() {
+  // TODO(#4160): Switch default to SimulatedTimeSystem.
+  auto make_real_time_system = []() -> std::unique_ptr<TestTimeSystem> {
+    return std::make_unique<TestRealTimeSystem>();
+  };
+  return singleton_->timeSystem(make_real_time_system);
+}
+
 void TestRealTimeSystem::sleep(const Duration& duration) { std::this_thread::sleep_for(duration); }
 
 Thread::CondVar::WaitStatus TestRealTimeSystem::waitFor(Thread::MutexBasicLockable& lock,

test/test_common/test_time.h

@@ -31,13 +31,7 @@ class TestRealTimeSystem : public TestTimeSystem {
 
 class GlobalTimeSystem : public DelegatingTestTimeSystemBase<TestTimeSystem> {
 public:
-  TestTimeSystem& timeSystem() override {
-    if (singleton_->timeSystem() == nullptr) {
-      // TODO(#4160): Switch default to SimulatedTimeSystem.
-      singleton_->set(new TestRealTimeSystem);
-    }
-    return *singleton_->timeSystem();
-  }
+  TestTimeSystem& timeSystem() override;
 
 private:
   Test::Global<SingletonTimeSystemHelper> singleton_;

test/test_common/test_time_system.cc

@@ -0,0 +1,19 @@
+#include "test/test_common/test_time_system.h"
+
+#include "envoy/event/timer.h"
+
+#include "common/common/thread.h"
+
+namespace Envoy {
+namespace Event {
+
+TestTimeSystem& SingletonTimeSystemHelper::timeSystem(const MakeTimeSystemFn& make_time_system) {
+  Thread::LockGuard lock(mutex_);
+  if (time_system_ == nullptr) {
+    time_system_ = make_time_system();
+  }
+  return *time_system_;
+}
+
+} // namespace Event
+} // namespace Envoy

test/test_common/test_time_system.h

@@ -61,15 +61,19 @@ class SingletonTimeSystemHelper {
 public:
   SingletonTimeSystemHelper() : time_system_(nullptr) {}
 
-  void set(TestTimeSystem* time_system) {
-    RELEASE_ASSERT(time_system_ == nullptr, "Unexpected reset of SingletonTimeSystemHelper");
-    time_system_.reset(time_system);
-  }
+  using MakeTimeSystemFn = std::function<std::unique_ptr<TestTimeSystem>()>;
 
-  TestTimeSystem* timeSystem() { return time_system_.get(); }
+  /**
+   * Returns a singleton time-system, creating a default one of there's not
+   * one already. This method is thread-safe.
+   *
+   * @return the time system.
+   */
+  TestTimeSystem& timeSystem(const MakeTimeSystemFn& make_time_system);
 
 private:
-  std::unique_ptr<TestTimeSystem> time_system_;
+  std::unique_ptr<TestTimeSystem> time_system_ GUARDED_BY(mutex_);
+  Thread::MutexBasicLockable mutex_;
 };
 
 // Implements the TestTimeSystem interface, delegating implementation of all
@@ -106,22 +110,22 @@ template <class TimeSystemVariant> class DelegatingTestTimeSystemBase : public T
 template <class TimeSystemVariant>
 class DelegatingTestTimeSystem : public DelegatingTestTimeSystemBase<TimeSystemVariant> {
 public:
-  DelegatingTestTimeSystem() {
-    TestTimeSystem* time_system = singleton_->timeSystem();
-    if (time_system == nullptr) {
-      time_system_ = new TimeSystemVariant;
-      singleton_->set(time_system_);
-    } else {
-      time_system_ = dynamic_cast<TimeSystemVariant*>(time_system);
-      RELEASE_ASSERT(time_system_, "Two different types of time-systems allocated");
-    }
-  }
+  DelegatingTestTimeSystem() : time_system_(initTimeSystem()) {}
 
-  TimeSystemVariant& timeSystem() override { return *time_system_; }
+  TimeSystemVariant& timeSystem() override { return time_system_; }
 
 private:
-  TimeSystemVariant* time_system_;
+  TimeSystemVariant& initTimeSystem() {
+    auto make_time_system = []() -> std::unique_ptr<TestTimeSystem> {
+      return std::make_unique<TimeSystemVariant>();
+    };
+    auto time_system = dynamic_cast<TimeSystemVariant*>(&singleton_->timeSystem(make_time_system));
+    RELEASE_ASSERT(time_system, "Two different types of time-systems allocated");
+    return *time_system;
+  }
+
   Test::Global<SingletonTimeSystemHelper> singleton_;
+  TimeSystemVariant& time_system_;
 };
 
 } // namespace Event