[Chromecast] Add an alarm manager for firing events on wall clock time.

chromecast/base/alarm_manager.h

+// Copyright 2017 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 CHROMECAST_BASE_ALARM_MANAGER_H_
+#define CHROMECAST_BASE_ALARM_MANAGER_H_
+
+#include <map>
+#include <queue>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread.h"
+#include "base/time/clock.h"
+#include "base/timer/timer.h"
+
+namespace chromecast {
+
+// Alarm manager allows setting a task for wall clock time rather than for an
+// elapsed amount of time. This is different from using long PostDelayedTasks
+// that are sensitive to time changes, clock drift, and other factors.
+//
+// Alarm manager polls the wall clock time every 5 seconds. If the clock is
+// equal or past the requested time, the alarm will fire.
+//
+// Any thread can add or remove alarms. The alarm will be fired on the original
+// thread used to set the alarm.
+//
+// When an alarm is added to the alarm manager, the task is guaranteed to not
+// run before the clock passes the requested time. The task may not run even if
+// it is past the requested time if the software is suspended. However, once
+// woken up, the event will fire within 5 seconds if the target time has passed.
+class AlarmManager {
+ public:
+  class AlarmDelegate {

[halliwell, 2017/02/20 22:28:26]

I wonder if it would be less intrusive for the caller to pass a Callback?  No
need to implement Delegate interface, they can just bind whatever function they
want.  It might also allow easier lifetime management for clients (e.g. could
bind with weak ptr and no need to RemoveAlarm).

[ryanchung, 2017/02/21 23:36:47]

Done.
I'm wondering if there's a need to explicitly cancel an alarm without destroying
the client.
sanfin@ suggested returning a handle that must be kept alive for each alarm.
That would allow the client to cancel alarms without killing itself. However,
now that adding an alarm involves posting a task to the alarm manager, it's a
little more complicated to implement that.
What do you think?

+   public:
+    // Called when the alarm fires.
+    virtual void OnAlarmFire() = 0;
+
+   protected:
+    virtual ~AlarmDelegate() {}
+  };
+
+  // Construct and start the alarm manager.

[halliwell, 2017/02/20 22:28:26]

Is this intended to be some kind of singleton?  Where will it be instantiated
and accessed from?
Given that we probably don't anticipate super widespread usage (is my assumption
right?), I wonder if a cleaner design would be to make something embeddable in a
class to provide an alarm for just that class.  The downside is that if we had a
lot of alarm clients, it would end up with lots of clock polling.  But if we
anticipate relatively few users, there are benefits ...
* No need for clumsiness of singleton or having to pass a central manager
instance around to where it's needed
* Fewer/no threading concerns
* Simpler implementation (e.g. no need for priority queue, locks)?

[ryanchung, 2017/02/21 23:36:47]

Yes, this will be some sort of singleton instantiated and owned by CCService. 

A quick brainstorm with sanfin@ identified 3 places this would be used initially
(TLS cert update, CRL download, and daily reboot). 

I agree having separate alarms in each class would be cleaner but I'm concerned
that the number of clock polling threads could grow unchecked if usage
increases. I'm not sure how many threads/polls-per-second would have effects on
our performance.
But at that point, is it more beneficial to poll more often on a single poller
to get more accuracy? (vs having multiple separate pollers that poll less
often?)

If the implementation here is too risky, we can go with the separate pollers.

+  AlarmManager();
+  ~AlarmManager();
+
+  // For testing only. Allows setting a fake clock.
+  AlarmManager(std::unique_ptr<base::Clock> clock);
+
+  // Add an alarm.
+  // OnAlarmFire() will be called on |delegate| at around |time|.
+  void AddAlarm(AlarmDelegate* delegate, base::Time time);
+
+  // Remove an alarm.
+  void RemoveAlarm(AlarmDelegate* delegate);
+
+ private:
+  class AlarmInfo {
+   public:
+    AlarmInfo(base::Time time,
+              scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+    ~AlarmInfo();
+    base::Time time_;
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(AlarmInfo);
+  };
+
+  typedef std::map<AlarmDelegate*, std::unique_ptr<AlarmInfo>> AlarmMap;

[halliwell, 2017/02/20 22:28:26]

nit: 'using' preferred over typedef now.

[ryanchung, 2017/02/21 23:36:47]

Done.

+
+  // Check if an alarm should fire.
+  void CheckAlarm();
+  // Start the poller.
+  void Start();

[halliwell, 2017/02/20 22:28:26]

nit: seems like 'Start' code could be inlined in ctor (use delegating ctor for
one form to call the other)

[ryanchung, 2017/02/21 23:36:47]

Done.

+  // Stop the poller.
+  void Stop();

[halliwell, 2017/02/20 22:28:26]

nit: could inline this code in dtor.

[ryanchung, 2017/02/21 23:36:47]

Done.

+
+  base::Lock alarms_lock_;

[halliwell, 2017/02/20 22:28:26]

Is there a strong reason to use a lock (vs posting tasks to add alarms on the
same thread where CheckAlarm runs)?

[ryanchung, 2017/02/21 23:36:47]

Done. Posting is better. Updated code.

+  std::unique_ptr<base::Clock> clock_;
+  std::unique_ptr<base::Timer> clock_tick_timer_;
+  // Store a list of the alarms to fire at a certain time.
+  AlarmMap alarms_;
+  // Store a list of times when a timer should fire.
+  std::priority_queue<base::Time,
+                      std::vector<base::Time>,
+                      std::greater<base::Time>>
+      next_alarm_;
+
+  base::WeakPtrFactory<AlarmManager> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(AlarmManager);
+};
+
+}  // chromecast

[halliwell, 2017/02/20 22:28:26]

nit: "namespace chromecast"

[ryanchung, 2017/02/21 23:36:46]

Done.

+
+#endif  // CHROMECAST_BASE_ALARM_MANAGER_H_