dcsctp: Handle starting timer from timer callback
This was caught in an integration test which had stricter assertions
than the FakeTimeout which is used in unit tests, so the first thing was
to add the same assertions to the FakeTimeout.
The issue is that when a Timer triggers, and if it's set to
automatically restart (possibly with an exponential backoff), the
`is_running_` field was set to true while the timer callback was called
to allow the client to know that the timer is in fact running, but the
timer was actually not started until the callback returned. Which made
sense, as the callback can with its return value override the duration,
which should affect the backoff algorithm.
The problem was when a timer was manually started within the callback.
As the Timer itself thought that it was already running, it first would
Stop() the underlying Timeout, then Start(). But calling Stop() on a
timeout that is not running is illegal, which set of assertions.
So the solution is to don't lie; Don't say that a timer is running when
it's not. Make sure that the timer is running when the timer callback is
triggered, which makes it consistent at all times. That may result in
unnecessary timeout invocations (stopping and starting), but that's not
too expensive.
Bug: webrtc:12614
Change-Id: I7b4447ccd88bd43d181e158f0d29b0770c8a3fd6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217522
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33926}
diff --git a/net/dcsctp/timer/timer_test.cc b/net/dcsctp/timer/timer_test.cc
index 82b92ef..a403bb6 100644
--- a/net/dcsctp/timer/timer_test.cc
+++ b/net/dcsctp/timer/timer_test.cc
@@ -351,5 +351,40 @@
AdvanceTimeAndRunTimers(Timer::kMaxTimerDuration);
}
+TEST_F(TimerTest, TimerCanBeStartedFromWithinExpirationHandler) {
+ std::unique_ptr<Timer> t1 = manager_.CreateTimer(
+ "t1", on_expired_.AsStdFunction(),
+ TimerOptions(DurationMs(1000), TimerBackoffAlgorithm::kFixed));
+
+ t1->Start();
+
+ // Start a timer, but don't return any new duration in callback.
+ EXPECT_CALL(on_expired_, Call).WillOnce([&]() {
+ EXPECT_TRUE(t1->is_running());
+ t1->set_duration(DurationMs(5000));
+ t1->Start();
+ return absl::nullopt;
+ });
+ AdvanceTimeAndRunTimers(DurationMs(1000));
+
+ EXPECT_CALL(on_expired_, Call).Times(0);
+ AdvanceTimeAndRunTimers(DurationMs(4999));
+
+ // Start a timer, and return any new duration in callback.
+ EXPECT_CALL(on_expired_, Call).WillOnce([&]() {
+ EXPECT_TRUE(t1->is_running());
+ t1->set_duration(DurationMs(5000));
+ t1->Start();
+ return absl::make_optional(DurationMs(8000));
+ });
+ AdvanceTimeAndRunTimers(DurationMs(1));
+
+ EXPECT_CALL(on_expired_, Call).Times(0);
+ AdvanceTimeAndRunTimers(DurationMs(7999));
+
+ EXPECT_CALL(on_expired_, Call).Times(1);
+ AdvanceTimeAndRunTimers(DurationMs(1));
+}
+
} // namespace
} // namespace dcsctp