diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index bd265aaccff6b..65f45cbb9c8e7 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -49,6 +49,7 @@ Removed Config or Runtime ------------------------- *Normally occurs at the end of the* :ref:`deprecation period ` +* event: removed ``envoy.reloadable_features.activate_timers_next_event_loop`` runtime guard and legacy code path. * http: removed ``envoy.reloadable_features.allow_500_after_100`` runtime guard and the legacy code path. * http: removed ``envoy.reloadable_features.always_apply_route_header_rules`` runtime guard and legacy code path. * http: removed ``envoy.reloadable_features.hcm_stream_error_on_invalid_message`` for disabling closing HTTP/1.1 connections on error. Connection-closing can still be disabled by setting the HTTP/1 configuration :ref:`override_stream_error_on_invalid_http_message `. diff --git a/source/common/event/BUILD b/source/common/event/BUILD index 333755eeac351..35e5bb6601b1b 100644 --- a/source/common/event/BUILD +++ b/source/common/event/BUILD @@ -186,7 +186,6 @@ envoy_cc_library( "//include/envoy/event:timer_interface", "//source/common/common:scope_tracker", "//source/common/common:utility_lib", - "//source/common/runtime:runtime_features_lib", ], ) diff --git a/source/common/event/libevent_scheduler.h b/source/common/event/libevent_scheduler.h index d9aeecd387ad9..063f3061a5ce8 100644 --- a/source/common/event/libevent_scheduler.h +++ b/source/common/event/libevent_scheduler.h @@ -43,8 +43,6 @@ namespace Event { // The same mechanism implements both of these operations, so they are invoked as a group. // - Event::SchedulableCallback::scheduleCallbackCurrentIteration(). Each of these callbacks is // scheduled and invoked independently. -// - Event::Timer::enableTimer(0) if "envoy.reloadable_features.activate_timers_next_event_loop" -// runtime feature is disabled. // // Event::FileEvent::activate and Event::SchedulableCallback::scheduleCallbackNextIteration are // implemented as libevent timers with a deadline of 0. Both of these actions are moved to the work diff --git a/source/common/event/timer_impl.cc b/source/common/event/timer_impl.cc index 41a63daf98d30..3a2189f95c40a 100644 --- a/source/common/event/timer_impl.cc +++ b/source/common/event/timer_impl.cc @@ -3,7 +3,6 @@ #include #include "common/common/assert.h" -#include "common/runtime/runtime_features.h" #include "event2/event.h" @@ -11,16 +10,7 @@ namespace Envoy { namespace Event { TimerImpl::TimerImpl(Libevent::BasePtr& libevent, TimerCb cb, Dispatcher& dispatcher) - : cb_(cb), dispatcher_(dispatcher), - activate_timers_next_event_loop_( - // Only read the runtime feature if the runtime loader singleton has already been created. - // Accessing runtime features too early in the initialization sequence triggers logging - // and the logging code itself depends on the use of timers. Attempts to log while - // initializing the logging subsystem will result in a crash. - Runtime::LoaderSingleton::getExisting() - ? Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.activate_timers_next_event_loop") - : true) { + : cb_(cb), dispatcher_(dispatcher) { ASSERT(cb_); evtimer_assign( &raw_event_, libevent.get(), @@ -59,11 +49,7 @@ void TimerImpl::internalEnableTimer(const timeval& tv, const ScopeTrackedObject* ASSERT(dispatcher_.isThreadSafe()); object_ = object; - if (!activate_timers_next_event_loop_ && tv.tv_sec == 0 && tv.tv_usec == 0) { - event_active(&raw_event_, EV_TIMEOUT, 0); - } else { - event_add(&raw_event_, &tv); - } + event_add(&raw_event_, &tv); } bool TimerImpl::enabled() { diff --git a/source/common/event/timer_impl.h b/source/common/event/timer_impl.h index f7513dda996c6..bd56d528409e3 100644 --- a/source/common/event/timer_impl.h +++ b/source/common/event/timer_impl.h @@ -70,11 +70,6 @@ class TimerImpl : public Timer, ImplBase { // example if the DispatcherImpl::post is called by two threads, they race to // both set this to null. std::atomic object_{}; - - // Latched "envoy.reloadable_features.activate_timers_next_event_loop" runtime feature. If true, - // timers scheduled with a 0 time delta are evaluated in the next iteration of the event loop - // after polling and activating new fd events. - const bool activate_timers_next_event_loop_; }; } // namespace Event diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index c44cc2a55ad38..0f8995fdb3f12 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -56,7 +56,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.test_feature_true", // Begin alphabetically sorted section. "envoy.deprecated_features.allow_deprecated_extension_names", - "envoy.reloadable_features.activate_timers_next_event_loop", "envoy.reloadable_features.add_and_validate_scheme_header", "envoy.reloadable_features.allow_preconnect", "envoy.reloadable_features.allow_response_for_timeout", diff --git a/test/common/event/dispatcher_impl_test.cc b/test/common/event/dispatcher_impl_test.cc index 4194c46b6b675..fab86c343f1bf 100644 --- a/test/common/event/dispatcher_impl_test.cc +++ b/test/common/event/dispatcher_impl_test.cc @@ -828,20 +828,15 @@ TEST_F(DispatcherMonotonicTimeTest, ApproximateMonotonicTime) { dispatcher_->run(Dispatcher::RunType::Block); } -class TimerImplTest : public testing::TestWithParam { +class TimerImplTest : public testing::Test { protected: TimerImplTest() { - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.activate_timers_next_event_loop", - activateTimersNextEventLoop() ? "true" : "false"}}); // Hook into event loop prepare and check events. evwatch_prepare_new(&libevent_base_, onWatcherReady, &prepare_watcher_); evwatch_check_new(&libevent_base_, onCheck, this); } ~TimerImplTest() override { ASSERT(check_callbacks_.empty()); } - bool activateTimersNextEventLoop() { return GetParam(); } - // Run a callback inside the event loop. The libevent monotonic time used for timer registration // is frozen while within this callback, so timers enabled within this callback end up with the // requested relative registration times. The callback can invoke advanceLibeventTime() to force @@ -918,9 +913,7 @@ class TimerImplTest : public testing::TestWithParam { bool in_event_loop_{}; }; -INSTANTIATE_TEST_SUITE_P(DelayActivation, TimerImplTest, testing::Bool()); - -TEST_P(TimerImplTest, TimerEnabledDisabled) { +TEST_F(TimerImplTest, TimerEnabledDisabled) { InSequence s; Event::TimerPtr timer = dispatcher_->createTimer([] {}); @@ -937,7 +930,7 @@ TEST_P(TimerImplTest, TimerEnabledDisabled) { EXPECT_FALSE(timer->enabled()); } -TEST_P(TimerImplTest, ChangeTimerBackwardsBeforeRun) { +TEST_F(TimerImplTest, ChangeTimerBackwardsBeforeRun) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -965,26 +958,18 @@ TEST_P(TimerImplTest, ChangeTimerBackwardsBeforeRun) { }); } -TEST_P(TimerImplTest, ChangeTimerForwardsToZeroBeforeRun) { +TEST_F(TimerImplTest, ChangeTimerForwardsToZeroBeforeRun) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); ReadyWatcher watcher2; Event::TimerPtr timer2 = dispatcher_->createTimer([&] { watcher2.ready(); }); - if (activateTimersNextEventLoop()) { - // Expect watcher1 to trigger first because timer1's deadline was moved forward. - InSequence s; - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher1, ready()); - EXPECT_CALL(watcher2, ready()); - } else { - // Timers execute in the wrong order. - InSequence s; - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher1, ready()); - } + // Expect watcher1 to trigger first because timer1's deadline was moved forward. + InSequence s; + EXPECT_CALL(prepare_watcher_, ready()); + EXPECT_CALL(watcher1, ready()); + EXPECT_CALL(watcher2, ready()); runInEventLoop([&]() { timer1->enableTimer(std::chrono::milliseconds(2)); timer2->enableTimer(std::chrono::milliseconds(1)); @@ -995,7 +980,7 @@ TEST_P(TimerImplTest, ChangeTimerForwardsToZeroBeforeRun) { }); } -TEST_P(TimerImplTest, ChangeTimerForwardsToNonZeroBeforeRun) { +TEST_F(TimerImplTest, ChangeTimerForwardsToNonZeroBeforeRun) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -1017,7 +1002,7 @@ TEST_P(TimerImplTest, ChangeTimerForwardsToNonZeroBeforeRun) { }); } -TEST_P(TimerImplTest, ChangeLargeTimerForwardToZeroBeforeRun) { +TEST_F(TimerImplTest, ChangeLargeTimerForwardToZeroBeforeRun) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -1036,7 +1021,7 @@ TEST_P(TimerImplTest, ChangeLargeTimerForwardToZeroBeforeRun) { }); } -TEST_P(TimerImplTest, ChangeLargeTimerForwardToNonZeroBeforeRun) { +TEST_F(TimerImplTest, ChangeLargeTimerForwardToNonZeroBeforeRun) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -1059,7 +1044,7 @@ TEST_P(TimerImplTest, ChangeLargeTimerForwardToNonZeroBeforeRun) { } // Timers scheduled at different times execute in order. -TEST_P(TimerImplTest, TimerOrdering) { +TEST_F(TimerImplTest, TimerOrdering) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -1071,17 +1056,10 @@ TEST_P(TimerImplTest, TimerOrdering) { // Expect watcher calls to happen in order since timers have different times. InSequence s; - if (activateTimersNextEventLoop()) { - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher1, ready()); - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher3, ready()); - } else { - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher1, ready()); - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher3, ready()); - } + EXPECT_CALL(prepare_watcher_, ready()); + EXPECT_CALL(watcher1, ready()); + EXPECT_CALL(watcher2, ready()); + EXPECT_CALL(watcher3, ready()); runInEventLoop([&]() { timer1->enableTimer(std::chrono::milliseconds(0)); @@ -1098,7 +1076,7 @@ TEST_P(TimerImplTest, TimerOrdering) { } // Alarms that are scheduled to execute and are cancelled do not trigger. -TEST_P(TimerImplTest, TimerOrderAndDisableAlarm) { +TEST_F(TimerImplTest, TimerOrderAndDisableAlarm) { ReadyWatcher watcher3; Event::TimerPtr timer3 = dispatcher_->createTimer([&] { watcher3.ready(); }); @@ -1132,7 +1110,7 @@ TEST_P(TimerImplTest, TimerOrderAndDisableAlarm) { // Change the registration time for a timer that is already activated by disabling and re-enabling // the timer. Verify that execution is delayed. -TEST_P(TimerImplTest, TimerOrderDisableAndReschedule) { +TEST_F(TimerImplTest, TimerOrderDisableAndReschedule) { ReadyWatcher watcher4; Event::TimerPtr timer4 = dispatcher_->createTimer([&] { watcher4.ready(); }); @@ -1154,29 +1132,17 @@ TEST_P(TimerImplTest, TimerOrderDisableAndReschedule) { // timer1 is expected to run first and reschedule timers 2 and 3. timer4 should fire before // timer2 and timer3 since timer4's registration is unaffected. InSequence s; - if (activateTimersNextEventLoop()) { - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher1, ready()); - EXPECT_CALL(watcher4, ready()); - // Sleep during prepare to ensure that enough time has elapsed before timer evaluation to ensure - // that timers 2 and 3 are picked up by the same loop iteration. Without the sleep the two - // timers could execute in different loop iterations. - EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { - advanceLibeventTimeNextIteration(absl::Milliseconds(10)); - })); - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher3, ready()); - } else { - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher1, ready()); - EXPECT_CALL(watcher4, ready()); - EXPECT_CALL(watcher2, ready()); - // Sleep in prepare cb to avoid flakiness if epoll_wait returns before the timer timeout. - EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { - advanceLibeventTimeNextIteration(absl::Milliseconds(10)); - })); - EXPECT_CALL(watcher3, ready()); - } + EXPECT_CALL(prepare_watcher_, ready()); + EXPECT_CALL(watcher1, ready()); + EXPECT_CALL(watcher4, ready()); + // Sleep during prepare to ensure that enough time has elapsed before timer evaluation to ensure + // that timers 2 and 3 are picked up by the same loop iteration. Without the sleep the two + // timers could execute in different loop iterations. + EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { + advanceLibeventTimeNextIteration(absl::Milliseconds(10)); + })); + EXPECT_CALL(watcher2, ready()); + EXPECT_CALL(watcher3, ready()); runInEventLoop([&]() { timer1->enableTimer(std::chrono::milliseconds(0)); timer2->enableTimer(std::chrono::milliseconds(1)); @@ -1195,7 +1161,7 @@ TEST_P(TimerImplTest, TimerOrderDisableAndReschedule) { // Change the registration time for a timer that is already activated by re-enabling the timer // without calling disableTimer first. -TEST_P(TimerImplTest, TimerOrderAndReschedule) { +TEST_F(TimerImplTest, TimerOrderAndReschedule) { ReadyWatcher watcher4; Event::TimerPtr timer4 = dispatcher_->createTimer([&] { watcher4.ready(); }); @@ -1218,25 +1184,15 @@ TEST_P(TimerImplTest, TimerOrderAndReschedule) { InSequence s; EXPECT_CALL(prepare_watcher_, ready()); EXPECT_CALL(watcher1, ready()); - if (activateTimersNextEventLoop()) { - EXPECT_CALL(watcher4, ready()); - // Sleep during prepare to ensure that enough time has elapsed before timer evaluation to ensure - // that timers 2 and 3 are picked up by the same loop iteration. Without the sleep the two - // timers could execute in different loop iterations. - EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { - advanceLibeventTimeNextIteration(absl::Milliseconds(10)); - })); - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher3, ready()); - } else { - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher4, ready()); - // Sleep in prepare cb to avoid flakiness if epoll_wait returns before the timer timeout. - EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { - advanceLibeventTimeNextIteration(absl::Milliseconds(10)); - })); - EXPECT_CALL(watcher3, ready()); - } + EXPECT_CALL(watcher4, ready()); + // Sleep during prepare to ensure that enough time has elapsed before timer evaluation to ensure + // that timers 2 and 3 are picked up by the same loop iteration. Without the sleep the two + // timers could execute in different loop iterations. + EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { + advanceLibeventTimeNextIteration(absl::Milliseconds(10)); + })); + EXPECT_CALL(watcher2, ready()); + EXPECT_CALL(watcher3, ready()); runInEventLoop([&]() { timer1->enableTimer(std::chrono::milliseconds(0)); timer2->enableTimer(std::chrono::milliseconds(1)); @@ -1253,7 +1209,7 @@ TEST_P(TimerImplTest, TimerOrderAndReschedule) { }); } -TEST_P(TimerImplTest, TimerChaining) { +TEST_F(TimerImplTest, TimerChaining) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -1284,17 +1240,11 @@ TEST_P(TimerImplTest, TimerChaining) { InSequence s; EXPECT_CALL(prepare_watcher_, ready()); EXPECT_CALL(watcher4, ready()); - if (activateTimersNextEventLoop()) { - EXPECT_CALL(prepare_watcher_, ready()); - } + EXPECT_CALL(prepare_watcher_, ready()); EXPECT_CALL(watcher3, ready()); - if (activateTimersNextEventLoop()) { - EXPECT_CALL(prepare_watcher_, ready()); - } + EXPECT_CALL(prepare_watcher_, ready()); EXPECT_CALL(watcher2, ready()); - if (activateTimersNextEventLoop()) { - EXPECT_CALL(prepare_watcher_, ready()); - } + EXPECT_CALL(prepare_watcher_, ready()); EXPECT_CALL(watcher1, ready()); dispatcher_->run(Dispatcher::RunType::NonBlock); @@ -1304,7 +1254,7 @@ TEST_P(TimerImplTest, TimerChaining) { EXPECT_FALSE(timer4->enabled()); } -TEST_P(TimerImplTest, TimerChainDisable) { +TEST_F(TimerImplTest, TimerChainDisable) { ReadyWatcher watcher; Event::TimerPtr timer1; Event::TimerPtr timer2; @@ -1335,7 +1285,7 @@ TEST_P(TimerImplTest, TimerChainDisable) { dispatcher_->run(Dispatcher::RunType::NonBlock); } -TEST_P(TimerImplTest, TimerChainDelete) { +TEST_F(TimerImplTest, TimerChainDelete) { ReadyWatcher watcher; Event::TimerPtr timer1; Event::TimerPtr timer2; diff --git a/test/test_common/simulated_time_system.cc b/test/test_common/simulated_time_system.cc index af27468a6a797..b01f13f6e0b35 100644 --- a/test/test_common/simulated_time_system.cc +++ b/test/test_common/simulated_time_system.cc @@ -107,12 +107,7 @@ class SimulatedTimeSystemHelper::SimulatedScheduler : public Scheduler { } if (!run_alarms_cb_->enabled()) { - if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.activate_timers_next_event_loop")) { - run_alarms_cb_->scheduleCallbackNextIteration(); - } else { - run_alarms_cb_->scheduleCallbackCurrentIteration(); - } + run_alarms_cb_->scheduleCallbackNextIteration(); } } @@ -213,14 +208,12 @@ class SimulatedTimeSystemHelper::SimulatedScheduler : public Scheduler { // True if the SimulatedTimeSystemHelper is waiting for the scheduler to process expired alarms // and call decPending after an update to monotonic time. bool pending_dec_ ABSL_GUARDED_BY(mutex_) = false; - // Used to randomize the ordering of alarms scheduled for the same time when the runtime feature - // envoy.reloadable_features.activate_timers_next_event_loop is enabled. This mimics the trigger + // Used to randomize the ordering of alarms scheduled for the same time. This mimics the trigger // order of real timers scheduled for the same absolute time is non-deterministic. // Each simulated scheduler has it's own TestRandomGenerator with the same seed to improve test // failure reproducibility when running against a specific seed by minimizing cross scheduler // interactions. TestRandomGenerator random_source_ ABSL_GUARDED_BY(mutex_); - uint64_t legacy_next_idx_ ABSL_GUARDED_BY(mutex_) = 0; }; // Our simulated alarm inherits from TimerImpl so that the same dispatching @@ -269,24 +262,14 @@ void SimulatedTimeSystemHelper::SimulatedScheduler::enableAlarm( absl::MutexLock lock(&mutex_); if (duration.count() == 0 && triggered_alarms_.contains(alarm)) { return; - } else if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.activate_timers_next_event_loop")) { - disableAlarmLockHeld(alarm); - registered_alarms_.add({monotonic_time_ + duration, random_source_.random(), alarm}); } else { disableAlarmLockHeld(alarm); - AlarmSet& alarm_set = (duration.count() != 0) ? registered_alarms_ : triggered_alarms_; - alarm_set.add({monotonic_time_ + duration, ++legacy_next_idx_, alarm}); + registered_alarms_.add({monotonic_time_ + duration, random_source_.random(), alarm}); } } if (duration.count() == 0) { - if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.activate_timers_next_event_loop")) { - run_alarms_cb_->scheduleCallbackNextIteration(); - } else { - run_alarms_cb_->scheduleCallbackCurrentIteration(); - } + run_alarms_cb_->scheduleCallbackNextIteration(); } } diff --git a/test/test_common/simulated_time_system_test.cc b/test/test_common/simulated_time_system_test.cc index e46284925deb8..f518a1a249853 100644 --- a/test/test_common/simulated_time_system_test.cc +++ b/test/test_common/simulated_time_system_test.cc @@ -17,20 +17,12 @@ namespace Event { namespace Test { namespace { -enum class ActivateMode { DelayActivateTimers, EagerlyActivateTimers }; - -class SimulatedTimeSystemTest : public testing::TestWithParam { +class SimulatedTimeSystemTest : public testing::Test { protected: SimulatedTimeSystemTest() : scheduler_(time_system_.createScheduler(base_scheduler_, base_scheduler_)), start_monotonic_time_(time_system_.monotonicTime()), - start_system_time_(time_system_.systemTime()) { - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.activate_timers_next_event_loop", - activateMode() == ActivateMode::DelayActivateTimers ? "true" : "false"}}); - } - - ActivateMode activateMode() { return GetParam(); } + start_system_time_(time_system_.systemTime()) {} void trackPrepareCalls() { base_scheduler_.registerOnPrepareCallback([this]() { output_.append(1, 'p'); }); @@ -78,11 +70,7 @@ class SimulatedTimeSystemTest : public testing::TestWithParam { SystemTime start_system_time_; }; -INSTANTIATE_TEST_SUITE_P(DelayTimerActivation, SimulatedTimeSystemTest, - testing::Values(ActivateMode::DelayActivateTimers, - ActivateMode::EagerlyActivateTimers)); - -TEST_P(SimulatedTimeSystemTest, AdvanceTimeAsync) { +TEST_F(SimulatedTimeSystemTest, AdvanceTimeAsync) { EXPECT_EQ(start_monotonic_time_, time_system_.monotonicTime()); EXPECT_EQ(start_system_time_, time_system_.systemTime()); advanceMsAndLoop(5); @@ -90,7 +78,7 @@ TEST_P(SimulatedTimeSystemTest, AdvanceTimeAsync) { EXPECT_EQ(start_system_time_ + std::chrono::milliseconds(5), time_system_.systemTime()); } -TEST_P(SimulatedTimeSystemTest, TimerTotalOrdering) { +TEST_F(SimulatedTimeSystemTest, TimerTotalOrdering) { trackPrepareCalls(); addTask(0, '0'); @@ -104,7 +92,7 @@ TEST_P(SimulatedTimeSystemTest, TimerTotalOrdering) { EXPECT_EQ("p012", output_); } -TEST_P(SimulatedTimeSystemTest, TimerPartialOrdering) { +TEST_F(SimulatedTimeSystemTest, TimerPartialOrdering) { trackPrepareCalls(); std::set outputs; @@ -124,16 +112,12 @@ TEST_P(SimulatedTimeSystemTest, TimerPartialOrdering) { timers_.clear(); } - if (activateMode() == ActivateMode::DelayActivateTimers) { - // Execution order of timers 1 and 2 is non-deterministic because the two timers were scheduled - // for the same time. Verify that both orderings were observed. - EXPECT_THAT(outputs, testing::ElementsAre("p0123", "p0213")); - } else { - EXPECT_THAT(outputs, testing::ElementsAre("p0123")); - } + // Execution order of timers 1 and 2 is non-deterministic because the two timers were scheduled + // for the same time. Verify that both orderings were observed. + EXPECT_THAT(outputs, testing::ElementsAre("p0123", "p0213")); } -TEST_P(SimulatedTimeSystemTest, TimerPartialOrdering2) { +TEST_F(SimulatedTimeSystemTest, TimerPartialOrdering2) { trackPrepareCalls(); std::set outputs; @@ -154,17 +138,13 @@ TEST_P(SimulatedTimeSystemTest, TimerPartialOrdering2) { timers_.clear(); } - if (activateMode() == ActivateMode::DelayActivateTimers) { - // Execution order of timers 1 and 2 is non-deterministic because the two timers were scheduled - // for the same time. Verify that both orderings were observed. - EXPECT_THAT(outputs, testing::ElementsAre("p0p123", "p0p213")); - } else { - EXPECT_THAT(outputs, testing::ElementsAre("p0p123")); - } + // Execution order of timers 1 and 2 is non-deterministic because the two timers were scheduled + // for the same time. Verify that both orderings were observed. + EXPECT_THAT(outputs, testing::ElementsAre("p0p123", "p0p213")); } // Timers that are scheduled to execute and but are disabled first do not trigger. -TEST_P(SimulatedTimeSystemTest, TimerOrderAndDisableTimer) { +TEST_F(SimulatedTimeSystemTest, TimerOrderAndDisableTimer) { trackPrepareCalls(); // Create 3 timers. The first timer should disable the second, so it doesn't trigger. @@ -181,7 +161,7 @@ TEST_P(SimulatedTimeSystemTest, TimerOrderAndDisableTimer) { } // Capture behavior of timers which are rescheduled without being disabled first. -TEST_P(SimulatedTimeSystemTest, TimerOrderAndRescheduleTimer) { +TEST_F(SimulatedTimeSystemTest, TimerOrderAndRescheduleTimer) { trackPrepareCalls(); // Reschedule timers 1, 2 and 4 without disabling first. @@ -201,34 +181,26 @@ TEST_P(SimulatedTimeSystemTest, TimerOrderAndRescheduleTimer) { // Timer 4 runs as part of the first wakeup since its new schedule time has a delta of 0. Timer 2 // is delayed since it is rescheduled with a non-zero delta. advanceMsAndLoop(5); - if (activateMode() == ActivateMode::DelayActivateTimers) { - if constexpr (Event::PlatformDefaultTriggerType == FileTriggerType::Level) { - // Force it to run again to pick up next iteration callbacks. - // The event loop runs for a single iteration in NonBlock mode on Windows as a hack to work - // around LEVEL trigger fd registrations constantly firing events and preventing the NonBlock - // event loop from ever reaching the no-fd event and no-expired timers termination condition. - // It is not possible to get consistent event loop behavior since the time system does not - // override the base scheduler's run behavior, and libevent does not provide a mode where it - // runs at most N iterations before breaking out of the loop for us to prefer over the single - // iteration mode used on Windows. - advanceMsAndLoop(0); - } - EXPECT_EQ("p013p4", output_); - } else { - EXPECT_EQ("p0134", output_); + if constexpr (Event::PlatformDefaultTriggerType == FileTriggerType::Level) { + // Force it to run again to pick up next iteration callbacks. + // The event loop runs for a single iteration in NonBlock mode on Windows as a hack to work + // around LEVEL trigger fd registrations constantly firing events and preventing the NonBlock + // event loop from ever reaching the no-fd event and no-expired timers termination condition. + // It is not possible to get consistent event loop behavior since the time system does not + // override the base scheduler's run behavior, and libevent does not provide a mode where it + // runs at most N iterations before breaking out of the loop for us to prefer over the single + // iteration mode used on Windows. + advanceMsAndLoop(0); } + EXPECT_EQ("p013p4", output_); advanceMsAndLoop(100); - if (activateMode() == ActivateMode::DelayActivateTimers) { - EXPECT_EQ("p013p4p2", output_); - } else { - EXPECT_EQ("p0134p2", output_); - } + EXPECT_EQ("p013p4p2", output_); } // Disable and re-enable timers that is already pending execution and verify that execution is // delayed. -TEST_P(SimulatedTimeSystemTest, TimerOrderDisableAndRescheduleTimer) { +TEST_F(SimulatedTimeSystemTest, TimerOrderDisableAndRescheduleTimer) { trackPrepareCalls(); // Disable and reschedule timers 1, 2 and 4 when timer 0 triggers. @@ -251,26 +223,18 @@ TEST_P(SimulatedTimeSystemTest, TimerOrderDisableAndRescheduleTimer) { // because it is scheduled with zero delay. Timer 2 executes in a later iteration because it is // re-enabled with a non-zero timeout. advanceMsAndLoop(5); - if (activateMode() == ActivateMode::DelayActivateTimers) { - if constexpr (Event::PlatformDefaultTriggerType == FileTriggerType::Level) { - // The event loop runs for a single iteration in NonBlock mode on Windows. Force it to run - // again to pick up next iteration callbacks. - advanceMsAndLoop(0); - } - EXPECT_THAT(output_, testing::AnyOf("p03p14", "p03p41")); - } else { - EXPECT_EQ("p0314", output_); + if constexpr (Event::PlatformDefaultTriggerType == FileTriggerType::Level) { + // The event loop runs for a single iteration in NonBlock mode on Windows. Force it to run + // again to pick up next iteration callbacks. + advanceMsAndLoop(0); } + EXPECT_THAT(output_, testing::AnyOf("p03p14", "p03p41")); advanceMsAndLoop(100); - if (activateMode() == ActivateMode::DelayActivateTimers) { - EXPECT_THAT(output_, testing::AnyOf("p03p14p2", "p03p41p2")); - } else { - EXPECT_EQ("p0314p2", output_); - } + EXPECT_THAT(output_, testing::AnyOf("p03p14p2", "p03p41p2")); } -TEST_P(SimulatedTimeSystemTest, AdvanceTimeWait) { +TEST_F(SimulatedTimeSystemTest, AdvanceTimeWait) { EXPECT_EQ(start_monotonic_time_, time_system_.monotonicTime()); EXPECT_EQ(start_system_time_, time_system_.systemTime()); @@ -292,7 +256,7 @@ TEST_P(SimulatedTimeSystemTest, AdvanceTimeWait) { EXPECT_EQ(start_system_time_ + std::chrono::milliseconds(5), time_system_.systemTime()); } -TEST_P(SimulatedTimeSystemTest, WaitFor) { +TEST_F(SimulatedTimeSystemTest, WaitFor) { EXPECT_EQ(start_monotonic_time_, time_system_.monotonicTime()); EXPECT_EQ(start_system_time_, time_system_.systemTime()); @@ -350,7 +314,7 @@ TEST_P(SimulatedTimeSystemTest, WaitFor) { EXPECT_EQ(MonotonicTime(std::chrono::seconds(60)), time_system_.monotonicTime()); } -TEST_P(SimulatedTimeSystemTest, Monotonic) { +TEST_F(SimulatedTimeSystemTest, Monotonic) { // Setting time forward works. time_system_.setMonotonicTime(start_monotonic_time_ + std::chrono::milliseconds(5)); EXPECT_EQ(start_monotonic_time_ + std::chrono::milliseconds(5), time_system_.monotonicTime()); @@ -360,7 +324,7 @@ TEST_P(SimulatedTimeSystemTest, Monotonic) { EXPECT_EQ(start_monotonic_time_ + std::chrono::milliseconds(5), time_system_.monotonicTime()); } -TEST_P(SimulatedTimeSystemTest, System) { +TEST_F(SimulatedTimeSystemTest, System) { // Setting time forward works. time_system_.setSystemTime(start_system_time_ + std::chrono::milliseconds(5)); EXPECT_EQ(start_system_time_ + std::chrono::milliseconds(5), time_system_.systemTime()); @@ -370,7 +334,7 @@ TEST_P(SimulatedTimeSystemTest, System) { EXPECT_EQ(start_system_time_ + std::chrono::milliseconds(3), time_system_.systemTime()); } -TEST_P(SimulatedTimeSystemTest, Ordering) { +TEST_F(SimulatedTimeSystemTest, Ordering) { addTask(5, '5'); addTask(3, '3'); addTask(6, '6'); @@ -381,7 +345,7 @@ TEST_P(SimulatedTimeSystemTest, Ordering) { EXPECT_EQ("356", output_); } -TEST_P(SimulatedTimeSystemTest, SystemTimeOrdering) { +TEST_F(SimulatedTimeSystemTest, SystemTimeOrdering) { addTask(5, '5'); addTask(3, '3'); addTask(6, '6'); @@ -395,7 +359,7 @@ TEST_P(SimulatedTimeSystemTest, SystemTimeOrdering) { EXPECT_EQ("356", output_); // callbacks don't get replayed. } -TEST_P(SimulatedTimeSystemTest, DisableTimer) { +TEST_F(SimulatedTimeSystemTest, DisableTimer) { addTask(5, '5'); addTask(3, '3'); addTask(6, '6'); @@ -407,7 +371,7 @@ TEST_P(SimulatedTimeSystemTest, DisableTimer) { EXPECT_EQ("36", output_); } -TEST_P(SimulatedTimeSystemTest, IgnoreRedundantDisable) { +TEST_F(SimulatedTimeSystemTest, IgnoreRedundantDisable) { addTask(5, '5'); timers_[0]->disableTimer(); timers_[0]->disableTimer(); @@ -415,7 +379,7 @@ TEST_P(SimulatedTimeSystemTest, IgnoreRedundantDisable) { EXPECT_EQ("", output_); } -TEST_P(SimulatedTimeSystemTest, OverrideEnable) { +TEST_F(SimulatedTimeSystemTest, OverrideEnable) { addTask(5, '5'); timers_[0]->enableTimer(std::chrono::milliseconds(6)); advanceMsAndLoop(5); @@ -424,7 +388,7 @@ TEST_P(SimulatedTimeSystemTest, OverrideEnable) { EXPECT_EQ("5", output_); } -TEST_P(SimulatedTimeSystemTest, DeleteTime) { +TEST_F(SimulatedTimeSystemTest, DeleteTime) { addTask(5, '5'); addTask(3, '3'); addTask(6, '6'); @@ -437,7 +401,7 @@ TEST_P(SimulatedTimeSystemTest, DeleteTime) { } // Regression test for issues documented in https://github.com/envoyproxy/envoy/pull/6956 -TEST_P(SimulatedTimeSystemTest, DuplicateTimer) { +TEST_F(SimulatedTimeSystemTest, DuplicateTimer) { // Set one alarm two times to test that pending does not get duplicated.. std::chrono::milliseconds delay(0); TimerPtr zero_timer = scheduler_->createTimer([this]() { output_.append(1, '2'); }, dispatcher_); @@ -448,7 +412,7 @@ TEST_P(SimulatedTimeSystemTest, DuplicateTimer) { } // Regression test for issues documented in https://github.com/envoyproxy/envoy/pull/6956 -TEST_P(SimulatedTimeSystemTest, DuplicateTimer2) { +TEST_F(SimulatedTimeSystemTest, DuplicateTimer2) { // Now set an alarm which requires 10s of progress and make sure advanceTimeWait and waitFor // works. absl::Mutex mutex; @@ -490,13 +454,13 @@ TEST_P(SimulatedTimeSystemTest, DuplicateTimer2) { thread->join(); } -TEST_P(SimulatedTimeSystemTest, Enabled) { +TEST_F(SimulatedTimeSystemTest, Enabled) { TimerPtr timer = scheduler_->createTimer({}, dispatcher_); timer->enableTimer(std::chrono::milliseconds(0)); EXPECT_TRUE(timer->enabled()); } -TEST_P(SimulatedTimeSystemTest, DeleteTimerFromThread) { +TEST_F(SimulatedTimeSystemTest, DeleteTimerFromThread) { TimerPtr timer = scheduler_->createTimer([]() {}, dispatcher_); timer->enableTimer(std::chrono::milliseconds(0)); auto thread = Thread::threadFactoryForTest().createThread([&timer]() { timer.reset(); }); @@ -504,7 +468,7 @@ TEST_P(SimulatedTimeSystemTest, DeleteTimerFromThread) { thread->join(); } -TEST_P(SimulatedTimeSystemTest, DeleteTimerFromThread2) { +TEST_F(SimulatedTimeSystemTest, DeleteTimerFromThread2) { TimerPtr timer = scheduler_->createTimer([]() {}, dispatcher_); timer->enableTimer(std::chrono::milliseconds(1)); auto thread = Thread::threadFactoryForTest().createThread([&timer]() { timer.reset(); });