Add TimerImpl::enableHRTimer - take two

include/envoy/event/timer.h

@@ -42,6 +42,15 @@ class Timer {
   virtual void enableTimer(const std::chrono::milliseconds& ms,
                            const ScopeTrackedObject* object = nullptr) PURE;
 
+  /**
+   * Enable a pending high resolution timeout. If a timeout is already pending, it will be reset to
+   * the new timeout.
+   *
+   * @param us supplies the duration of the alarm in microseconds.
+   * @param object supplies an optional scope for the duration of the alarm.
+   */
+  virtual void enableHRTimer(const std::chrono::microseconds& us,
+                             const ScopeTrackedObject* object = nullptr) PURE;
   /**
    * Return whether the timer is currently armed.
    */

source/common/event/timer_impl.cc

@@ -9,14 +9,6 @@
 namespace Envoy {
 namespace Event {
 
-void TimerUtils::millisecondsToTimeval(const std::chrono::milliseconds& d, timeval& tv) {
-  std::chrono::seconds secs = std::chrono::duration_cast<std::chrono::seconds>(d);
-  std::chrono::microseconds usecs = std::chrono::duration_cast<std::chrono::microseconds>(d - secs);
-
-  tv.tv_sec = secs.count();
-  tv.tv_usec = usecs.count();
-}
-
 TimerImpl::TimerImpl(Libevent::BasePtr& libevent, TimerCb cb, Dispatcher& dispatcher)
     : cb_(cb), dispatcher_(dispatcher) {
   ASSERT(cb_);
@@ -38,12 +30,23 @@ TimerImpl::TimerImpl(Libevent::BasePtr& libevent, TimerCb cb, Dispatcher& dispat
 void TimerImpl::disableTimer() { event_del(&raw_event_); }
 
 void TimerImpl::enableTimer(const std::chrono::milliseconds& d, const ScopeTrackedObject* object) {
+  timeval tv;
+  TimerUtils::durationToTimeval(d, tv);
+  internalEnableTimer(tv, object);
+}
+
+void TimerImpl::enableHRTimer(const std::chrono::microseconds& d,
+                              const ScopeTrackedObject* object = nullptr) {
+  timeval tv;
+  TimerUtils::durationToTimeval(d, tv);
+  internalEnableTimer(tv, object);
+}
+
+void TimerImpl::internalEnableTimer(const timeval& tv, const ScopeTrackedObject* object) {
   object_ = object;
-  if (d.count() == 0) {
+  if (tv.tv_sec == 0 && tv.tv_usec == 0) {
     event_active(&raw_event_, EV_TIMEOUT, 0);
   } else {
-    timeval tv;
-    TimerUtils::millisecondsToTimeval(d, tv);
     event_add(&raw_event_, &tv);
   }
 }

source/common/event/timer_impl.h

@@ -16,7 +16,34 @@ namespace Event {
  */
 class TimerUtils {
 public:
-  static void millisecondsToTimeval(const std::chrono::milliseconds& d, timeval& tv);
+  /**
+   * Intended for consumption by enable(HR)Timer, this method is templated method to avoid implicit
+   * duration conversions for its input arguments. This lets us have an opportunity to check bounds
+   * before doing any conversions. When the passed in duration exceeds INT32_MAX max seconds, the
+   * output will be clipped to yield INT32_MAX seconds and 0 microseconds for the
+   * output argument. We clip to INT32_MAX to guard against overflowing the timeval structure.
+   * Throws an EnvoyException on negative duration input.
+   * @tparam Duration std::chrono duration type, e.g. seconds, milliseconds, ...
+   * @param d duration value
+   * @param tv output parameter that will be updated
+   */
+  template <typename Duration> static void durationToTimeval(const Duration& d, timeval& tv) {
+    if (d.count() < 0) {
+      throw EnvoyException(
+          fmt::format("Negative duration passed to durationToTimeval(): {}", d.count()));
+    };
+    constexpr int64_t clip_to = INT32_MAX; // 136.102208 years
+    auto secs = std::chrono::duration_cast<std::chrono::seconds>(d);
+    if (secs.count() > clip_to) {
+      tv.tv_sec = clip_to;
+      tv.tv_usec = 0;
+      return;
+    }
+
+    auto usecs = std::chrono::duration_cast<std::chrono::microseconds>(d - secs);
+    tv.tv_sec = secs.count();
+    tv.tv_usec = usecs.count();
+  }
 };
 
 /**
@@ -28,10 +55,15 @@ class TimerImpl : public Timer, ImplBase {
 
   // Timer
   void disableTimer() override;
+
   void enableTimer(const std::chrono::milliseconds& d, const ScopeTrackedObject* scope) override;
+  void enableHRTimer(const std::chrono::microseconds& us,
+                     const ScopeTrackedObject* object) override;
+
   bool enabled() override;
 
 private:
+  void internalEnableTimer(const timeval& tv, const ScopeTrackedObject* scope);
   TimerCb cb_;
   Dispatcher& dispatcher_;
   // This has to be atomic for alarms which are handled out of thread, for

test/common/event/BUILD

@@ -18,6 +18,7 @@ envoy_cc_test(
         "//source/common/stats:isolated_store_lib",
         "//test/mocks:common_lib",
         "//test/mocks/stats:stats_mocks",
+        "//test/test_common:simulated_time_system_lib",
         "//test/test_common:utility_lib",
     ],
 )

test/common/event/dispatcher_impl_test.cc

@@ -10,6 +10,7 @@
 
 #include "test/mocks/common.h"
 #include "test/mocks/stats/mocks.h"
+#include "test/test_common/simulated_time_system.h"
 #include "test/test_common/utility.h"
 
 #include "gmock/gmock.h"
@@ -82,6 +83,33 @@ class DispatcherImplTest : public testing::Test {
     dispatcher_thread_->join();
   }
 
+  void timerTest(std::function<void(Timer&)> enable_timer_delegate) {
+    TimerPtr timer;
+    dispatcher_->post([this, &timer]() {
+      {
+        Thread::LockGuard lock(mu_);
+        timer = dispatcher_->createTimer([this]() {
+          {
+            Thread::LockGuard lock(mu_);
+            work_finished_ = true;
+          }
+          cv_.notifyOne();
+        });
+        EXPECT_FALSE(timer->enabled());
+      }
+      cv_.notifyOne();
+    });
+
+    Thread::LockGuard lock(mu_);
+    while (timer == nullptr) {
+      cv_.wait(mu_);
+    }
+    enable_timer_delegate(*timer);
+    while (!work_finished_) {
+      cv_.wait(mu_);
+    }
+  }
+
   NiceMock<Stats::MockStore> scope_; // Used in InitializeStats, must outlive dispatcher_->exit().
   Api::ApiPtr api_;
   Thread::ThreadPtr dispatcher_thread_;
@@ -158,31 +186,8 @@ TEST_F(DispatcherImplTest, RunPostCallbacksLocking) {
 }
 
 TEST_F(DispatcherImplTest, Timer) {
-  TimerPtr timer;
-  dispatcher_->post([this, &timer]() {
-    {
-      Thread::LockGuard lock(mu_);
-      timer = dispatcher_->createTimer([this]() {
-        {
-          Thread::LockGuard lock(mu_);
-          work_finished_ = true;
-        }
-        cv_.notifyOne();
-      });
-      EXPECT_FALSE(timer->enabled());
-    }
-    cv_.notifyOne();
-  });
-
-  Thread::LockGuard lock(mu_);
-  while (timer == nullptr) {
-    cv_.wait(mu_);
-  }
-  timer->enableTimer(std::chrono::milliseconds(50));
-
-  while (!work_finished_) {
-    cv_.wait(mu_);
-  }
+  timerTest([](Timer& timer) { timer.enableTimer(std::chrono::milliseconds(50)); });
+  timerTest([](Timer& timer) { timer.enableHRTimer(std::chrono::microseconds(50)); });
 }
 
 TEST_F(DispatcherImplTest, TimerWithScope) {
@@ -305,29 +310,101 @@ TEST(TimerImplTest, TimerEnabledDisabled) {
   EXPECT_TRUE(timer->enabled());
   dispatcher->run(Dispatcher::RunType::NonBlock);
   EXPECT_FALSE(timer->enabled());
+  timer->enableHRTimer(std::chrono::milliseconds(0));
+  EXPECT_TRUE(timer->enabled());
+  dispatcher->run(Dispatcher::RunType::NonBlock);
+  EXPECT_FALSE(timer->enabled());
+}
+
+class TimerImplTimingTest : public testing::Test {
+public:
+  std::chrono::nanoseconds getTimerTiming(Event::SimulatedTimeSystem& time_system,
+                                          Dispatcher& dispatcher, Event::Timer& timer) {
+    const auto start = time_system.monotonicTime();
+    EXPECT_TRUE(timer.enabled());
+    while (true) {
+      dispatcher.run(Dispatcher::RunType::NonBlock);
+      if (timer.enabled()) {
+        time_system.sleep(std::chrono::microseconds(1));
+      } else {
+        break;
+      }
+    }
+    return time_system.monotonicTime() - start;
+  }
+};
+
+// Test the timer with a series of timings and measure they fire accurately
+// using simulated time. enableTimer() should be precise at the millisecond
+// level, whereas enableHRTimer should be precise at the microsecond level.
+// For good measure, also check that '0'/immediate does what it says on the tin.
+TEST_F(TimerImplTimingTest, TheoreticalTimerTiming) {
+  Event::SimulatedTimeSystem time_system;
+  Api::ApiPtr api = Api::createApiForTest(time_system);
+  DispatcherPtr dispatcher(api->allocateDispatcher());
+  Event::TimerPtr timer = dispatcher->createTimer([&dispatcher] { dispatcher->exit(); });
+
+  const uint64_t timings[] = {0, 10, 50, 1234};
+  for (const uint64_t timing : timings) {
+    std::chrono::milliseconds ms(timing);
+    timer->enableTimer(ms);
+    EXPECT_EQ(std::chrono::duration_cast<std::chrono::milliseconds>(
+                  getTimerTiming(time_system, *dispatcher, *timer))
+                  .count(),
+              timing);
+
+    std::chrono::microseconds us(timing);
+    timer->enableHRTimer(us);
+    EXPECT_EQ(std::chrono::duration_cast<std::chrono::microseconds>(
+                  getTimerTiming(time_system, *dispatcher, *timer))
+                  .count(),
+              timing);
+  }
 }
 
-TEST(TimerImplTest, TimerValueConversion) {
+class TimerUtilsTest : public testing::Test {
+public:
+  template <typename Duration>
+  void checkConversion(const Duration& duration, const uint64_t expected_secs,
+                       const uint64_t expected_usecs) {
+    timeval tv;
+    TimerUtils::durationToTimeval(duration, tv);
+    EXPECT_EQ(tv.tv_sec, expected_secs);
+    EXPECT_EQ(tv.tv_usec, expected_usecs);
+  }
+};
+
+TEST_F(TimerUtilsTest, TimerNegativeValueThrows) {
   timeval tv;
-  std::chrono::milliseconds msecs;
+  const int negative_sample = -1;
+  EXPECT_THROW_WITH_MESSAGE(
+      TimerUtils::durationToTimeval(std::chrono::seconds(negative_sample), tv), EnvoyException,
+      fmt::format("Negative duration passed to durationToTimeval(): {}", negative_sample));
+}
+
+TEST_F(TimerUtilsTest, TimerValueConversion) {
+  // Check input is bounded.
+  checkConversion(std::chrono::nanoseconds::duration::max(), INT32_MAX, 0);
+  checkConversion(std::chrono::microseconds::duration::max(), INT32_MAX, 0);
+  checkConversion(std::chrono::milliseconds::duration::max(), INT32_MAX, 0);
+  checkConversion(std::chrono::seconds::duration::max(), INT32_MAX, 0);
+
+  // Test the clipping boundary
+  checkConversion(std::chrono::seconds(INT32_MAX) - std::chrono::seconds(1), INT32_MAX - 1, 0);
+  checkConversion(std::chrono::seconds(INT32_MAX) - std::chrono::nanoseconds(1), INT32_MAX - 1,
+                  999999);
 
   // Basic test with zero milliseconds.
-  msecs = std::chrono::milliseconds(0);
-  TimerUtils::millisecondsToTimeval(msecs, tv);
-  EXPECT_EQ(tv.tv_sec, 0);
-  EXPECT_EQ(tv.tv_usec, 0);
+  checkConversion(std::chrono::milliseconds(0), 0, 0);
 
   // 2050 milliseconds is 2 seconds and 50000 microseconds.
-  msecs = std::chrono::milliseconds(2050);
-  TimerUtils::millisecondsToTimeval(msecs, tv);
-  EXPECT_EQ(tv.tv_sec, 2);
-  EXPECT_EQ(tv.tv_usec, 50000);
-
-  // Check maximum value conversion.
-  msecs = std::chrono::milliseconds::duration::max();
-  TimerUtils::millisecondsToTimeval(msecs, tv);
-  EXPECT_EQ(tv.tv_sec, msecs.count() / 1000);
-  EXPECT_EQ(tv.tv_usec, (msecs.count() % tv.tv_sec) * 1000);
+  checkConversion(std::chrono::milliseconds(2050), 2, 50000);
+
+  // Some arbitrary tests for good measure.
+  checkConversion(std::chrono::microseconds(233), 0, 233);
+
+  // Some arbitrary tests for good measure.
+  checkConversion(std::chrono::milliseconds(600014), 600, 14000);
 }
 
 } // namespace

test/mocks/event/mocks.h

@@ -148,6 +148,8 @@ class MockTimer : public Timer {
   MOCK_METHOD0(disableTimer, void());
   MOCK_METHOD2(enableTimer,
                void(const std::chrono::milliseconds&, const ScopeTrackedObject* scope));
+  MOCK_METHOD2(enableHRTimer,
+               void(const std::chrono::microseconds&, const ScopeTrackedObject* scope));
   MOCK_METHOD0(enabled, bool());
 
   MockDispatcher* dispatcher_{};

test/test_common/simulated_time_system.cc

@@ -61,7 +61,11 @@ class SimulatedTimeSystemHelper::Alarm : public Timer {
   // Timer
   void disableTimer() override;
   void enableTimer(const std::chrono::milliseconds& duration,
-                   const ScopeTrackedObject* scope) override;
+                   const ScopeTrackedObject* scope) override {
+    enableHRTimer(duration, scope);
+  };
+  void enableHRTimer(const std::chrono::microseconds& duration,
+                     const ScopeTrackedObject* scope) override;
   bool enabled() override {
     Thread::LockGuard lock(time_system_.mutex_);
     return armed_ || base_timer_->enabled();
@@ -177,8 +181,8 @@ void SimulatedTimeSystemHelper::Alarm::Alarm::disableTimerLockHeld() {
   }
 }
 
-void SimulatedTimeSystemHelper::Alarm::Alarm::enableTimer(const std::chrono::milliseconds& duration,
-                                                          const ScopeTrackedObject* scope) {
+void SimulatedTimeSystemHelper::Alarm::Alarm::enableHRTimer(
+    const std::chrono::microseconds& duration, const ScopeTrackedObject* scope) {
   Thread::LockGuard lock(time_system_.mutex_);
   disableTimerLockHeld();
   armed_ = true;
@@ -287,7 +291,7 @@ int64_t SimulatedTimeSystemHelper::nextIndex() {
 }
 
 void SimulatedTimeSystemHelper::addAlarmLockHeld(
-    Alarm* alarm, const std::chrono::milliseconds& duration) NO_THREAD_SAFETY_ANALYSIS {
+    Alarm* alarm, const std::chrono::microseconds& duration) NO_THREAD_SAFETY_ANALYSIS {
   ASSERT(&(alarm->timeSystem()) == this);
   alarm->setTimeLockHeld(monotonic_time_ + duration);
   alarms_.insert(alarm);

test/test_common/simulated_time_system.h

@@ -86,7 +86,7 @@ class SimulatedTimeSystemHelper : public TestTimeSystem {
   int64_t nextIndex();
 
   // Adds/removes an alarm.
-  void addAlarmLockHeld(Alarm*, const std::chrono::milliseconds& duration)
+  void addAlarmLockHeld(Alarm*, const std::chrono::microseconds& duration)
       EXCLUSIVE_LOCKS_REQUIRED(mutex_);
   void removeAlarmLockHeld(Alarm*) EXCLUSIVE_LOCKS_REQUIRED(mutex_);
 

tools/spelling_dictionary.txt

@@ -994,6 +994,7 @@ thru
 timespan
 timestamp
 timestamps
+timeval
 tm
 tmp
 tmpfile