From 6666db72231985e661aa5061930df43efa4f9bfb Mon Sep 17 00:00:00 2001 From: Craig Radcliffe Date: Fri, 22 Jan 2021 12:55:55 -0500 Subject: [PATCH 1/4] Adding unit test that reproduces the issue with disabling while processing a callback Signed-off-by: Craig Radcliffe --- .../scaled_range_timer_manager_impl_test.cc | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/common/event/scaled_range_timer_manager_impl_test.cc b/test/common/event/scaled_range_timer_manager_impl_test.cc index 14d240ecc704b..a6a6dd31f4f91 100644 --- a/test/common/event/scaled_range_timer_manager_impl_test.cc +++ b/test/common/event/scaled_range_timer_manager_impl_test.cc @@ -157,6 +157,33 @@ TEST_F(ScaledRangeTimerManagerTest, DisableWhileScalingMax) { simTime().advanceTimeAndRun(std::chrono::seconds(100), dispatcher_, Dispatcher::RunType::Block); } +TEST_F(ScaledRangeTimerManagerTest, DisableOtherTimerInCallbackEmptiesQueue) { + ScaledRangeTimerManagerImpl manager(dispatcher_); + + MockFunction callback1; + auto timer1 = + manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback1.AsStdFunction()); + MockFunction callback2; + auto timer2 = + manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback2.AsStdFunction()); + + timer1->enableTimer(std::chrono::seconds(100)); + timer2->enableTimer(std::chrono::seconds(100)); + + simTime().advanceTimeAndRun(std::chrono::seconds(5), dispatcher_, Dispatcher::RunType::Block); + + EXPECT_TRUE(timer1->enabled()); + EXPECT_TRUE(timer2->enabled()); + + EXPECT_CALL(callback1, Call).WillOnce(Invoke([&]() { + timer2->disableTimer(); + timer2.reset(); + })); + + // Run the dispatcher to make sure nothing happens when it's not supposed to. + simTime().advanceTimeAndRun(std::chrono::seconds(100), dispatcher_, Dispatcher::RunType::Block); +} + TEST_F(ScaledRangeTimerManagerTest, DisableWithZeroMinTime) { ScaledRangeTimerManagerImpl manager(dispatcher_); From 1514eecaa2d6a3777b9d0f5c452f6c737af46b3a Mon Sep 17 00:00:00 2001 From: Craig Radcliffe Date: Fri, 22 Jan 2021 14:28:14 -0500 Subject: [PATCH 2/4] scaled range timer: guard against queue deletion during timer fire Add a `processing_timers_` flag to the timer queues that is set during `onQueueTimerFired` processing. This flag is checked when a timer is removed to ensure that the timer's queue isn't deleted while it is in a callback triggered by `onQueueTimerFired`. Signed-off-by: Craig Radcliffe --- source/common/event/scaled_range_timer_manager_impl.cc | 8 +++++++- source/common/event/scaled_range_timer_manager_impl.h | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/source/common/event/scaled_range_timer_manager_impl.cc b/source/common/event/scaled_range_timer_manager_impl.cc index baea9de390bfc..919116ae06d22 100644 --- a/source/common/event/scaled_range_timer_manager_impl.cc +++ b/source/common/event/scaled_range_timer_manager_impl.cc @@ -225,7 +225,11 @@ void ScaledRangeTimerManagerImpl::removeTimer(ScalingTimerHandle handle) { handle.queue_.range_timers_.erase(handle.iterator_); // Don't keep around empty queues if (handle.queue_.range_timers_.empty()) { - queues_.erase(handle.queue_); + // Skip erasing the queue if we're in the middle of processing timers for the queue. The + // queue will be erased in `onQueueTimerFired` after the queue entries have been processed. + if (!handle.queue_.processing_timers_) { + queues_.erase(handle.queue_); + } return; } @@ -255,12 +259,14 @@ void ScaledRangeTimerManagerImpl::onQueueTimerFired(Queue& queue) { // Pop and trigger timers until the one at the front isn't supposed to have expired yet (given the // current scale factor). + queue.processing_timers_ = true; while (!timers.empty() && computeTriggerTime(timers.front(), queue.duration_, scale_factor_) <= now) { auto item = std::move(queue.range_timers_.front()); queue.range_timers_.pop_front(); item.timer_.trigger(); } + queue.processing_timers_ = false; if (queue.range_timers_.empty()) { // Maintain the invariant that queues are never empty. diff --git a/source/common/event/scaled_range_timer_manager_impl.h b/source/common/event/scaled_range_timer_manager_impl.h index 4c36b50f5de0f..2eb19c2c69fe3 100644 --- a/source/common/event/scaled_range_timer_manager_impl.h +++ b/source/common/event/scaled_range_timer_manager_impl.h @@ -68,6 +68,10 @@ class ScaledRangeTimerManagerImpl : public ScaledRangeTimerManager { // 2) on expiration // 3) when the scale factor changes const TimerPtr timer_; + + // A flag indicating whether the queue is currently processing timers. Used to guard against + // queue deletion during timer processing. + bool processing_timers_{false}; }; /** From bf84a7b51bbe8a9496cc5c4bb0df14606f9545f0 Mon Sep 17 00:00:00 2001 From: Craig Radcliffe Date: Fri, 22 Jan 2021 15:56:41 -0500 Subject: [PATCH 3/4] Adjusting the test timers so they have a guaranteed firing order Signed-off-by: Craig Radcliffe --- test/common/event/scaled_range_timer_manager_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/event/scaled_range_timer_manager_impl_test.cc b/test/common/event/scaled_range_timer_manager_impl_test.cc index a6a6dd31f4f91..4665f06a3869d 100644 --- a/test/common/event/scaled_range_timer_manager_impl_test.cc +++ b/test/common/event/scaled_range_timer_manager_impl_test.cc @@ -162,12 +162,12 @@ TEST_F(ScaledRangeTimerManagerTest, DisableOtherTimerInCallbackEmptiesQueue) { MockFunction callback1; auto timer1 = - manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback1.AsStdFunction()); + manager.createTimer(AbsoluteMinimum(std::chrono::seconds(0)), callback1.AsStdFunction()); MockFunction callback2; auto timer2 = manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback2.AsStdFunction()); - timer1->enableTimer(std::chrono::seconds(100)); + timer1->enableTimer(std::chrono::seconds(95)); timer2->enableTimer(std::chrono::seconds(100)); simTime().advanceTimeAndRun(std::chrono::seconds(5), dispatcher_, Dispatcher::RunType::Block); From 023d9ec654eadd34acc3e70d5e468bc28ccab93b Mon Sep 17 00:00:00 2001 From: Craig Radcliffe Date: Fri, 22 Jan 2021 17:15:11 -0500 Subject: [PATCH 4/4] Adding test for cancellation of timer in callback for timer in different queue Signed-off-by: Craig Radcliffe --- .../scaled_range_timer_manager_impl_test.cc | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/test/common/event/scaled_range_timer_manager_impl_test.cc b/test/common/event/scaled_range_timer_manager_impl_test.cc index 4665f06a3869d..90ad37070a86f 100644 --- a/test/common/event/scaled_range_timer_manager_impl_test.cc +++ b/test/common/event/scaled_range_timer_manager_impl_test.cc @@ -157,7 +157,7 @@ TEST_F(ScaledRangeTimerManagerTest, DisableWhileScalingMax) { simTime().advanceTimeAndRun(std::chrono::seconds(100), dispatcher_, Dispatcher::RunType::Block); } -TEST_F(ScaledRangeTimerManagerTest, DisableOtherTimerInCallbackEmptiesQueue) { +TEST_F(ScaledRangeTimerManagerTest, InCallbackDisableLastTimerInSameQueue) { ScaledRangeTimerManagerImpl manager(dispatcher_); MockFunction callback1; @@ -184,6 +184,33 @@ TEST_F(ScaledRangeTimerManagerTest, DisableOtherTimerInCallbackEmptiesQueue) { simTime().advanceTimeAndRun(std::chrono::seconds(100), dispatcher_, Dispatcher::RunType::Block); } +TEST_F(ScaledRangeTimerManagerTest, InCallbackDisableTimerInOtherQueue) { + ScaledRangeTimerManagerImpl manager(dispatcher_); + + MockFunction callback1; + auto timer1 = + manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback1.AsStdFunction()); + MockFunction callback2; + auto timer2 = + manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback2.AsStdFunction()); + + timer1->enableTimer(std::chrono::seconds(95)); + timer2->enableTimer(std::chrono::seconds(100)); + + simTime().advanceTimeAndRun(std::chrono::seconds(5), dispatcher_, Dispatcher::RunType::Block); + + EXPECT_TRUE(timer1->enabled()); + EXPECT_TRUE(timer2->enabled()); + + EXPECT_CALL(callback1, Call).WillOnce(Invoke([&]() { + timer2->disableTimer(); + timer2.reset(); + })); + + // Run the dispatcher to make sure nothing happens when it's not supposed to. + simTime().advanceTimeAndRun(std::chrono::seconds(100), dispatcher_, Dispatcher::RunType::Block); +} + TEST_F(ScaledRangeTimerManagerTest, DisableWithZeroMinTime) { ScaledRangeTimerManagerImpl manager(dispatcher_);