diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index 571f2665d9471..eb1c531cf3068 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/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 diff --git a/test/test_common/BUILD b/test/test_common/BUILD index b314e54f0c13e..83a43c77e4ab5 100644 --- a/test/test_common/BUILD +++ b/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", diff --git a/test/test_common/test_time.cc b/test/test_common/test_time.cc index 24bd072e6b4fb..3b97d7f6afb36 100644 --- a/test/test_common/test_time.cc +++ b/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 { + return std::make_unique(); + }; + 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, diff --git a/test/test_common/test_time.h b/test/test_common/test_time.h index ea0411f6a2c95..ca659426d044d 100644 --- a/test/test_common/test_time.h +++ b/test/test_common/test_time.h @@ -31,13 +31,7 @@ class TestRealTimeSystem : public TestTimeSystem { class GlobalTimeSystem : public DelegatingTestTimeSystemBase { 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 singleton_; diff --git a/test/test_common/test_time_system.cc b/test/test_common/test_time_system.cc new file mode 100644 index 0000000000000..a24763e9c07db --- /dev/null +++ b/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 diff --git a/test/test_common/test_time_system.h b/test/test_common/test_time_system.h index cced13dbb6b93..864b4a4286be8 100644 --- a/test/test_common/test_time_system.h +++ b/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()>; - 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 time_system_; + std::unique_ptr time_system_ GUARDED_BY(mutex_); + Thread::MutexBasicLockable mutex_; }; // Implements the TestTimeSystem interface, delegating implementation of all @@ -106,22 +110,22 @@ template class DelegatingTestTimeSystemBase : public T template class DelegatingTestTimeSystem : public DelegatingTestTimeSystemBase { public: - DelegatingTestTimeSystem() { - TestTimeSystem* time_system = singleton_->timeSystem(); - if (time_system == nullptr) { - time_system_ = new TimeSystemVariant; - singleton_->set(time_system_); - } else { - time_system_ = dynamic_cast(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 { + return std::make_unique(); + }; + auto time_system = dynamic_cast(&singleton_->timeSystem(make_time_system)); + RELEASE_ASSERT(time_system, "Two different types of time-systems allocated"); + return *time_system; + } + Test::Global singleton_; + TimeSystemVariant& time_system_; }; } // namespace Event