test: Ensure only one thread advances time, and enable simulated-time for hds_integration_test.

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;

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<sockaddr*>(&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;

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",
     ],
 )
 

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);

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");

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<Network::Address::IpVersion>,
+                           public Event::TestUsingSimulatedTime,
                            public HttpIntegrationTest {
 public:
   HdsIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) {}

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",

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

test/test_common/contention.h

@@ -3,6 +3,8 @@
 #include <string>
 #include <vector>
 
+#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<bool> found_contention_{false};
 };
 
 } // namespace TestUtil

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

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

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

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

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<MonotonicTime::duration>(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<Duration>(std::chrono::milliseconds(50)), duration));
   const MonotonicTime end_time = monotonicTime() + duration;

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<uint32_t> pending_alarms_;
+  Thread::OnlyOneThread only_one_thread_;
 };
 
 // Represents a simulated time system, where time is advanced by calling

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);
 }
 

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<TestTimeSystem> {

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

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