From 46b4a0f3fa7faa174b378b562207067c59f54a6b Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Tue, 26 Mar 2019 15:24:23 +0100 Subject: [PATCH] Removes PostStop from RepeatingTaskHandle. It isn't used and can easily be replaced with posting a task, as shown in the changes to the unit test. Also removing the sequenced task checker that no longer adds any value. We now ensure that the task can only be stopped with a reference to the task queue it runs on. Bug: webrtc:10365 Change-Id: Ie8aef6f742c55db1fb686f20c2a28c606c721fa0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/129725 Commit-Queue: Sebastian Jansson Reviewed-by: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#27292} --- rtc_base/task_utils/repeating_task.cc | 34 +------------------ rtc_base/task_utils/repeating_task.h | 14 ++------ .../task_utils/repeating_task_unittest.cc | 22 +++++++++--- 3 files changed, 21 insertions(+), 49 deletions(-) diff --git a/rtc_base/task_utils/repeating_task.cc b/rtc_base/task_utils/repeating_task.cc index 0cc9a6279d..cdabf2fc85 100644 --- a/rtc_base/task_utils/repeating_task.cc +++ b/rtc_base/task_utils/repeating_task.cc @@ -54,38 +54,16 @@ void RepeatingTaskBase::Stop() { next_run_time_ = Timestamp::PlusInfinity(); } -void RepeatingTaskBase::PostStop() { - if (task_queue_->IsCurrent()) { - RTC_DLOG(LS_INFO) << "Using PostStop() from the task queue running the " - "repeated task. Consider calling Stop() instead."; - } - task_queue_->PostTask(ToQueuedTask([this] { - RTC_DCHECK_RUN_ON(task_queue_); - Stop(); - })); -} - } // namespace webrtc_repeating_task_impl -RepeatingTaskHandle::RepeatingTaskHandle() { - sequence_checker_.Detach(); -} -RepeatingTaskHandle::~RepeatingTaskHandle() { - sequence_checker_.Detach(); -} RepeatingTaskHandle::RepeatingTaskHandle(RepeatingTaskHandle&& other) : repeating_task_(other.repeating_task_) { - RTC_DCHECK_RUN_ON(&sequence_checker_); other.repeating_task_ = nullptr; } RepeatingTaskHandle& RepeatingTaskHandle::operator=( RepeatingTaskHandle&& other) { - RTC_DCHECK_RUN_ON(&other.sequence_checker_); - { - RTC_DCHECK_RUN_ON(&sequence_checker_); - repeating_task_ = other.repeating_task_; - } + repeating_task_ = other.repeating_task_; other.repeating_task_ = nullptr; return *this; } @@ -95,7 +73,6 @@ RepeatingTaskHandle::RepeatingTaskHandle( : repeating_task_(repeating_task) {} void RepeatingTaskHandle::Stop() { - RTC_DCHECK_RUN_ON(&sequence_checker_); if (repeating_task_) { RTC_DCHECK_RUN_ON(repeating_task_->task_queue_); repeating_task_->Stop(); @@ -103,16 +80,7 @@ void RepeatingTaskHandle::Stop() { } } -void RepeatingTaskHandle::PostStop() { - RTC_DCHECK_RUN_ON(&sequence_checker_); - if (repeating_task_) { - repeating_task_->PostStop(); - repeating_task_ = nullptr; - } -} - bool RepeatingTaskHandle::Running() const { - RTC_DCHECK_RUN_ON(&sequence_checker_); return repeating_task_ != nullptr; } diff --git a/rtc_base/task_utils/repeating_task.h b/rtc_base/task_utils/repeating_task.h index eaf2a4b684..21ec130143 100644 --- a/rtc_base/task_utils/repeating_task.h +++ b/rtc_base/task_utils/repeating_task.h @@ -38,7 +38,6 @@ class RepeatingTaskBase : public QueuedTask { bool Run() final; void Stop() RTC_RUN_ON(task_queue_); - void PostStop(); TaskQueueBase* const task_queue_; // This is always finite, except for the special case where it's PlusInfinity @@ -77,8 +76,8 @@ class RepeatingTaskImpl final : public RepeatingTaskBase { // not thread safe. class RepeatingTaskHandle { public: - RepeatingTaskHandle(); - ~RepeatingTaskHandle(); + RepeatingTaskHandle() = default; + ~RepeatingTaskHandle() = default; RepeatingTaskHandle(RepeatingTaskHandle&& other); RepeatingTaskHandle& operator=(RepeatingTaskHandle&& other); RepeatingTaskHandle(const RepeatingTaskHandle&) = delete; @@ -122,11 +121,6 @@ class RepeatingTaskHandle { // closure itself. void Stop(); - // Stops future invocations of the repeating task closure. The closure might - // still be running when PostStop() returns, but there will be no future - // invocation. - void PostStop(); - // Returns true if Start() or DelayedStart() was called most recently. Returns // false initially and if Stop() or PostStop() was called most recently. bool Running() const; @@ -134,10 +128,8 @@ class RepeatingTaskHandle { private: explicit RepeatingTaskHandle( webrtc_repeating_task_impl::RepeatingTaskBase* repeating_task); - rtc::SequencedTaskChecker sequence_checker_; // Owned by the task queue. - webrtc_repeating_task_impl::RepeatingTaskBase* repeating_task_ - RTC_GUARDED_BY(sequence_checker_) = nullptr; + webrtc_repeating_task_impl::RepeatingTaskBase* repeating_task_ = nullptr; }; } // namespace webrtc diff --git a/rtc_base/task_utils/repeating_task_unittest.cc b/rtc_base/task_utils/repeating_task_unittest.cc index 6f8e7faebc..74261dff24 100644 --- a/rtc_base/task_utils/repeating_task_unittest.cc +++ b/rtc_base/task_utils/repeating_task_unittest.cc @@ -60,6 +60,18 @@ class MoveOnlyClosure { private: MockClosure* mock_; }; + +// Helper closure class to stop repeating task on a task queue. This is +// equivalent to [handle{move(handle)}] { handle.Stop(); } in c++14. +class TaskHandleStopper { + public: + explicit TaskHandleStopper(RepeatingTaskHandle handle) + : handle_(std::move(handle)) {} + void operator()() { handle_.Stop(); } + + private: + RepeatingTaskHandle handle_; +}; } // namespace TEST(RepeatingTaskTest, TaskIsStoppedOnStop) { @@ -79,7 +91,7 @@ TEST(RepeatingTaskTest, TaskIsStoppedOnStop) { Sleep(kShortInterval * (kShortIntervalCount + kMargin)); EXPECT_EQ(counter.load(), kShortIntervalCount); - handle.PostStop(); + task_queue.PostTask(TaskHandleStopper(std::move(handle))); // Sleep long enough that the task would run at least once more if not // stopped. Sleep(kLongInterval * 2); @@ -132,7 +144,7 @@ TEST(RepeatingTaskTest, CancelDelayedTaskBeforeItRuns) { TaskQueueForTest task_queue("queue"); auto handle = RepeatingTaskHandle::DelayedStart( task_queue.Get(), TimeDelta::ms(100), MoveOnlyClosure(&mock)); - handle.PostStop(); + task_queue.PostTask(TaskHandleStopper(std::move(handle))); EXPECT_TRUE(done.Wait(kTimeout.ms())); } @@ -144,7 +156,7 @@ TEST(RepeatingTaskTest, CancelTaskAfterItRuns) { TaskQueueForTest task_queue("queue"); auto handle = RepeatingTaskHandle::Start(task_queue.Get(), MoveOnlyClosure(&mock)); - handle.PostStop(); + task_queue.PostTask(TaskHandleStopper(std::move(handle))); EXPECT_TRUE(done.Wait(kTimeout.ms())); } @@ -211,9 +223,9 @@ TEST(RepeatingTaskTest, Example) { RepeatingTaskHandle handle; object->StartPeriodicTask(&handle, task_queue.Get()); // Restart the task - handle.PostStop(); + task_queue.PostTask(TaskHandleStopper(std::move(handle))); object->StartPeriodicTask(&handle, task_queue.Get()); - handle.PostStop(); + task_queue.PostTask(TaskHandleStopper(std::move(handle))); struct Destructor { void operator()() { object.reset(); } std::unique_ptr object;