Skip to content

scaled range timer: guard against queue deletion during timer fire #14799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggreenway merged 4 commits into from
Jan 27, 2021
+65 −1
Merged

scaled range timer: guard against queue deletion during timer fire #14799

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6666db7
Adding unit test that reproduces the issue with disabling while proce…
Jan 22, 2021
1514eec
scaled range timer: guard against queue deletion during timer fire
Jan 22, 2021
bf84a7b
Adjusting the test timers so they have a guaranteed firing order
Jan 22, 2021
023d9ec
Adding test for cancellation of timer in callback for timer in differ…
Jan 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion source/common/event/scaled_range_timer_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;

@antoniovicente antoniovicente Jan 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So processing_timers_ needs to be per queue since otherwise the queue not empty invariant would not hold if a timer from queue A removes a timer from queue B. It would be good to also cover that case in tests.

Also, is there code somewhere that verifies the invariant that all active queues are non-empty?

@akonradi akonradi Jan 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is there code somewhere that verifies the invariant that all active queues are non-empty?

I don't believe there is currently. The number of queues and sizes is not currently exposed. I'm okay to keep this PR focused on the bugfix and add additional testing in a follow-on.

@chradcliffe chradcliffe Jan 22, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

023d9ec adds a test for cancelling a timer from a different queue in the context of a timer callback.

@antoniovicente antoniovicente Jan 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @asraa

@akonradi Please do followup. Regarding ASSERTs, I think failing this one will result in a crash in the next line.

envoy/source/common/event/scaled_range_timer_manager_impl.cc

Line 244 in bf84a7b

ASSERT(!queue.range_timers_.empty());


if (queue.range_timers_.empty()) {
// Maintain the invariant that queues are never empty.
Expand Down
4 changes: 4 additions & 0 deletions source/common/event/scaled_range_timer_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
};

/**
Expand Down
27 changes: 27 additions & 0 deletions test/common/event/scaled_range_timer_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<TimerCb> callback1;
auto timer1 =
manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback1.AsStdFunction());
MockFunction<TimerCb> callback2;
auto timer2 =
manager.createTimer(AbsoluteMinimum(std::chrono::seconds(5)), callback2.AsStdFunction());

timer1->enableTimer(std::chrono::seconds(100));
timer2->enableTimer(std::chrono::seconds(100));

@akonradi akonradi Jan 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these timers are enabled for the same duration, they can fire in either order. Consider making timer1 fire in 95 seconds with an absolute minimum of 0.

@antoniovicente antoniovicente Jan 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need both timers to end up in the same queues so their durations should be the same.

I think the solution is to do an advance of 5 seconds between the first enable and the second enable.

@akonradi akonradi Jan 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either of those will work. The duration that matters is (max - min) which is (100 - 5) or (95 - 0) as adjusted.


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

Expand Down