From 979d6e5bc4084aae21935171a503e0f493497f6f Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Tue, 25 Mar 2025 12:44:05 -0700 Subject: [PATCH] Fix timer cannot be restarted after firing. PiperOrigin-RevId: 740450323 --- internal/platform/timer.h | 2 +- internal/platform/timer_impl.cc | 15 +++++------ internal/platform/timer_impl.h | 4 +-- internal/platform/timer_impl_test.cc | 38 +++++++++++++++++++++++++--- internal/test/fake_timer.cc | 9 +++---- internal/test/fake_timer.h | 4 +-- internal/test/fake_timer_test.cc | 14 +++++----- 7 files changed, 57 insertions(+), 29 deletions(-) diff --git a/internal/platform/timer.h b/internal/platform/timer.h index 9aac95a4..87295f8c 100644 --- a/internal/platform/timer.h +++ b/internal/platform/timer.h @@ -35,7 +35,7 @@ class Timer { // @return Returns true if succeed, otherwise false is returned. virtual bool Start(int delay, int period, absl::AnyInvocable callback) = 0; - virtual bool Stop() = 0; + virtual void Stop() = 0; virtual bool IsRunning() = 0; virtual bool FireNow() = 0; }; diff --git a/internal/platform/timer_impl.cc b/internal/platform/timer_impl.cc index 604e9270..8cc4ddb7 100644 --- a/internal/platform/timer_impl.cc +++ b/internal/platform/timer_impl.cc @@ -16,7 +16,8 @@ #include -#include "absl/time/clock.h" +#include "absl/functional/any_invocable.h" +#include "internal/platform/implementation/platform.h" #include "internal/platform/logging.h" namespace nearby { @@ -39,15 +40,13 @@ bool TimerImpl::Start(int delay, int period, return true; } -bool TimerImpl::Stop() { +void TimerImpl::Stop() { if (internal_timer_ == nullptr) { - return true; + return; } - if (internal_timer_->Stop()) { - internal_timer_ = nullptr; - return true; - } - return false; + // Stop returns false if timer has already fired. We can ignore that. + internal_timer_->Stop(); + internal_timer_ = nullptr; } bool TimerImpl::IsRunning() { return (internal_timer_ != nullptr); } diff --git a/internal/platform/timer_impl.h b/internal/platform/timer_impl.h index c1608de1..8e1cf865 100644 --- a/internal/platform/timer_impl.h +++ b/internal/platform/timer_impl.h @@ -18,7 +18,7 @@ #include #include "absl/functional/any_invocable.h" -#include "internal/platform/implementation/platform.h" +#include "internal/platform/implementation/timer.h" #include "internal/platform/timer.h" namespace nearby { @@ -28,7 +28,7 @@ class TimerImpl : public Timer { bool Start(int delay, int period, absl::AnyInvocable callback) override; - bool Stop() override; + void Stop() override; bool IsRunning() override; bool FireNow() override; diff --git a/internal/platform/timer_impl_test.cc b/internal/platform/timer_impl_test.cc index a79c8a4b..1d3e6e1a 100644 --- a/internal/platform/timer_impl_test.cc +++ b/internal/platform/timer_impl_test.cc @@ -15,6 +15,8 @@ #include "internal/platform/timer_impl.h" #include "gtest/gtest.h" +#include "absl/synchronization/notification.h" +#include "absl/time/time.h" namespace nearby { namespace { @@ -24,7 +26,7 @@ TEST(TimerImpl, TestCreateTimer) { EXPECT_FALSE(timer.Start(-100, 0, nullptr)); EXPECT_TRUE(timer.Start(100, 100, []() {})); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); } TEST(TimerImpl, TestRunningStatus) { @@ -32,7 +34,7 @@ TEST(TimerImpl, TestRunningStatus) { EXPECT_TRUE(timer.Start(100, 100, []() {})); EXPECT_TRUE(timer.IsRunning()); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); EXPECT_FALSE(timer.IsRunning()); } @@ -41,7 +43,7 @@ TEST(TimerImpl, TestStartRunningTimer) { EXPECT_TRUE(timer.Start(100, 100, []() {})); EXPECT_FALSE(timer.Start(100, 100, []() {})); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); } TEST(TimerImpl, TestFireNow) { @@ -50,9 +52,37 @@ TEST(TimerImpl, TestFireNow) { EXPECT_TRUE(timer.Start(100, 100, [&count]() { ++count; })); EXPECT_TRUE(timer.FireNow()); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); EXPECT_EQ(count, 1); } +TEST(TimerImpl, TestStopAfterFire) { + TimerImpl timer; + absl::Notification notification; + EXPECT_TRUE(timer.Start(0, 0, [¬ification]() { + notification.Notify(); + })); + notification.WaitForNotificationWithTimeout(absl::Seconds(1)); + + timer.Stop(); +} + +TEST(TimerImpl, TestRestartAfterFire) { + TimerImpl timer; + absl::Notification notification; + EXPECT_TRUE(timer.Start(0, 0, [¬ification]() { + notification.Notify(); + })); + notification.WaitForNotificationWithTimeout(absl::Seconds(1)); + timer.Stop(); + absl::Notification notification2; + + EXPECT_TRUE(timer.Start(100, 100, [¬ification2]() { + notification2.Notify(); + })); + notification2.WaitForNotificationWithTimeout(absl::Seconds(1)); + timer.Stop(); +} + } // namespace } // namespace nearby diff --git a/internal/test/fake_timer.cc b/internal/test/fake_timer.cc index 08d47e8a..c11bfab2 100644 --- a/internal/test/fake_timer.cc +++ b/internal/test/fake_timer.cc @@ -36,9 +36,9 @@ bool FakeTimer::Start(int delay, int period, return InternalStart(delay, period, std::move(callback)); } -bool FakeTimer::Stop() { +void FakeTimer::Stop() { MutexLock lock(&mutex_); - return InternalStop(); + InternalStop(); } bool FakeTimer::IsRunning() { @@ -115,13 +115,12 @@ bool FakeTimer::InternalStart(int delay, int period, return true; } -bool FakeTimer::InternalStop() { +void FakeTimer::InternalStop() { if (timer_data_.id.empty()) { - return true; + return; } clock_->RemoveObserver(timer_data_.id); timer_data_ = {}; - return true; } } // namespace nearby diff --git a/internal/test/fake_timer.h b/internal/test/fake_timer.h index 6f4e2e20..003b33d7 100644 --- a/internal/test/fake_timer.h +++ b/internal/test/fake_timer.h @@ -31,7 +31,7 @@ class FakeTimer : public Timer { bool Start(int delay, int period, absl::AnyInvocable callback) override; - bool Stop() override; + void Stop() override; bool IsRunning() override; bool FireNow() override; @@ -49,7 +49,7 @@ class FakeTimer : public Timer { void ClockUpdated(); bool InternalStart(int delay, int period, absl::AnyInvocable callback); - bool InternalStop(); + void InternalStop(); mutable RecursiveMutex mutex_; FakeClock* clock_ = nullptr; diff --git a/internal/test/fake_timer_test.cc b/internal/test/fake_timer_test.cc index 89a185b9..f90cb042 100644 --- a/internal/test/fake_timer_test.cc +++ b/internal/test/fake_timer_test.cc @@ -32,7 +32,7 @@ TEST(FakeTimer, TestOneTimeTimer) { EXPECT_TRUE(timer.IsRunning()); clock.FastForward(absl::Milliseconds(1000)); EXPECT_EQ(count, 1); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); EXPECT_FALSE(timer.IsRunning()); } @@ -45,7 +45,7 @@ TEST(FakeTimer, TestRepeatTimer) { EXPECT_TRUE(timer.IsRunning()); clock.FastForward(absl::Milliseconds(1000)); EXPECT_EQ(count, 10); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); EXPECT_FALSE(timer.IsRunning()); } @@ -56,7 +56,7 @@ TEST(FakeTimer, TestInvalidInput) { auto callback = [&count]() { ++count; }; timer.Start(-100, 100, callback); EXPECT_FALSE(timer.IsRunning()); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); } TEST(FakeTimer, TestStopTimerBeforeClockUpdate) { @@ -66,7 +66,7 @@ TEST(FakeTimer, TestStopTimerBeforeClockUpdate) { auto callback = [&count]() { ++count; }; timer.Start(100, 100, callback); EXPECT_TRUE(timer.IsRunning()); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); EXPECT_FALSE(timer.IsRunning()); clock.FastForward(absl::Milliseconds(1000)); EXPECT_EQ(count, 0); @@ -85,7 +85,7 @@ TEST(FakeTimer, TestUpdateMultipleTimesClockForOnetimeTimer) { EXPECT_EQ(count, 1); clock.FastForward(absl::Milliseconds(1000)); EXPECT_EQ(count, 1); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); EXPECT_FALSE(timer.IsRunning()); } @@ -98,7 +98,7 @@ TEST(FakeTimer, TestInstantRunTimer) { EXPECT_TRUE(timer.IsRunning()); clock.FastForward(absl::Milliseconds(1000)); EXPECT_EQ(count, 11); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); EXPECT_FALSE(timer.IsRunning()); } @@ -112,7 +112,7 @@ TEST(FakeTimer, TestTimerDestructor) { EXPECT_EQ(clock.GetObserversCount(), 1); EXPECT_TRUE(timer.IsRunning()); EXPECT_EQ(count, 0); - EXPECT_TRUE(timer.Stop()); + timer.Stop(); EXPECT_FALSE(timer.IsRunning()); } EXPECT_EQ(clock.GetObserversCount(), 0);