Index: base/timer/timer.cc |
diff --git a/base/timer/timer.cc b/base/timer/timer.cc |
index 6ec18f181488c3a5b533abe93cf9b793914e6451..6895f9e2eda9feef7a709b25db1d06830d4493d0 100644 |
--- a/base/timer/timer.cc |
+++ b/base/timer/timer.cc |
@@ -11,68 +11,205 @@ |
#include "base/logging.h" |
#include "base/memory/ptr_util.h" |
#include "base/memory/ref_counted.h" |
-#include "base/single_thread_task_runner.h" |
-#include "base/threading/platform_thread.h" |
-#include "base/threading/thread_task_runner_handle.h" |
+#include "base/sequenced_task_runner.h" |
+#include "base/threading/sequenced_task_runner_handle.h" |
#include "base/time/tick_clock.h" |
namespace base { |
-// BaseTimerTaskInternal is a simple delegate for scheduling a callback to |
-// Timer in the thread's default task runner. It also handles the following |
-// edge cases: |
-// - deleted by the task runner. |
-// - abandoned (orphaned) by Timer. |
-class BaseTimerTaskInternal { |
+// BaseTimerTaskInternal is a simple delegate for scheduling a callback to Timer |
vmpstr
2016/12/03 01:14:04
s/simple // :)
Can you also split up the comment
gab
2016/12/23 20:22:11
:)
|
+// in the destination task runner. It owns itself on the destination task |
+// runner. If non-repeating, it will self-destruct when it fires otherwise it |
+// will re-enqueue until abandoned. If non-repeating it will also invoke |
+// |stop_callback| on |origin_task_runner| *before* running |task|. The WeakPtr |
+// ensures that the task to trigger a self-destruction cancels all others. As |
+// such, cancellation is racing with the scheduled delayed Run() task but so |
vmpstr
2016/12/03 01:14:04
By race here, do you mean that either Abandon will
gab
2016/12/23 20:22:11
I mean that Abandon() is not a delayed task while
|
+// would any other cancellation method (e.g. using locks and sharing state with |
+// the origin task runner). An invariant assumed below is that there always is |
+// exactly one pending Run() task associated with this BaseTimerTaskInternal, as |
+// such only Run() needs to repost itself to maintain that invariant until self- |
+// destruction. |
+// |
+// Note: Timer and BaseTimerTaskInternal try to communicate synchronously when |
+// possible (i.e. when SetTaskRunner() wasn't used, which they can verify |
+// through RunsTasksOnCurrentThread()). Timer must also always use asynchronous |
+// calls when invoking Abandon() to avoid calling "delete this" from a reentrant |
+// call while handling Run(). |
+class BaseTimerTaskInternal : public SupportsWeakPtr<BaseTimerTaskInternal> { |
public: |
- explicit BaseTimerTaskInternal(Timer* timer) |
- : timer_(timer) { |
+ // A helper which invokes a Closure when destroyed. Required to notify |
+ // BaseTimerTaskInternal when its Run() task is suppressed before it's |
+ // scheduled (e.g. because the sequence is shutdown). Note: such cleanups |
+ // can't be bound directly to ~BaseTimerTaskInternal() as it owns itself (its |
+ // tasks use WeakPtrs and deleting them won't trigger |
+ // ~BaseTimerTaskInternal()). |
+ class CleanupTrigger { |
vmpstr
2016/12/03 01:14:04
Consider adding RunOnDestroy type of class to base
gab
2016/12/23 20:22:11
Whereelse? Tried to code search for it and didn't
|
+ public: |
+ ~CleanupTrigger() { std::move(on_destruction_).Run(); } |
+ |
+ private: |
+ friend class BaseTimerTaskInternal; |
+ CleanupTrigger(OnceClosure on_destruction) |
+ : on_destruction_(std::move(on_destruction)) {} |
+ |
+ OnceClosure on_destruction_; |
+ |
+ DISALLOW_COPY_AND_ASSIGN(CleanupTrigger); |
+ }; |
+ |
+ BaseTimerTaskInternal(const tracked_objects::Location& posted_from, |
+ const Closure& task, |
+ TimeDelta delay, |
+ bool is_repeating, |
+ TickClock* tick_clock, |
+ scoped_refptr<SequencedTaskRunner> origin_task_runner, |
+ const Closure& stop_callback) |
+ : posted_from_(posted_from), |
+ task_(task), |
+ delay_(delay), |
+ is_repeating_(is_repeating), |
+ tick_clock_(tick_clock), |
+ desired_run_time_(Now()), |
+ origin_task_runner_(std::move(origin_task_runner)), |
+ stop_callback_(stop_callback) { |
+ destination_sequence_checker_.DetachFromSequence(); |
vmpstr
2016/12/03 01:14:04
Can you comment that we need to detatch to ensure
gab
2016/12/23 20:22:11
Right, already have a comment to that effect on |d
|
} |
- ~BaseTimerTaskInternal() { |
- // This task may be getting cleared because the task runner has been |
- // destructed. If so, don't leave Timer with a dangling pointer |
- // to this. |
- if (timer_) |
- timer_->StopAndAbandon(); |
- } |
+ ~BaseTimerTaskInternal() = default; |
- void Run() { |
- // timer_ is NULL if we were abandoned. |
- if (!timer_) |
+ void Run(std::unique_ptr<CleanupTrigger> cleanup_trigger) { |
+ DCHECK(destination_sequence_checker_.CalledOnValidSequence()); |
+ |
+ const TimeDelta delay_remaining = desired_run_time_ - Now(); |
+ if (delay_remaining > TimeDelta::FromMicroseconds(0)) { |
+#if DCHECK_IS_ON() |
+ DCHECK(was_reset_) << delay_remaining; |
+#endif |
+ SequencedTaskRunnerHandle::Get()->PostDelayedTask( |
+ posted_from_, base::Bind(&BaseTimerTaskInternal::Run, AsWeakPtr(), |
+ Passed(std::move(cleanup_trigger))), |
+ delay_remaining); |
return; |
+ } |
- // *this will be deleted by the task runner, so Timer needs to |
- // forget us: |
- timer_->scheduled_task_ = NULL; |
+ // Trigger cleanup before running the |task_|. e.g. to Stop() OneShotTimers |
+ // so that anything posted from the task runs in the context of a stopped |
+ // timer as expected. |
+ if (!is_repeating_) |
+ cleanup_trigger.reset(); |
+ |
+ task_.Run(); |
+ |
+ if (is_repeating_) { |
+ // TODO(gab): Currently rebasing on Now() to match prior status quo but |
vmpstr
2016/12/03 01:14:04
Can you file a bug for this and reference it here?
gab
2016/12/23 20:22:10
Done.
|
+ // think it would be better to merely |+= delay_| here to avoid the timer |
+ // drifting with the delayed task overhead. |
+ desired_run_time_ = Now() + delay_; |
+ SequencedTaskRunnerHandle::Get()->PostDelayedTask( |
+ posted_from_, base::Bind(&BaseTimerTaskInternal::Run, AsWeakPtr(), |
+ Passed(std::move(cleanup_trigger))), |
+ delay_); |
vmpstr
2016/12/03 01:14:04
was_reset_ = false?
gab
2016/12/23 20:22:10
Done.
|
+ } else { |
+ delete this; |
+ } |
+ } |
- // Although Timer should not call back into *this, let's clear |
- // the timer_ member first to be pedantic. |
- Timer* timer = timer_; |
- timer_ = NULL; |
- timer->RunScheduledTask(); |
+ // Postpones the |desired_run_time_| by |delay_|. |
+ void Reset() { |
+ DCHECK(destination_sequence_checker_.CalledOnValidSequence()); |
+ DCHECK(!delay_.is_zero()); |
+ |
+ // Since this message is sent asynchronously, a few things might occur: |
+ // - This is a one-shot timer and Run() hasn't occurred: |
+ // postpone |desired_run_time_|, Run() will postpone itself when it |
+ // runs. |
+ // - This is a one-shot timer and Run() already occurred: |
+ // Reset() came in too late and will be canceled via the WeakPtr. |
+ // - This is a repeating timer and Run() has occurred zero to many times: |
+ // postpone the next |desired_run_time_|. |
+ // So overall, either the Reset() came in too late and will be canceled or |
vmpstr
2016/12/03 01:14:04
This comment is a bit confusing to me. So does thi
gab
2016/12/23 20:22:11
This whole comment is just meant to say that altho
|
+ // the |desired_run_time_| merely needs to be postponed. |
+ |
+ desired_run_time_ = Now() + delay_; |
+ |
+#if DCHECK_IS_ON() |
+ was_reset_ = true; |
+#endif |
} |
- // The task remains in the MessageLoop queue, but nothing will happen when it |
- // runs. |
void Abandon() { |
- timer_ = NULL; |
+ DCHECK(destination_sequence_checker_.CalledOnValidSequence()); |
+ |
+ // Delete self, invalidating WeakPtrs and as such, pending Run()/Reset() |
vmpstr
2016/12/03 01:14:04
Here as well, what's a reset task here?
gab
2016/12/23 20:22:11
Actually it can only cancel "the pending Run() tas
|
+ // tasks. |
+ delete this; |
+ } |
+ |
+ std::unique_ptr<CleanupTrigger> GetCleanupTrigger() { |
vmpstr
2016/12/03 01:14:04
nit: CreateCleanupTrigger
gab
2016/12/23 20:22:10
Done.
|
+ // Note: it's important that CleanupTrigger's Closure be bound to a WeakPtr |
+ // so that it only triggers if the Run() task is suppressed from its |
+ // sequence while it was live (i.e. it shouldn't trigger when the task is |
+ // destroyed because the WeakPtr was invalid when an abandoned Run() task |
+ // was scheduled). |
+ return WrapUnique( |
+ new CleanupTrigger(Bind(&BaseTimerTaskInternal::Cleanup, AsWeakPtr()))); |
} |
private: |
- Timer* timer_; |
+ TimeTicks Now() const { |
+ return tick_clock_ ? tick_clock_->NowTicks() : TimeTicks::Now(); |
+ } |
+ |
+ // Cleans up state on the Timer side when this BaseTimerTaskInternal is done |
+ // with its role. |
+ void Cleanup() { |
+ DCHECK(destination_sequence_checker_.CalledOnValidSequence()); |
+ |
+ if (origin_task_runner_->RunsTasksOnCurrentThread()) { |
+ stop_callback_.Run(); |
+ } else { |
+ origin_task_runner_->PostTask(FROM_HERE, stop_callback_); |
+ } |
+ } |
+ |
+ const tracked_objects::Location posted_from_; |
+ const Closure task_; |
+ const TimeDelta delay_; |
+ const bool is_repeating_; |
+ |
+ TickClock* tick_clock_; |
+ TimeTicks desired_run_time_; |
+ |
+ const scoped_refptr<SequencedTaskRunner> origin_task_runner_; |
+ const Closure stop_callback_; |
+ |
+ // Verifies that every operation (but construction) happens on the sequence |
+ // where the task is intended to run. |
+ SequenceChecker destination_sequence_checker_; |
+ |
+#if DCHECK_IS_ON() |
+ // Used to verify that reposts only occur when a Reset() was involved. |
+ bool was_reset_ = false; |
+#endif |
+ |
+ DISALLOW_COPY_AND_ASSIGN(BaseTimerTaskInternal); |
}; |
Timer::Timer(bool retain_user_task, bool is_repeating) |
: Timer(retain_user_task, is_repeating, nullptr) {} |
Timer::Timer(bool retain_user_task, bool is_repeating, TickClock* tick_clock) |
- : scheduled_task_(nullptr), |
- thread_id_(0), |
- is_repeating_(is_repeating), |
+ : is_repeating_(is_repeating), |
retain_user_task_(retain_user_task), |
tick_clock_(tick_clock), |
- is_running_(false) {} |
+ is_running_(false), |
+ weak_ptr_factory_(this) { |
+ // It is safe for the timer to be created on a different thread/sequence than |
+ // the one from which the timer APIs are called. The first call to the |
+ // checker's CalledOnValidSequence() method will re-bind the checker, and |
+ // later calls will verify that the same task runner is used. |
+ origin_sequence_checker_.DetachFromSequence(); |
+} |
Timer::Timer(const tracked_objects::Location& posted_from, |
TimeDelta delay, |
@@ -85,72 +222,86 @@ Timer::Timer(const tracked_objects::Location& posted_from, |
const base::Closure& user_task, |
bool is_repeating, |
TickClock* tick_clock) |
- : scheduled_task_(nullptr), |
- posted_from_(posted_from), |
+ : posted_from_(posted_from), |
delay_(delay), |
user_task_(user_task), |
- thread_id_(0), |
is_repeating_(is_repeating), |
retain_user_task_(true), |
tick_clock_(tick_clock), |
- is_running_(false) {} |
+ is_running_(false), |
+ weak_ptr_factory_(this) { |
+ // See comment in other constructor. |
+ origin_sequence_checker_.DetachFromSequence(); |
+} |
Timer::~Timer() { |
- StopAndAbandon(); |
+ if (origin_sequence_checker_.CalledOnValidSequence()) { |
vmpstr
2016/12/03 01:14:04
This is changing code behavior based on whether DC
gab
2016/12/23 20:22:10
No it doesn't because the member is a SequenceChec
|
+ Stop(); |
+ } else { |
+ // As highlighted in the constructor. It's okay to start the Timer on a |
+ // different sequence but it must then be sequentially stopped on that |
+ // sequence as well before it can be deleted on its original sequence. |
+ DCHECK(!is_running_); |
+ } |
} |
bool Timer::IsRunning() const { |
+ DCHECK(origin_sequence_checker_.CalledOnValidSequence()); |
return is_running_; |
} |
TimeDelta Timer::GetCurrentDelay() const { |
+ DCHECK(origin_sequence_checker_.CalledOnValidSequence()); |
return delay_; |
} |
-void Timer::SetTaskRunner(scoped_refptr<SingleThreadTaskRunner> task_runner) { |
- // Do not allow changing the task runner once something has been scheduled. |
- DCHECK_EQ(thread_id_, 0); |
- task_runner_.swap(task_runner); |
+void Timer::SetTaskRunner(scoped_refptr<SequencedTaskRunner> task_runner) { |
+ DCHECK(origin_sequence_checker_.CalledOnValidSequence()); |
+ |
+ // Do not allow changing the task runner while something is been scheduled. |
vmpstr
2016/12/03 01:14:04
s/been //
gab
2016/12/23 20:22:11
Done.
|
+ DCHECK(!is_running_); |
+ destination_task_runner_ = std::move(task_runner); |
} |
void Timer::Start(const tracked_objects::Location& posted_from, |
TimeDelta delay, |
const base::Closure& user_task) { |
+ DCHECK(origin_sequence_checker_.CalledOnValidSequence()); |
+ |
+ // Do a best-effort to cancel the previous task, it may still racily fire |
+ // though. |
+ AbandonScheduledTask(); |
+ |
SetTaskInfo(posted_from, delay, user_task); |
Reset(); |
vmpstr
2016/12/03 01:14:04
Is this call always just PostNewScheduledTask?
gab
2016/12/23 20:22:11
Actually yes it is, inlined that instead and added
|
} |
void Timer::Stop() { |
- is_running_ = false; |
+ DCHECK(origin_sequence_checker_.CalledOnValidSequence()); |
+ |
+ AbandonScheduledTask(); |
if (!retain_user_task_) |
user_task_.Reset(); |
} |
void Timer::Reset() { |
+ DCHECK(origin_sequence_checker_.CalledOnValidSequence()); |
DCHECK(!user_task_.is_null()); |
- // If there's no pending task, start one up and return. |
- if (!scheduled_task_) { |
+ if (is_running_) { |
+ if (destination_task_runner_) { |
+ destination_task_runner_->PostTask( |
+ FROM_HERE, |
+ base::Bind(&BaseTimerTaskInternal::Reset, scheduled_task_weak_ref_)); |
+ } else { |
+ // |is_running_| guarantees a valid pointer when not using an external |
+ // |destination_task_runner_|. |
+ DCHECK(scheduled_task_weak_ref_); |
+ scheduled_task_weak_ref_->Reset(); |
+ } |
+ } else { |
PostNewScheduledTask(delay_); |
vmpstr
2016/12/03 01:14:04
It's unclear to me if there other call sites other
gab
2016/12/23 20:22:11
The API is a bit of a mess but yes this is require
|
- return; |
} |
- |
- // Set the new desired_run_time_. |
- if (delay_ > TimeDelta::FromMicroseconds(0)) |
- desired_run_time_ = Now() + delay_; |
- else |
- desired_run_time_ = TimeTicks(); |
- |
- // We can use the existing scheduled task if it arrives before the new |
- // desired_run_time_. |
- if (desired_run_time_ >= scheduled_run_time_) { |
- is_running_ = true; |
- return; |
- } |
- |
- // We can't reuse the scheduled_task_, so abandon it and post a new one. |
- AbandonScheduledTask(); |
- PostNewScheduledTask(delay_); |
} |
TimeTicks Timer::Now() const { |
@@ -160,75 +311,65 @@ TimeTicks Timer::Now() const { |
void Timer::SetTaskInfo(const tracked_objects::Location& posted_from, |
TimeDelta delay, |
const base::Closure& user_task) { |
+ DCHECK(origin_sequence_checker_.CalledOnValidSequence()); |
+ DCHECK(!is_running_); |
+ |
posted_from_ = posted_from; |
delay_ = delay; |
user_task_ = user_task; |
} |
void Timer::PostNewScheduledTask(TimeDelta delay) { |
- DCHECK(scheduled_task_ == NULL); |
- is_running_ = true; |
- scheduled_task_ = new BaseTimerTaskInternal(this); |
- if (delay > TimeDelta::FromMicroseconds(0)) { |
- GetTaskRunner()->PostDelayedTask(posted_from_, |
- base::Bind(&BaseTimerTaskInternal::Run, base::Owned(scheduled_task_)), |
- delay); |
- scheduled_run_time_ = desired_run_time_ = Now() + delay; |
- } else { |
- GetTaskRunner()->PostTask(posted_from_, |
- base::Bind(&BaseTimerTaskInternal::Run, base::Owned(scheduled_task_))); |
- scheduled_run_time_ = desired_run_time_ = TimeTicks(); |
- } |
- // Remember the thread ID that posts the first task -- this will be verified |
- // later when the task is abandoned to detect misuse from multiple threads. |
- if (!thread_id_) |
- thread_id_ = static_cast<int>(PlatformThread::CurrentId()); |
-} |
+ DCHECK(origin_sequence_checker_.CalledOnValidSequence()); |
+ DCHECK(SequencedTaskRunnerHandle::IsSet()); |
-scoped_refptr<SingleThreadTaskRunner> Timer::GetTaskRunner() { |
- return task_runner_.get() ? task_runner_ : ThreadTaskRunnerHandle::Get(); |
-} |
+ scoped_refptr<SequencedTaskRunner> destination_task_runner = |
+ destination_task_runner_ ? destination_task_runner_ |
+ : SequencedTaskRunnerHandle::Get(); |
-void Timer::AbandonScheduledTask() { |
- DCHECK(thread_id_ == 0 || |
- thread_id_ == static_cast<int>(PlatformThread::CurrentId())); |
- if (scheduled_task_) { |
- scheduled_task_->Abandon(); |
- scheduled_task_ = NULL; |
- } |
+ is_running_ = true; |
+ BaseTimerTaskInternal* scheduled_task = new BaseTimerTaskInternal( |
+ posted_from_, user_task_, delay, is_repeating_, tick_clock_, |
+ SequencedTaskRunnerHandle::Get(), |
+ base::Bind(&Timer::Stop, weak_ptr_factory_.GetWeakPtr())); |
+ scheduled_task_weak_ref_ = scheduled_task->AsWeakPtr(); |
+ destination_task_runner->PostDelayedTask( |
+ posted_from_, |
+ base::Bind(&BaseTimerTaskInternal::Run, scheduled_task_weak_ref_, |
+ Passed(scheduled_task->GetCleanupTrigger())), |
+ delay); |
} |
-void Timer::RunScheduledTask() { |
- // Task may have been disabled. |
- if (!is_running_) |
- return; |
- |
- // First check if we need to delay the task because of a new target time. |
- if (desired_run_time_ > scheduled_run_time_) { |
- // Now() can be expensive, so only call it if we know the user has changed |
- // the desired_run_time_. |
- TimeTicks now = Now(); |
- // Task runner may have called us late anyway, so only post a continuation |
- // task if the desired_run_time_ is in the future. |
- if (desired_run_time_ > now) { |
- // Post a new task to span the remaining time. |
- PostNewScheduledTask(desired_run_time_ - now); |
- return; |
+void Timer::AbandonScheduledTask() { |
+ DCHECK(origin_sequence_checker_.CalledOnValidSequence()); |
+ |
+ if (is_running_) { |
+ // Toggle |is_running| first to be reentrancy safe, just in case. |
+ is_running_ = false; |
+ |
+ const Closure abandon = |
+ base::Bind(&BaseTimerTaskInternal::Abandon, scheduled_task_weak_ref_); |
+ if (destination_task_runner_) { |
+ destination_task_runner_->PostTask(FROM_HERE, abandon); |
+ } else if (SequencedTaskRunnerHandle::IsSet()) { |
+ // Post Abandon() as an asynchronous call to avoid deleting |
+ // BaseTimerTaskInternal while it handles Run() (reentrancy is possible, |
+ // e.g. when the task itself deletes the Timer or when a OneShotTimer's |
+ // BaseTimerTaskInternal::Run() invoked its |stop_callback_|). |
+ SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE, abandon); |
+ } else { |
+ // |is_running_| guarantees a valid pointer when not using an external |
+ // |destination_task_runner_|. |
+ DCHECK(scheduled_task_weak_ref_); |
+ // The reentrancy situations described above happen while Run() is active |
+ // which implies SequencedTaskRunnerHandle::IsSet(). |
+ // SequencedTaskRunnerHandle not being set typically means this |
+ // AbandonScheduledTask() was triggered by ~Timer() on shutdown in which |
+ // case it's safe to synchronously abandon on sequence. |
+ scheduled_task_weak_ref_->Abandon(); |
} |
+ scheduled_task_weak_ref_ = nullptr; |
} |
- |
- // Make a local copy of the task to run. The Stop method will reset the |
- // user_task_ member if retain_user_task_ is false. |
- base::Closure task = user_task_; |
- |
- if (is_repeating_) |
- PostNewScheduledTask(delay_); |
- else |
- Stop(); |
- |
- task.Run(); |
- |
- // No more member accesses here: *this could be deleted at this point. |
} |
} // namespace base |