diff --git a/source/exe/main_common.h b/source/exe/main_common.h index 28e5708018a58..f88fcbb457183 100644 --- a/source/exe/main_common.h +++ b/source/exe/main_common.h @@ -106,6 +106,12 @@ class MainCommon { static std::string hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len, bool hot_restart_enabled); + /** + * @return a pointer to the server instance, or nullptr if initialized into + * validation mode. + */ + Server::Instance* server() { return base_.server(); } + private: #ifdef ENVOY_HANDLE_SIGNALS Envoy::SignalAction handle_sigs; diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index 1fb34c4a879c4..a2a6dc5e002a3 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -160,8 +160,9 @@ int HotRestartImpl::bindDomainSocket(uint64_t id) { Api::SysCallIntResult result = os_sys_calls.bind(fd, reinterpret_cast(&address), sizeof(address)); if (result.rc_ != 0) { - throw EnvoyException( - fmt::format("unable to bind domain socket with id={} (see --base-id option)", id)); + throw EnvoyException(fmt::format( + "unable to bind domain socket with id={}, (see --base-id option), address={}, errno={}: {}", + id, result.errno_, strerror(result.errno_))); } return fd; diff --git a/test/common/common/BUILD b/test/common/common/BUILD index dc68632fe5bb6..598ad406adc9f 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -119,6 +119,7 @@ envoy_cc_test( deps = [ "//source/common/common:mutex_tracer_lib", "//test/test_common:contention_lib", + "//test/test_common:utility_lib", ], ) diff --git a/test/common/common/mutex_tracer_test.cc b/test/common/common/mutex_tracer_test.cc index 042b0469f1994..e72fc3a995b94 100644 --- a/test/common/common/mutex_tracer_test.cc +++ b/test/common/common/mutex_tracer_test.cc @@ -5,6 +5,7 @@ #include "common/common/mutex_tracer_impl.h" #include "test/test_common/contention.h" +#include "test/test_common/utility.h" #include "absl/synchronization/mutex.h" #include "gtest/gtest.h" @@ -72,13 +73,16 @@ TEST_F(MutexTracerTest, TryLockNoContention) { } TEST_F(MutexTracerTest, TwoThreadsWithContention) { + Api::ApiPtr api = Api::createApiForTest(); + int64_t prev_num_contentions = tracer_.numContentions(); for (int i = 1; i <= 10; ++i) { int64_t curr_num_lifetime_wait_cycles = tracer_.lifetimeWaitCycles(); - Thread::TestUtil::ContentionGenerator contention_generator; - + Thread::TestUtil::ContentionGenerator contention_generator(*api); contention_generator.generateContention(tracer_); - EXPECT_EQ(tracer_.numContentions(), i); + int64_t num_contentions = tracer_.numContentions(); + EXPECT_LT(prev_num_contentions, num_contentions); + prev_num_contentions = num_contentions; EXPECT_GT(tracer_.currentWaitCycles(), 0); // This shouldn't be hardcoded. EXPECT_GT(tracer_.lifetimeWaitCycles(), 0); EXPECT_GT(tracer_.lifetimeWaitCycles(), curr_num_lifetime_wait_cycles); diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 1e5136b96e249..2b1aee544fec3 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -308,7 +308,7 @@ TEST_P(AdminRequestTest, AdminRequestContentionEnabled) { waitForEnvoyRun(); // Induce contention to guarantee a non-zero num_contentions count. - Thread::TestUtil::ContentionGenerator contention_generator; + Thread::TestUtil::ContentionGenerator contention_generator(main_common_->server()->api()); contention_generator.generateContention(MutexTracerImpl::getOrCreateTracer()); std::string response = adminRequest("/contention", "GET"); diff --git a/test/integration/hds_integration_test.cc b/test/integration/hds_integration_test.cc index dd8fe59451747..eb4eba9ce2893 100644 --- a/test/integration/hds_integration_test.cc +++ b/test/integration/hds_integration_test.cc @@ -26,6 +26,7 @@ namespace { // TODO(jmarantz): switch this to simulated-time after debugging flakes. class HdsIntegrationTest : public testing::TestWithParam, + public Event::TestUsingSimulatedTime, public HttpIntegrationTest { public: HdsIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) {} diff --git a/test/test_common/BUILD b/test/test_common/BUILD index 6044a39b67715..293640e4d3cee 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -92,7 +92,9 @@ envoy_cc_test_library( "abseil_strings", ], deps = [ + ":file_system_for_test_lib", ":test_time_lib", + ":thread_factory_for_test_lib", "//include/envoy/buffer:buffer_interface", "//include/envoy/http:codec_interface", "//include/envoy/network:address_interface", @@ -117,6 +119,26 @@ envoy_cc_test_library( ], ) +envoy_cc_test_library( + name = "thread_factory_for_test_lib", + srcs = ["thread_factory_for_test.cc"], + hdrs = ["thread_factory_for_test.h"], + deps = [ + "//source/common/common:thread_lib", + "//source/common/common:utility_lib", + ], +) + +envoy_cc_test_library( + name = "file_system_for_test_lib", + srcs = ["file_system_for_test.cc"], + hdrs = ["file_system_for_test.h"], + deps = [ + "//source/common/common:utility_lib", + "//source/common/filesystem:filesystem_lib", + ], +) + envoy_cc_test( name = "utility_test", srcs = ["utility_test.cc"], @@ -161,12 +183,20 @@ envoy_cc_test( ], ) +envoy_cc_test_library( + name = "only_one_thread_lib", + srcs = ["only_one_thread.cc"], + hdrs = ["only_one_thread.h"], + deps = [":thread_factory_for_test_lib"], +) + envoy_cc_test_library( name = "test_time_lib", srcs = ["test_time.cc"], hdrs = ["test_time.h"], deps = [ ":global_lib", + ":only_one_thread_lib", ":test_time_system_interface", "//source/common/event:real_time_system_lib", ], @@ -188,6 +218,7 @@ envoy_cc_test_library( srcs = ["simulated_time_system.cc"], hdrs = ["simulated_time_system.h"], deps = [ + ":only_one_thread_lib", ":test_time_system_interface", "//source/common/event:event_impl_base_lib", "//source/common/event:real_time_system_lib", diff --git a/test/test_common/contention.cc b/test/test_common/contention.cc index 10df692c4d8ef..9160120ba5faa 100644 --- a/test/test_common/contention.cc +++ b/test/test_common/contention.cc @@ -19,14 +19,25 @@ Envoy::Thread::ThreadPtr ContentionGenerator::launchThread(MutexTracerImpl& trac } void ContentionGenerator::holdUntilContention(MutexTracerImpl& tracer) { + Event::DispatcherPtr dispatcher = api_.allocateDispatcher(); + Event::TimerPtr timer = dispatcher->createTimer([&dispatcher]() { dispatcher->exit(); }); + auto sleep_ms = [&timer, &dispatcher](int num_ms) { + timer->enableTimer(std::chrono::milliseconds(num_ms)); + dispatcher->run(Event::Dispatcher::RunType::RunUntilExit); + }; int64_t curr_num_contentions = tracer.numContentions(); - while (tracer.numContentions() == curr_num_contentions) { - test_time_.timeSystem().sleep(std::chrono::milliseconds(1)); - LockGuard lock(mutex_); - // We hold the lock 90% of the time to ensure both contention and eventual acquisition, which - // is needed to bump numContentions(). - test_time_.timeSystem().sleep(std::chrono::milliseconds(9)); - } + do { + sleep_ms(1); + { + LockGuard lock(mutex_); + // We hold the lock 90% of the time to ensure both contention and eventual acquisition, which + // is needed to bump numContentions(). + sleep_ms(9); + } + if (tracer.numContentions() > curr_num_contentions) { + found_contention_ = true; + } + } while (!found_contention_); } } // namespace TestUtil diff --git a/test/test_common/contention.h b/test/test_common/contention.h index 77ce14d4872a8..d90ab20d4eea0 100644 --- a/test/test_common/contention.h +++ b/test/test_common/contention.h @@ -3,6 +3,8 @@ #include #include +#include "envoy/api/api.h" + #include "common/common/lock_guard.h" #include "common/common/mutex_tracer_impl.h" #include "common/common/thread.h" @@ -20,6 +22,8 @@ namespace TestUtil { class ContentionGenerator { public: + ContentionGenerator(Api::Api& api) : api_(api) {} + /** * Generates at least once occurrence of mutex contention, as measured by tracer. */ @@ -30,7 +34,8 @@ class ContentionGenerator { void holdUntilContention(MutexTracerImpl& tracer); MutexBasicLockable mutex_; - DangerousDeprecatedTestTime test_time_; + Api::Api& api_; + std::atomic found_contention_{false}; }; } // namespace TestUtil diff --git a/test/test_common/file_system_for_test.cc b/test/test_common/file_system_for_test.cc new file mode 100644 index 0000000000000..68d24fd9105f9 --- /dev/null +++ b/test/test_common/file_system_for_test.cc @@ -0,0 +1,20 @@ +#include "common/filesystem/filesystem_impl.h" + +namespace Envoy { + +namespace Filesystem { + +// TODO(sesmith177) Tests should get the Filesystem::Instance from the same location as the main +// code +Instance& fileSystemForTest() { +#ifdef WIN32 + static InstanceImplWin32* file_system = new InstanceImplWin32(); +#else + static InstanceImplPosix* file_system = new InstanceImplPosix(); +#endif + return *file_system; +} + +} // namespace Filesystem + +} // namespace Envoy diff --git a/test/test_common/file_system_for_test.h b/test/test_common/file_system_for_test.h new file mode 100644 index 0000000000000..43a443f68b6f5 --- /dev/null +++ b/test/test_common/file_system_for_test.h @@ -0,0 +1,9 @@ +#include "envoy/filesystem/filesystem.h" + +namespace Envoy { + +namespace Filesystem { +Instance& fileSystemForTest(); +} // namespace Filesystem + +} // namespace Envoy diff --git a/test/test_common/only_one_thread.cc b/test/test_common/only_one_thread.cc new file mode 100644 index 0000000000000..a6a5869b7eda6 --- /dev/null +++ b/test/test_common/only_one_thread.cc @@ -0,0 +1,25 @@ +#include "test/test_common/only_one_thread.h" + +#include "envoy/thread/thread.h" + +#include "common/common/lock_guard.h" + +#include "test/test_common/thread_factory_for_test.h" + +namespace Envoy { +namespace Thread { + +OnlyOneThread::OnlyOneThread() : thread_factory_(threadFactoryForTest()) {} + +void OnlyOneThread::checkOneThread() { + LockGuard lock(mutex_); + if (thread_advancing_time_ == nullptr) { + thread_advancing_time_ = thread_factory_.currentThreadId(); + } else { + RELEASE_ASSERT(thread_advancing_time_->isCurrentThreadId(), + "time should only be advanced on one thread in the context of a test"); + } +} + +} // namespace Thread +} // namespace Envoy diff --git a/test/test_common/only_one_thread.h b/test/test_common/only_one_thread.h new file mode 100644 index 0000000000000..1051c632e78a0 --- /dev/null +++ b/test/test_common/only_one_thread.h @@ -0,0 +1,28 @@ +#pragma once + +#include "common/common/assert.h" +#include "common/common/thread.h" + +namespace Envoy { +namespace Thread { + +// Ensures that an operation is performed on only one thread. The first caller +// to OnlyOneThread::checkOneThread establishes the thread ID, and subsequent +// ones will assert-fail if they do not match. +class OnlyOneThread { +public: + OnlyOneThread(); + + /** + * Ensures that one thread is used in a testcase to access some resource. + */ + void checkOneThread(); + +private: + ThreadFactory& thread_factory_; + ThreadIdPtr thread_advancing_time_ GUARDED_BY(mutex_); + mutable MutexBasicLockable mutex_; +}; + +} // namespace Thread +} // namespace Envoy diff --git a/test/test_common/simulated_time_system.cc b/test/test_common/simulated_time_system.cc index 204c47943a4c0..a54767114b119 100644 --- a/test/test_common/simulated_time_system.cc +++ b/test/test_common/simulated_time_system.cc @@ -207,6 +207,7 @@ MonotonicTime SimulatedTimeSystemHelper::monotonicTime() { } void SimulatedTimeSystemHelper::sleep(const Duration& duration) { + only_one_thread_.checkOneThread(); mutex_.lock(); MonotonicTime monotonic_time = monotonic_time_ + std::chrono::duration_cast(duration); @@ -216,6 +217,7 @@ void SimulatedTimeSystemHelper::sleep(const Duration& duration) { Thread::CondVar::WaitStatus SimulatedTimeSystemHelper::waitFor( Thread::MutexBasicLockable& mutex, Thread::CondVar& condvar, const Duration& duration) noexcept EXCLUSIVE_LOCKS_REQUIRED(mutex) { + only_one_thread_.checkOneThread(); const Duration real_time_poll_delay( std::min(std::chrono::duration_cast(std::chrono::milliseconds(50)), duration)); const MonotonicTime end_time = monotonicTime() + duration; diff --git a/test/test_common/simulated_time_system.h b/test/test_common/simulated_time_system.h index 11ccad33a8e87..7dfec54b012e6 100644 --- a/test/test_common/simulated_time_system.h +++ b/test/test_common/simulated_time_system.h @@ -6,6 +6,7 @@ #include "common/common/thread.h" #include "common/common/utility.h" +#include "test/test_common/only_one_thread.h" #include "test/test_common/test_time_system.h" namespace Envoy { @@ -102,6 +103,7 @@ class SimulatedTimeSystemHelper : public TestTimeSystem { uint64_t index_ GUARDED_BY(mutex_); mutable Thread::MutexBasicLockable mutex_; std::atomic pending_alarms_; + Thread::OnlyOneThread only_one_thread_; }; // Represents a simulated time system, where time is advanced by calling diff --git a/test/test_common/test_time.cc b/test/test_common/test_time.cc index 3b97d7f6afb36..09a5391cf128f 100644 --- a/test/test_common/test_time.cc +++ b/test/test_common/test_time.cc @@ -18,11 +18,15 @@ TestTimeSystem& GlobalTimeSystem::timeSystem() { return singleton_->timeSystem(make_real_time_system); } -void TestRealTimeSystem::sleep(const Duration& duration) { std::this_thread::sleep_for(duration); } +void TestRealTimeSystem::sleep(const Duration& duration) { + only_one_thread_.checkOneThread(); + std::this_thread::sleep_for(duration); +} Thread::CondVar::WaitStatus TestRealTimeSystem::waitFor(Thread::MutexBasicLockable& lock, Thread::CondVar& condvar, const Duration& duration) noexcept { + only_one_thread_.checkOneThread(); return condvar.waitFor(lock, duration); } diff --git a/test/test_common/test_time.h b/test/test_common/test_time.h index 4dc8bdb84cd6a..e97af83100640 100644 --- a/test/test_common/test_time.h +++ b/test/test_common/test_time.h @@ -3,6 +3,7 @@ #include "common/event/real_time_system.h" #include "test/test_common/global.h" +#include "test/test_common/only_one_thread.h" #include "test/test_common/test_time_system.h" namespace Envoy { @@ -27,6 +28,7 @@ class TestRealTimeSystem : public TestTimeSystem { private: Event::RealTimeSystem real_time_system_; + Thread::OnlyOneThread only_one_thread_; }; class GlobalTimeSystem : public DelegatingTestTimeSystemBase { diff --git a/test/test_common/thread_factory_for_test.cc b/test/test_common/thread_factory_for_test.cc new file mode 100644 index 0000000000000..0bdd93e0cdde9 --- /dev/null +++ b/test/test_common/thread_factory_for_test.cc @@ -0,0 +1,19 @@ +#include "common/common/thread_impl.h" + +namespace Envoy { + +namespace Thread { + +// TODO(sesmith177) Tests should get the ThreadFactory from the same location as the main code +ThreadFactory& threadFactoryForTest() { +#ifdef WIN32 + static ThreadFactoryImplWin32* thread_factory = new ThreadFactoryImplWin32(); +#else + static ThreadFactoryImplPosix* thread_factory = new ThreadFactoryImplPosix(); +#endif + return *thread_factory; +} + +} // namespace Thread + +} // namespace Envoy diff --git a/test/test_common/thread_factory_for_test.h b/test/test_common/thread_factory_for_test.h new file mode 100644 index 0000000000000..26060fca781e6 --- /dev/null +++ b/test/test_common/thread_factory_for_test.h @@ -0,0 +1,9 @@ +#include "envoy/thread/thread.h" + +namespace Envoy { + +namespace Thread { +ThreadFactory& threadFactoryForTest(); +} // namespace Thread + +} // namespace Envoy diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 8a92397b47810..3589718497169 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -401,35 +401,6 @@ MockedTestAllocator::~MockedTestAllocator() {} } // namespace Stats -namespace Thread { - -// TODO(sesmith177) Tests should get the ThreadFactory from the same location as the main code -ThreadFactory& threadFactoryForTest() { -#ifdef WIN32 - static ThreadFactoryImplWin32* thread_factory = new ThreadFactoryImplWin32(); -#else - static ThreadFactoryImplPosix* thread_factory = new ThreadFactoryImplPosix(); -#endif - return *thread_factory; -} - -} // namespace Thread - -namespace Filesystem { - -// TODO(sesmith177) Tests should get the Filesystem::Instance from the same location as the main -// code -Instance& fileSystemForTest() { -#ifdef WIN32 - static InstanceImplWin32* file_system = new InstanceImplWin32(); -#else - static InstanceImplPosix* file_system = new InstanceImplPosix(); -#endif - return *file_system; -} - -} // namespace Filesystem - namespace Api { class TestImplProvider { diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 063e6c89ae9ed..7318664e40f33 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -24,7 +24,9 @@ #include "common/stats/fake_symbol_table_impl.h" #include "common/stats/raw_stat_data.h" +#include "test/test_common/file_system_for_test.h" #include "test/test_common/printers.h" +#include "test/test_common/thread_factory_for_test.h" #include "absl/time/time.h" #include "gmock/gmock.h" @@ -534,14 +536,6 @@ class MockedTestAllocator : public TestAllocator { } // namespace Stats -namespace Thread { -ThreadFactory& threadFactoryForTest(); -} // namespace Thread - -namespace Filesystem { -Instance& fileSystemForTest(); -} // namespace Filesystem - namespace Api { ApiPtr createApiForTest(); ApiPtr createApiForTest(Stats::Store& stat_store);