Commit 0531a97f authored by falken's avatar falken Committed by Commit bot

Revert of C++ readability review (patchset #10 id:180001 of...

Revert of C++ readability review (patchset #10 id:180001 of https://codereview.chromium.org/706993003/)

Reason for revert:
It looks like this broke tests the following tests on Linux Chromium OS:
AlarmTimerTest.OneShotTimerConcurrentResetAndTimerFired
AlarmTimerTest.RepeatingTimerConcurrentResetAndTimerFired

Example output:
AlarmTimerTest.OneShotTimerConcurrentResetAndTimerFired (run #1):
[ RUN      ] AlarmTimerTest.OneShotTimerConcurrentResetAndTimerFired
[11334:11334:0406/201742:404219967:INFO:alarm_timer_chromeos.cc(166)] CLOCK_REALTIME_ALARM not supported on this system: Invalid argument
[11334:11334:0406/201742:404230154:FATAL:timer.cc(116)] Check failed: !user_task_.is_null().

AlarmTimerTest.RepeatingTimerConcurrentResetAndTimerFired (run #1):
[ RUN      ] AlarmTimerTest.RepeatingTimerConcurrentResetAndTimerFired
[11382:11382:0406/201748:411022458:INFO:alarm_timer_chromeos.cc(166)] CLOCK_REALTIME_ALARM not supported on this system: Invalid argument
../../components/timers/alarm_timer_unittest.cc:469: Failure
Value of: did_run
  Actual: true
Expected: false

Example build:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/996

Original issue's description:
> C++ readability review
>
> BUG=b/18275087
>
> Committed: https://crrev.com/05e4d854dca737cfb43cd967cd72d024f6b1625b
> Cr-Commit-Position: refs/heads/master@{#324004}

TBR=derat@chromium.org,gromer@google.com,zea@chromium.org,chirantan@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=b/18275087

Review URL: https://codereview.chromium.org/1058693003

Cr-Commit-Position: refs/heads/master@{#324009}
parent 7e939708
......@@ -25,7 +25,7 @@
#include "net/url_request/url_request_context_getter.h"
#if defined(OS_CHROMEOS)
#include "components/timers/alarm_timer_chromeos.h"
#include "components/timers/alarm_timer.h"
#endif
namespace gcm {
......@@ -344,7 +344,7 @@ void GCMDriverDesktop::IOWorker::WakeFromSuspendForHeartbeat(bool wake) {
scoped_ptr<base::Timer> timer;
if (wake)
timer.reset(new timers::SimpleAlarmTimer());
timer.reset(new timers::AlarmTimer(true, false));
else
timer.reset(new base::Timer(true, false));
......
......@@ -15,8 +15,10 @@
'../base/base.gyp:base',
],
'sources': [
'timers/alarm_timer_chromeos.cc',
'timers/alarm_timer_chromeos.h',
'timers/alarm_timer.cc',
'timers/alarm_timer.h',
'timers/rtc_alarm.cc',
'timers/rtc_alarm.h',
],
},
],
......
......@@ -4,8 +4,10 @@
static_library("timers") {
sources = [
"alarm_timer_chromeos.cc",
"alarm_timer_chromeos.h",
"alarm_timer.cc",
"alarm_timer.h",
"rtc_alarm.cc",
"rtc_alarm.h",
]
deps = [
......
// Copyright 2014 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "components/timers/alarm_timer.h"
#include "base/bind.h"
#include "base/bind_helpers.h"
#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/pending_task.h"
#include "components/timers/rtc_alarm.h"
namespace timers {
AlarmTimer::AlarmTimer(bool retain_user_task, bool is_repeating)
: base::Timer(retain_user_task, is_repeating),
delegate_(new RtcAlarm()),
can_wake_from_suspend_(false),
origin_message_loop_(NULL),
weak_factory_(this) {
can_wake_from_suspend_ = delegate_->Init(weak_factory_.GetWeakPtr());
}
AlarmTimer::AlarmTimer(const tracked_objects::Location& posted_from,
base::TimeDelta delay,
const base::Closure& user_task,
bool is_repeating)
: base::Timer(posted_from, delay, user_task, is_repeating),
delegate_(new RtcAlarm()),
can_wake_from_suspend_(false),
origin_message_loop_(NULL),
weak_factory_(this) {
can_wake_from_suspend_ = delegate_->Init(weak_factory_.GetWeakPtr());
}
AlarmTimer::~AlarmTimer() {
Stop();
}
void AlarmTimer::OnTimerFired() {
if (!base::Timer::IsRunning())
return;
DCHECK(pending_task_.get());
// Take ownership of the pending user task, which is going to be cleared by
// the Stop() or Reset() functions below.
scoped_ptr<base::PendingTask> pending_user_task(pending_task_.Pass());
// Re-schedule or stop the timer as requested.
if (base::Timer::is_repeating())
Reset();
else
Stop();
// Now run the user task.
base::MessageLoop::current()->task_annotator()->RunTask(
"AlarmTimer::Reset", "AlarmTimer::OnTimerFired", *pending_user_task);
}
void AlarmTimer::Stop() {
if (!can_wake_from_suspend_) {
base::Timer::Stop();
return;
}
// Clear the running flag, stop the delegate, and delete the pending task.
base::Timer::set_is_running(false);
delegate_->Stop();
pending_task_.reset();
// Stop is called when the AlarmTimer is destroyed so we need to remove
// ourselves as a MessageLoop::DestructionObserver to prevent a segfault
// later.
if (origin_message_loop_) {
origin_message_loop_->RemoveDestructionObserver(this);
origin_message_loop_ = NULL;
}
if (!base::Timer::retain_user_task())
base::Timer::set_user_task(base::Closure());
}
void AlarmTimer::Reset() {
if (!can_wake_from_suspend_) {
base::Timer::Reset();
return;
}
DCHECK(!base::Timer::user_task().is_null());
DCHECK(!origin_message_loop_ ||
origin_message_loop_->task_runner()->RunsTasksOnCurrentThread());
// Make sure that the timer will stop if the underlying message loop is
// destroyed.
if (!origin_message_loop_) {
origin_message_loop_ = base::MessageLoop::current();
origin_message_loop_->AddDestructionObserver(this);
}
// Set up the pending task.
if (base::Timer::GetCurrentDelay() > base::TimeDelta::FromMicroseconds(0)) {
base::Timer::set_desired_run_time(
base::TimeTicks::Now() + base::Timer::GetCurrentDelay());
pending_task_.reset(new base::PendingTask(base::Timer::posted_from(),
base::Timer::user_task(),
base::Timer::desired_run_time(),
true /* nestable */));
} else {
base::Timer::set_desired_run_time(base::TimeTicks());
pending_task_.reset(new base::PendingTask(base::Timer::posted_from(),
base::Timer::user_task()));
}
base::MessageLoop::current()->task_annotator()->DidQueueTask(
"AlarmTimer::Reset", *pending_task_);
// Now start up the timer.
delegate_->Reset(base::Timer::GetCurrentDelay());
base::Timer::set_is_running(true);
}
void AlarmTimer::WillDestroyCurrentMessageLoop() {
Stop();
}
} // namespace timers
......@@ -2,18 +2,18 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef COMPONENTS_TIMERS_ALARM_TIMER_CHROMEOS_H_
#define COMPONENTS_TIMERS_ALARM_TIMER_CHROMEOS_H_
#ifndef COMPONENTS_TIMER_ALARM_TIMER_H_
#define COMPONENTS_TIMER_ALARM_TIMER_H_
#include "base/callback.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/message_loop/message_loop.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
namespace base {
class MessageLoop;
struct PendingTask;
}
......@@ -24,45 +24,61 @@ namespace timers {
// Currently, this feature is only available on Chrome OS systems running linux
// version 3.11 or higher. On all other platforms, the AlarmTimer behaves
// exactly the same way as a regular Timer.
class AlarmTimer : public base::Timer {
class AlarmTimer : public base::Timer,
public base::MessageLoop::DestructionObserver {
public:
~AlarmTimer() override;
bool can_wake_from_suspend() const { return can_wake_from_suspend_; }
// The delegate is responsible for managing the system level details for
// actually setting up and monitoring a timer that is capable of waking the
// system from suspend. This class is reference counted because it may need
// to outlive the timer in order to clean up after itself.
class Delegate : public base::RefCountedThreadSafe<Delegate> {
public:
// Initializes the timer. Should return true iff the system has timers that
// can wake it up from suspend. Will only be called once.
virtual bool Init(base::WeakPtr<AlarmTimer> timer) = 0;
// Sets a hook that will be called when the timer fires and a task has been
// queued on |origin_message_loop_|. Used by tests to wait until a task is
// pending in the MessageLoop.
void SetTimerFiredCallbackForTest(base::Closure test_callback);
// Stops the currently running timer. It should be safe to call this more
// than once.
virtual void Stop() = 0;
// Timer overrides.
void Stop() override;
void Reset() override;
// Resets the timer to fire after |delay| has passed. Cancels any
// pre-existing delay.
virtual void Reset(base::TimeDelta delay) = 0;
protected:
// The constructors for this class are protected because consumers should
// instantiate one of the specialized sub-classes defined below instead.
virtual ~Delegate() {}
private:
friend class base::RefCountedThreadSafe<Delegate>;
};
AlarmTimer(bool retain_user_task, bool is_repeating);
AlarmTimer(const tracked_objects::Location& posted_from,
base::TimeDelta delay,
const base::Closure& user_task,
bool is_repeating);
private:
// Common initialization that must be performed by both constructors. This
// really should live in a delegated constructor but the way base::Timer's
// constructors are written makes it really hard to do so.
void Init();
~AlarmTimer() override;
// Will be called by the delegate to indicate that the timer has fired and
bool can_wake_from_suspend() const { return can_wake_from_suspend_; }
// Must be called by the delegate to indicate that the timer has fired and
// that the user task should be run.
void OnTimerFired();
// Called when |origin_message_loop_| will be destroyed.
void WillDestroyCurrentMessageLoop();
// Timer overrides.
void Stop() override;
void Reset() override;
// MessageLoop::DestructionObserver overrides.
void WillDestroyCurrentMessageLoop() override;
private:
// Initializes the timer with the appropriate delegate.
void Init();
// Delegate that will manage actually setting the timer.
class Delegate;
scoped_refptr<Delegate> delegate_;
// Keeps track of the user task we want to run. A new one is constructed
......@@ -79,68 +95,11 @@ class AlarmTimer : public base::Timer {
// destruction of that message loop.
base::MessageLoop* origin_message_loop_;
// Observes |origin_message_loop_| and informs this class if it will be
// destroyed.
class MessageLoopObserver;
scoped_ptr<MessageLoopObserver> message_loop_observer_;
base::WeakPtrFactory<AlarmTimer> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(AlarmTimer);
};
// As its name suggests, a OneShotAlarmTimer runs a given task once. It does
// not remember the task that was given to it after it has fired and does not
// repeat. Useful for fire-and-forget tasks.
class OneShotAlarmTimer : public AlarmTimer {
public:
// Constructs a basic OneShotAlarmTimer. An AlarmTimer constructed this way
// requires that Start() is called before Reset() is called.
OneShotAlarmTimer();
~OneShotAlarmTimer() override;
};
// A RepeatingAlarmTimer takes a task and delay and repeatedly runs the task
// using the specified delay as an interval between the runs until it is
// explicitly stopped. It remembers both the task and the delay it was given
// after it fires.
class RepeatingAlarmTimer : public AlarmTimer {
public:
// Constructs a basic RepeatingAlarmTimer. An AlarmTimer constructed this way
// requires that Start() is called before Reset() is called.
RepeatingAlarmTimer();
// Constructs a RepeatingAlarmTimer with pre-populated parameters but does not
// start it. Useful if |user_task| or |delay| are not going to change.
// Reset() can be called immediately after constructing an AlarmTimer in this
// way.
RepeatingAlarmTimer(const tracked_objects::Location& posted_from,
base::TimeDelta delay,
const base::Closure& user_task);
~RepeatingAlarmTimer() override;
};
// A SimpleAlarmTimer only fires once but remembers the task that it was given
// even after it has fired. Useful if you want to run the same task multiple
// times but not at a regular interval.
class SimpleAlarmTimer : public AlarmTimer {
public:
// Constructs a basic SimpleAlarmTimer. An AlarmTimer constructed this way
// requires that Start() is called before Reset() is called.
SimpleAlarmTimer();
// Constructs a SimpleAlarmTimer with pre-populated parameters but does not
// start it. Useful if |user_task| or |delay| are not going to change.
// Reset() can be called immediately after constructing an AlarmTimer in this
// way.
SimpleAlarmTimer(const tracked_objects::Location& posted_from,
base::TimeDelta delay,
const base::Closure& user_task);
~SimpleAlarmTimer() override;
};
} // namespace timers
#endif // COMPONENTS_TIMERS_ALARM_TIMER_CHROMEOS_H_
#endif // COMPONENTS_TIMER_ALARM_TIMER_H_
This diff is collapsed.
This diff is collapsed.
// Copyright 2014 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "components/timers/rtc_alarm.h"
#include <sys/timerfd.h>
#include "base/bind.h"
#include "base/files/file_util.h"
#include "base/lazy_instance.h"
#include "base/logging.h"
#include "base/macros.h"
#include "base/message_loop/message_loop_proxy.h"
#include "base/threading/thread.h"
namespace timers {
namespace {
// Helper class to ensure that the IO thread we will use for watching file
// descriptors is started only once.
class IOThreadStartHelper {
public:
IOThreadStartHelper() : thread_(new base::Thread("RTC Alarm IO Thread")) {
CHECK(thread_->StartWithOptions(
base::Thread::Options(base::MessageLoop::TYPE_IO, 0)));
}
~IOThreadStartHelper() {}
base::Thread& operator*() const { return *thread_.get(); }
base::Thread* operator->() const { return thread_.get(); }
private:
scoped_ptr<base::Thread> thread_;
};
base::LazyInstance<IOThreadStartHelper> g_io_thread = LAZY_INSTANCE_INITIALIZER;
} // namespace
RtcAlarm::RtcAlarm()
: alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)),
origin_event_id_(0),
io_event_id_(0) {
}
RtcAlarm::~RtcAlarm() {
if (alarm_fd_ != -1)
close(alarm_fd_);
}
bool RtcAlarm::Init(base::WeakPtr<AlarmTimer> parent) {
parent_ = parent;
return alarm_fd_ != -1;
}
void RtcAlarm::Stop() {
// Make sure that we stop the RTC from a MessageLoopForIO.
if (!base::MessageLoopForIO::IsCurrent()) {
g_io_thread.Get()->task_runner()->PostTask(
FROM_HERE,
base::Bind(&RtcAlarm::Stop, scoped_refptr<RtcAlarm>(this)));
return;
}
// Stop watching for events.
fd_watcher_.reset();
// Now clear the timer.
DCHECK_NE(alarm_fd_, -1);
itimerspec blank_time = {};
timerfd_settime(alarm_fd_, 0, &blank_time, NULL);
}
void RtcAlarm::Reset(base::TimeDelta delay) {
// Get a proxy for the current message loop. When the timer fires, we will
// post tasks to this proxy to let the parent timer know.
origin_message_loop_ = base::MessageLoopProxy::current();
// Increment the event id. Used to invalidate any events that have been
// queued but not yet run since the last time Reset() was called.
origin_event_id_++;
// Calling timerfd_settime with a zero delay actually clears the timer so if
// the user has requested a zero delay timer, we need to handle it
// differently. We queue the task here but we still go ahead and call
// timerfd_settime with the zero delay anyway to cancel any previous delay
// that might have been programmed.
if (delay <= base::TimeDelta::FromMicroseconds(0)) {
// The timerfd_settime documentation is vague on what happens when it is
// passed a negative delay. We can sidestep the issue by ensuring that the
// delay is 0.
delay = base::TimeDelta::FromMicroseconds(0);
origin_message_loop_->PostTask(FROM_HERE,
base::Bind(&RtcAlarm::OnTimerFired,
scoped_refptr<RtcAlarm>(this),
origin_event_id_));
}
// Make sure that we are running on a MessageLoopForIO.
if (!base::MessageLoopForIO::IsCurrent()) {
g_io_thread.Get()->task_runner()->PostTask(
FROM_HERE,
base::Bind(&RtcAlarm::ResetImpl,
scoped_refptr<RtcAlarm>(this),
delay,
origin_event_id_));
return;
}
ResetImpl(delay, origin_event_id_);
}
void RtcAlarm::OnFileCanReadWithoutBlocking(int fd) {
DCHECK_EQ(alarm_fd_, fd);
// Read from the fd to ack the event.
char val[sizeof(uint64_t)];
base::ReadFromFD(alarm_fd_, val, sizeof(uint64_t));
// Make sure that the parent timer is informed on the proper message loop.
if (origin_message_loop_->RunsTasksOnCurrentThread()) {
OnTimerFired(io_event_id_);
return;
}
origin_message_loop_->PostTask(FROM_HERE,
base::Bind(&RtcAlarm::OnTimerFired,
scoped_refptr<RtcAlarm>(this),
io_event_id_));
}
void RtcAlarm::OnFileCanWriteWithoutBlocking(int fd) {
NOTREACHED();
}
void RtcAlarm::ResetImpl(base::TimeDelta delay, int event_id) {
DCHECK(base::MessageLoopForIO::IsCurrent());
DCHECK_NE(alarm_fd_, -1);
// Store the event id in the IO thread variable. When the timer fires, we
// will bind this value to the OnTimerFired callback to ensure that we do the
// right thing if the timer gets reset.
io_event_id_ = event_id;
// If we were already watching the fd, this will stop watching it.
fd_watcher_.reset(new base::MessageLoopForIO::FileDescriptorWatcher);
// Start watching the fd to see when the timer fires.
if (!base::MessageLoopForIO::current()->WatchFileDescriptor(
alarm_fd_,
false,
base::MessageLoopForIO::WATCH_READ,
fd_watcher_.get(),
this)) {
LOG(ERROR) << "Error while attempting to watch file descriptor for RTC "
<< "alarm. Timer will not fire.";
}
// Actually set the timer. This will also clear the pre-existing timer, if
// any.
itimerspec alarm_time = {};
alarm_time.it_value.tv_sec = delay.InSeconds();
alarm_time.it_value.tv_nsec =
(delay.InMicroseconds() % base::Time::kMicrosecondsPerSecond) *
base::Time::kNanosecondsPerMicrosecond;
if (timerfd_settime(alarm_fd_, 0, &alarm_time, NULL) < 0)
PLOG(ERROR) << "Error while setting alarm time. Timer will not fire";
}
void RtcAlarm::OnTimerFired(int event_id) {
DCHECK(origin_message_loop_->RunsTasksOnCurrentThread());
// Check to make sure that the timer was not reset in the time between when
// this task was queued to run and now. If it was reset, then don't do
// anything.
if (event_id != origin_event_id_)
return;
if (parent_)
parent_->OnTimerFired();
}
} // namespace timers
// Copyright 2014 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef COMPONENTS_TIMER_RTC_ALARM_H_
#define COMPONENTS_TIMER_RTC_ALARM_H_
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
#include "base/memory/weak_ptr.h"
#include "base/message_loop/message_loop.h"
#include "base/time/time.h"
#include "components/timers/alarm_timer.h"
namespace base {
class MessageLoopProxy;
}
namespace timers {
// This class manages a Real Time Clock (RTC) alarm, a feature that is available
// from linux version 3.11 onwards. It creates a file descriptor for the RTC
// alarm timer and then watches that file descriptor to see when it can be read
// without blocking, indicating that the timer has fired.
//
// A major problem for this class is that watching file descriptors is only
// available on a MessageLoopForIO but there is no guarantee the timer is going
// to be created on one. To get around this, the timer has a dedicated thread
// with a MessageLoopForIO that posts tasks back to the thread that started the
// timer.
//
// This class is designed to work together with the AlarmTimer class and is
// tested through the AlarmTimer unit tests. DO NOT TRY TO USE THIS CLASS
// DIRECTLY. Or use it anyway, I'm a comment not a cop.
class RtcAlarm : public AlarmTimer::Delegate,
public base::MessageLoopForIO::Watcher {
public:
RtcAlarm();
// AlarmTimer::Delegate overrides.
bool Init(base::WeakPtr<AlarmTimer> timer) override;
void Stop() override;
void Reset(base::TimeDelta delay) override;
// base::MessageLoopForIO::Watcher overrides.
void OnFileCanReadWithoutBlocking(int fd) override;
void OnFileCanWriteWithoutBlocking(int fd) override;
protected:
// Needs to be protected because AlarmTimer::Delegate is a refcounted class.
~RtcAlarm() override;
private:
// Actually performs the system calls to set up the timer. This must be
// called on a MessageLoopForIO.
void ResetImpl(base::TimeDelta delay, int event_id);
// Callback that is run when the timer fires. Must be run on
// |origin_message_loop_|.
void OnTimerFired(int event_id);
// File descriptor associated with the alarm timer.
int alarm_fd_;
// Message loop which initially started the timer.
scoped_refptr<base::MessageLoopProxy> origin_message_loop_;
// The parent timer that should be informed when the timer fires. We may end
// up outliving the parent so we need to ensure the reference is valid before
// we try to call it.
base::WeakPtr<AlarmTimer> parent_;
// Manages watching file descriptors.
scoped_ptr<base::MessageLoopForIO::FileDescriptorWatcher> fd_watcher_;
// These two variables are used for coordinating between the thread that
// started the timer and the IO thread being used to watch the timer file
// descriptor. When Reset() is called, the original thread increments
// |origin_event_id_| and binds its value to ResetImpl(), which gets posted to
// the IO thread. When the IO thread runs, it saves this value in
// |io_event_id_|. Later, when the timer fires, the IO thread binds the value
// of |io_event_id_| to OnTimerFired() and posts it to the original thread.
// When the original thread runs OnTimerFired(), it calls
// parent_->OnTimerFired() only if |origin_event_id_| matches the event id
// that was passed in to it. This is used to get around a race condition
// where the user resets the timer on the original thread, while the event is
// being fired on the IO thread at the same time.
int origin_event_id_;
int io_event_id_;
DISALLOW_COPY_AND_ASSIGN(RtcAlarm);
};
} // namespace timers
#endif // COMPONENTS_TIMER_RTC_ALARM_H_
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment