diff --git a/include/envoy/event/timer.h b/include/envoy/event/timer.h index 629fcdf102406..c2255e8dece94 100644 --- a/include/envoy/event/timer.h +++ b/include/envoy/event/timer.h @@ -42,15 +42,6 @@ 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. */ diff --git a/source/common/event/timer_impl.cc b/source/common/event/timer_impl.cc index 277437a59578b..ba9ea231c57bf 100644 --- a/source/common/event/timer_impl.cc +++ b/source/common/event/timer_impl.cc @@ -9,7 +9,7 @@ namespace Envoy { namespace Event { -void TimerUtils::microsecondsToTimeval(const std::chrono::microseconds& d, timeval& tv) { +void TimerUtils::millisecondsToTimeval(const std::chrono::milliseconds& d, timeval& tv) { std::chrono::seconds secs = std::chrono::duration_cast(d); std::chrono::microseconds usecs = std::chrono::duration_cast(d - secs); @@ -38,17 +38,12 @@ 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) { - enableHRTimer(d, object); -} - -void TimerImpl::enableHRTimer(const std::chrono::microseconds& d, - const ScopeTrackedObject* object = nullptr) { object_ = object; if (d.count() == 0) { event_active(&raw_event_, EV_TIMEOUT, 0); } else { timeval tv; - TimerUtils::microsecondsToTimeval(d, tv); + TimerUtils::millisecondsToTimeval(d, tv); event_add(&raw_event_, &tv); } } diff --git a/source/common/event/timer_impl.h b/source/common/event/timer_impl.h index 96bf489912949..172f1b142c0fa 100644 --- a/source/common/event/timer_impl.h +++ b/source/common/event/timer_impl.h @@ -16,7 +16,7 @@ namespace Event { */ class TimerUtils { public: - static void microsecondsToTimeval(const std::chrono::microseconds& d, timeval& tv); + static void millisecondsToTimeval(const std::chrono::milliseconds& d, timeval& tv); }; /** @@ -29,9 +29,6 @@ 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: diff --git a/test/common/event/BUILD b/test/common/event/BUILD index 76c55d3c27061..f3215f14dd1d9 100644 --- a/test/common/event/BUILD +++ b/test/common/event/BUILD @@ -18,7 +18,6 @@ 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", ], ) diff --git a/test/common/event/dispatcher_impl_test.cc b/test/common/event/dispatcher_impl_test.cc index 66229cf621b00..3059bea1584a7 100644 --- a/test/common/event/dispatcher_impl_test.cc +++ b/test/common/event/dispatcher_impl_test.cc @@ -10,7 +10,6 @@ #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" @@ -83,33 +82,6 @@ class DispatcherImplTest : public testing::Test { dispatcher_thread_->join(); } - void timerTest(std::function 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 scope_; // Used in InitializeStats, must outlive dispatcher_->exit(). Api::ApiPtr api_; Thread::ThreadPtr dispatcher_thread_; @@ -186,8 +158,31 @@ TEST_F(DispatcherImplTest, RunPostCallbacksLocking) { } TEST_F(DispatcherImplTest, Timer) { - timerTest([](Timer& timer) { timer.enableTimer(std::chrono::milliseconds(50)); }); - timerTest([](Timer& timer) { timer.enableHRTimer(std::chrono::microseconds(50)); }); + 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_); + } } TEST_F(DispatcherImplTest, TimerWithScope) { @@ -310,56 +305,6 @@ 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( - getTimerTiming(time_system, *dispatcher, *timer)) - .count(), - timing); - - std::chrono::microseconds us(timing); - timer->enableHRTimer(us); - EXPECT_EQ(std::chrono::duration_cast( - getTimerTiming(time_system, *dispatcher, *timer)) - .count(), - timing); - } } TEST(TimerImplTest, TimerValueConversion) { @@ -368,21 +313,21 @@ TEST(TimerImplTest, TimerValueConversion) { // Basic test with zero milliseconds. msecs = std::chrono::milliseconds(0); - TimerUtils::microsecondsToTimeval(msecs, tv); + TimerUtils::millisecondsToTimeval(msecs, tv); EXPECT_EQ(tv.tv_sec, 0); EXPECT_EQ(tv.tv_usec, 0); // 2050 milliseconds is 2 seconds and 50000 microseconds. msecs = std::chrono::milliseconds(2050); - TimerUtils::microsecondsToTimeval(msecs, tv); + TimerUtils::millisecondsToTimeval(msecs, tv); EXPECT_EQ(tv.tv_sec, 2); EXPECT_EQ(tv.tv_usec, 50000); // Check maximum value conversion. - const auto usecs = std::chrono::microseconds::duration::max(); - TimerUtils::microsecondsToTimeval(usecs, tv); - EXPECT_EQ(tv.tv_sec, usecs.count() / 1000000); - EXPECT_EQ(tv.tv_usec, usecs.count() % tv.tv_sec); + 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); } } // namespace diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index d4f644ae53317..a5ab4479e4a51 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -147,8 +147,6 @@ 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_{}; diff --git a/test/test_common/simulated_time_system.cc b/test/test_common/simulated_time_system.cc index 598cd688bccbc..43db9e511148a 100644 --- a/test/test_common/simulated_time_system.cc +++ b/test/test_common/simulated_time_system.cc @@ -61,11 +61,7 @@ class SimulatedTimeSystemHelper::Alarm : public Timer { // Timer void disableTimer() override; void enableTimer(const std::chrono::milliseconds& duration, - const ScopeTrackedObject* scope) override { - enableHRTimer(duration, scope); - }; - void enableHRTimer(const std::chrono::microseconds& duration, - const ScopeTrackedObject* scope) override; + const ScopeTrackedObject* scope) override; bool enabled() override { Thread::LockGuard lock(time_system_.mutex_); return armed_ || base_timer_->enabled(); @@ -181,8 +177,8 @@ void SimulatedTimeSystemHelper::Alarm::Alarm::disableTimerLockHeld() { } } -void SimulatedTimeSystemHelper::Alarm::Alarm::enableHRTimer( - const std::chrono::microseconds& duration, const ScopeTrackedObject* scope) { +void SimulatedTimeSystemHelper::Alarm::Alarm::enableTimer(const std::chrono::milliseconds& duration, + const ScopeTrackedObject* scope) { Thread::LockGuard lock(time_system_.mutex_); disableTimerLockHeld(); armed_ = true; @@ -291,7 +287,7 @@ int64_t SimulatedTimeSystemHelper::nextIndex() { } void SimulatedTimeSystemHelper::addAlarmLockHeld( - Alarm* alarm, const std::chrono::microseconds& duration) NO_THREAD_SAFETY_ANALYSIS { + Alarm* alarm, const std::chrono::milliseconds& duration) NO_THREAD_SAFETY_ANALYSIS { ASSERT(&(alarm->timeSystem()) == this); alarm->setTimeLockHeld(monotonic_time_ + duration); alarms_.insert(alarm); diff --git a/test/test_common/simulated_time_system.h b/test/test_common/simulated_time_system.h index 5f257a7c5ee07..7dfec54b012e6 100644 --- a/test/test_common/simulated_time_system.h +++ b/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::microseconds& duration) + void addAlarmLockHeld(Alarm*, const std::chrono::milliseconds& duration) EXCLUSIVE_LOCKS_REQUIRED(mutex_); void removeAlarmLockHeld(Alarm*) EXCLUSIVE_LOCKS_REQUIRED(mutex_);