Add SyncScheduler for scheduling CryptAuth enrollments and syncing devices.

Add SyncScheduler for scheduling CryptAuth enrollments and syncing devices.

BUG=420315
TEST=unit test

Committed: https://crrev.com/27e044cfc5c2a43b6c575508aca64763bfa2deba
Cr-Commit-Position: refs/heads/master@{#330713}

Committed: https://crrev.com/a2a7afa4b2312f11f35207dc4f667078964c0cb8
Cr-Commit-Position: refs/heads/master@{#330887}

[Tim Song, 2015-05-15 21:59:32]

Patchset #2 (id:20001) has been deleted

[Tim Song, 2015-05-15 21:59:56]

Patchset #1 (id:1) has been deleted

[Tim Song, 2015-05-15 22:01:00]

tengs@chromium.org changed reviewers:
+ isherman@chromium.org

[Tim Song, 2015-05-15 22:01:00]

Please take a look.

[Ilya Sherman, 2015-05-18 23:25:39]

Thanks, Tim.  Very elegant code, as usual =)

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler.cc (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.cc:15:
is_aggressive_recovery_(is_aggressive_recovery),
What is this variable used for?

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler.h (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:6: #define
COMPONENTS_PROXIMITY_AUTH_CRYPTAUTH_CRYPTAUTH_SYNC_SCHEDULER_H
nit: Duplicate cryptauth?

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:25: enum class State : int
{
nit: Probably no need to specify that the backing storage is int.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:27: PERIODIC_REFRESH,
nit: Maybe add "IDLE" somewhere to this state name to distinguish from the
syncing state?  Or, separate out "is_syncing()" into a method, and reduce this
to just three states?

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:42: void Complete(bool
success);
nit: Perhaps name this "OnDidComplete"?

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:51: bool
is_aggressive_recovery_;
nit: Can this be const?

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:62: // most likely
won't be present after a restart.
nit: Double "most likely"

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:64: failure_count_ =
1;
Why 1 and not 0?

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:124: PA_LOG(INFO) <<
"Scheduling next sync for " << scheduler_name_ << ":\r\n"
nit: Why "\r"?

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:153:
failure_count_++;
nit: ++failure_count_

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:162: base::TimeDelta
jittered_time_delta = period + (period * jitter);
nit: It looks like "max_jitter_percentage_" is actually a simple ratio, not a
percentage (which is 100x a normal ratio).  Let's rename the variable for
clarity.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler_impl.h (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:27: const std::string&
scheduler_name);
nit: Please document the jitter percentage as well.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:33: bool
is_aggressive_recovery) override;
Let's use an enum rather than a boolean for is_aggressive_recovery, so that call
sites are clearer.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:52: // Randomly adds a
random jitter to the value of GetPeriod(). The returned
nit: "Randomly adds a random" -> "Adds a random"

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:74: // period so we
can avoid deterministic behaviour.
nit: Please re-wrap

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:74: // period so we
can avoid deterministic behaviour.
nit: We're not really trying to avoid deterministic behavior; we're just trying
to avoid lots of synchronized calls to the server -- right?

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:77: // The name of the
scheduler used for debugging purposes.
nit: "scheduler used" -> "scheduler, used"

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:85: int
failure_count_;
nit: size_t?

[Tim Song, 2015-05-19 22:13:19]

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler.cc (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.cc:15:
is_aggressive_recovery_(is_aggressive_recovery),
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> What is this variable used for?

I was going to use it to set the InvocationReason field for the requests, but I
added a GetStrategy() instead, so this is unnecessary now.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler.h (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:6: #define
COMPONENTS_PROXIMITY_AUTH_CRYPTAUTH_CRYPTAUTH_SYNC_SCHEDULER_H
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> nit: Duplicate cryptauth?

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:25: enum class State : int
{
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> nit: Probably no need to specify that the backing storage is int.

This is mainly for the logs, so I can cast the enum to an int.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:27: PERIODIC_REFRESH,
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> nit: Maybe add "IDLE" somewhere to this state name to distinguish from the
> syncing state?  Or, separate out "is_syncing()" into a method, and reduce this
> to just three states?

Done. I split this State enum into a SyncState and Strategy enum with the
corresponding getter methods.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:42: void Complete(bool
success);
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> nit: Perhaps name this "OnDidComplete"?

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:51: bool
is_aggressive_recovery_;
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> nit: Can this be const?

I removed this field.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:62: // most likely
won't be present after a restart.
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> nit: Double "most likely"

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:64: failure_count_ =
1;
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> Why 1 and not 0?

This is so the backoff is calculated properly, as in failures, the
AGGRESSIVE_RECOVERY state has failure_count == 1.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:124: PA_LOG(INFO) <<
"Scheduling next sync for " << scheduler_name_ << ":\r\n"
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> nit: Why "\r"?

I think this looks a bit more readable in the logs.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:153:
failure_count_++;
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> nit: ++failure_count_

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:162: base::TimeDelta
jittered_time_delta = period + (period * jitter);
On 2015/05/18 23:25:38, Ilya Sherman wrote:
> nit: It looks like "max_jitter_percentage_" is actually a simple ratio, not a
> percentage (which is 100x a normal ratio).  Let's rename the variable for
> clarity.

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler_impl.h (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:27: const std::string&
scheduler_name);
On 2015/05/18 23:25:39, Ilya Sherman wrote:
> nit: Please document the jitter percentage as well.

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:33: bool
is_aggressive_recovery) override;
On 2015/05/18 23:25:39, Ilya Sherman wrote:
> Let's use an enum rather than a boolean for is_aggressive_recovery, so that
call
> sites are clearer.

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:52: // Randomly adds a
random jitter to the value of GetPeriod(). The returned
On 2015/05/18 23:25:39, Ilya Sherman wrote:
> nit: "Randomly adds a random" -> "Adds a random"

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:74: // period so we
can avoid deterministic behaviour.
On 2015/05/18 23:25:39, Ilya Sherman wrote:
> nit: Please re-wrap

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:74: // period so we
can avoid deterministic behaviour.
On 2015/05/18 23:25:39, Ilya Sherman wrote:
> nit: We're not really trying to avoid deterministic behavior; we're just
trying
> to avoid lots of synchronized calls to the server -- right?

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:77: // The name of the
scheduler used for debugging purposes.
On 2015/05/18 23:25:39, Ilya Sherman wrote:
> nit: "scheduler used" -> "scheduler, used"

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.h:85: int
failure_count_;
On 2015/05/18 23:25:39, Ilya Sherman wrote:
> nit: size_t?

Done.

[Ilya Sherman, 2015-05-19 22:46:55]

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler.h (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:25: enum class State : int
{
On 2015/05/19 22:13:18, Tim Song wrote:
> On 2015/05/18 23:25:38, Ilya Sherman wrote:
> > nit: Probably no need to specify that the backing storage is int.
> 
> This is mainly for the logs, so I can cast the enum to an int.

I think you can cast to an int even without this -- right?  AFAIK, int is
defined to be the default backing type.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:124: PA_LOG(INFO) <<
"Scheduling next sync for " << scheduler_name_ << ":\r\n"
On 2015/05/19 22:13:18, Tim Song wrote:
> On 2015/05/18 23:25:38, Ilya Sherman wrote:
> > nit: Why "\r"?
> 
> I think this looks a bit more readable in the logs.

Sorry, maybe my question wasn't clear enough.  Could "\r\n" be written as simply
"\n" or "\n\n" if you want two newlines?  "\r\n" is a Windows-ism that's
surprising to see in this context.

https://codereview.chromium.org/1147563002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler.h (right):

https://codereview.chromium.org/1147563002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:29: NOT_STARTED = 0,
nit: Is the "= 0" important?  I'd omit it if not.

https://codereview.chromium.org/1147563002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/1147563002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl_unittest.cc:106:
SyncScheduler::Strategy::PERIODIC_REFRESH);
Optional nit: I'd add using stmts for SyncScheduler::Strategy and
SyncScheduler::SyncState for this file.

[Tim Song, 2015-05-20 00:13:22]

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler.h (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler.h:25: enum class State : int
{
On 2015/05/19 22:46:54, Ilya Sherman wrote:
> On 2015/05/19 22:13:18, Tim Song wrote:
> > On 2015/05/18 23:25:38, Ilya Sherman wrote:
> > > nit: Probably no need to specify that the backing storage is int.
> > 
> > This is mainly for the logs, so I can cast the enum to an int.
> 
> I think you can cast to an int even without this -- right?  AFAIK, int is
> defined to be the default backing type.

Done.

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/1147563002/diff/40001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl.cc:124: PA_LOG(INFO) <<
"Scheduling next sync for " << scheduler_name_ << ":\r\n"
On 2015/05/19 22:46:55, Ilya Sherman wrote:
> On 2015/05/19 22:13:18, Tim Song wrote:
> > On 2015/05/18 23:25:38, Ilya Sherman wrote:
> > > nit: Why "\r"?
> > 
> > I think this looks a bit more readable in the logs.
> 
> Sorry, maybe my question wasn't clear enough.  Could "\r\n" be written as
simply
> "\n" or "\n\n" if you want two newlines?  "\r\n" is a Windows-ism that's
> surprising to see in this context.

Done.

https://codereview.chromium.org/1147563002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/sync_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/1147563002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/sync_scheduler_impl_unittest.cc:106:
SyncScheduler::Strategy::PERIODIC_REFRESH);
On 2015/05/19 22:46:55, Ilya Sherman wrote:
> Optional nit: I'd add using stmts for SyncScheduler::Strategy and
> SyncScheduler::SyncState for this file.

Done.

[Ilya Sherman, 2015-05-20 00:14:52]

LGTM, thanks.

[Tim Song, 2015-05-20 07:05:19]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2015-05-20 07:05:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1147563002/80001

[commit-bot: I haz the power, 2015-05-20 07:58:41]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-05-20 07:59:41]

Patchset 3 (id:??) landed as
https://crrev.com/27e044cfc5c2a43b6c575508aca64763bfa2deba
Cr-Commit-Position: refs/heads/master@{#330713}

[eugenis, 2015-05-20 18:55:00]

On 2015/05/20 07:59:41, I haz the power (commit-bot) wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/27e044cfc5c2a43b6c575508aca64763bfa2deba
> Cr-Commit-Position: refs/heads/master@{#330713}

This change results in MSan reports:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests...

and Valgrind reports:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v...

Reverting.

[eugenis, 2015-05-20 18:55:41]

A revert of this CL (patchset #3 id:80001) has been created in
https://codereview.chromium.org/1146193002/ by eugenis@chromium.org.

The reason for reverting is: MSan and Valgrind failures (use-of-uninitialized
memory)..

[Tim Song, 2015-05-21 02:11:47]

Fixed uninitialized variable error.

components_unittests on MSAN bots pass, so I'm resubmitting.

[Tim Song, 2015-05-21 02:11:51]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2015-05-21 02:11:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1147563002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-05-21 02:12:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1147563002/100001

[commit-bot: I haz the power, 2015-05-21 04:38:17]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-05-21 04:41:15]

Patchset 4 (id:??) landed as
https://crrev.com/a2a7afa4b2312f11f35207dc4f667078964c0cb8
Cr-Commit-Position: refs/heads/master@{#330887}