sync: Refactor job ownership in SyncScheduler

sync/engine/sync_scheduler_impl.h

Index: sync/engine/sync_scheduler_impl.h
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index f3c4d5aad856df0123572d5122543b9e5af4e4bf..61c78ffd3a53312f8d8e401331411fba6ff4a69a 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -150,10 +150,6 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl : public SyncScheduler {
     bool had_nudge;
     base::TimeDelta length;
     base::OneShotTimer<SyncSchedulerImpl> timer;
-
-    // Configure jobs are saved only when backing off or throttling. So we
-    // expose the pointer here (does not own, similar to pending_nudge).
-    SyncSessionJob* pending_configure_job;
   };
 
   static const char* GetModeString(Mode mode);
@@ -167,15 +163,21 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl : public SyncScheduler {
                        const base::Closure& task,
                        base::TimeDelta delay);
 
-  // Invoke the Syncer to perform a non-poll job.
-  bool DoSyncSessionJob(scoped_ptr<SyncSessionJob> job,
-                        JobPriority priority);
+  // Invoke the syncer to perform a non-POLL job.
+  bool DoSyncSessionJobImpl(scoped_ptr<SyncSessionJob> job,
+                            JobPriority priority);
+
+  // Invoke the syncer to perform a nudge job.
+  bool DoNudgeSyncSessionJob(JobPriority priority);
+
+  // Invoke the syncer to perform a configuration job.
+  bool DoConfigurationSyncSessionJob(JobPriority priority);
 
   // Returns whether or not it's safe to run a poll job at this time.
   bool ShouldPoll();
 
   // Invoke the Syncer to perform a poll job.
-  void DoPollSyncSessionJob(scoped_ptr<SyncSessionJob> job);
+  void DoPollSyncSessionJob();
 
   // Called after the Syncer has performed the sync represented by |job|, to
   // reset our state.  |exited_prematurely| is true if the Syncer did not
@@ -192,8 +194,13 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl : public SyncScheduler {
   // Helper to configure polling intervals. Used by Start and ScheduleNextSync.
   void AdjustPolling(const SyncSessionJob* old_job);
 
+  // Resumes an existing wait interval, taking into account the time already
+  // spent waiting.  Calling this funciton on a wait interval that is still
+  // active will have not effect, give or take a few small timing side-effects.
+  void ResumeWaiting();
+
   // Helper to restart waiting with |wait_interval_|'s timer.
-  void RestartWaiting(scoped_ptr<SyncSessionJob> job);
+  void RestartWaiting();
 
   // Helper to ScheduleNextSync in case of consecutive sync errors.
   void HandleContinuationError(scoped_ptr<SyncSessionJob> old_job,
@@ -203,10 +210,6 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl : public SyncScheduler {
   JobProcessDecision DecideOnJob(const SyncSessionJob& job,
                                  JobPriority priority);
 
-  // If DecideOnJob decides that |job| should be SAVEd, this function will
-  // carry out the task of actually "saving" (or coalescing) the job.
-  void HandleSaveJobDecision(scoped_ptr<SyncSessionJob> job);
-
   // Decide on whether to CONTINUE, SAVE or DROP the job when we are in
   // backoff mode.
   JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job,
@@ -235,22 +238,12 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl : public SyncScheduler {
   // Helper to signal listeners about changed retry time
   void NotifyRetryTime(base::Time retry_time);
 
-  // Callback to change backoff state. |to_be_canary| in both cases is the job
-  // that should be granted canary privileges. Note: it is possible that the
-  // job that gets scheduled when this callback is scheduled is different from
-  // the job that will actually get executed, because other jobs may have been
-  // scheduled while we were waiting for the callback.
-  void DoCanaryJob(scoped_ptr<SyncSessionJob> to_be_canary);
-  void Unthrottle(scoped_ptr<SyncSessionJob> to_be_canary);
-
-  // Returns a pending job that has potential to run given the state of the
-  // scheduler, if it exists. Useful whenever an event occurs that may
-  // change conditions that permit a job to run, such as re-establishing
-  // network connection, auth refresh, mode changes etc. Note that the returned
-  // job may have been scheduled to run at a later time, or may have been
-  // unscheduled.  In the former case, this will result in abandoning the old
-  // job and effectively cancelling it.
-  scoped_ptr<SyncSessionJob> TakePendingJobForCurrentMode();
+  // Looks for pending work and, if it finds any, run this work at "canary"
+  // priority.
+  void TryCanaryJob();
+
+  // Transitions out of the THROTTLED WaitInterval then calls TryCanaryJob().
+  void Unthrottle();
 
   // Called when the root cause of the current connection error is fixed.
   void OnServerConnectionErrorFixed();
@@ -304,19 +297,6 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl : public SyncScheduler {
   // The mode of operation.
   Mode mode_;
 
-  // Tracks (does not own) in-flight nudges (scheduled or unscheduled),
-  // so we can coalesce. NULL if there is no pending nudge.
-  SyncSessionJob* pending_nudge_;
-
-  // There are certain situations where we want to remember a nudge, but
-  // there is no well defined moment in time in the future when that nudge
-  // should run, e.g. if it requires a mode switch or updated auth credentials.
-  // This member will own NUDGE jobs in those cases, until an external event
-  // (mode switch or fixed auth) occurs to trigger a retry.  Should be treated
-  // as opaque / not interacted with (i.e. we could build a wrapper to
-  // hide the type, but that's probably overkill).
-  scoped_ptr<SyncSessionJob> unscheduled_nudge_storage_;
-
   // Current wait state.  Null if we're not in backoff and not throttled.
   scoped_ptr<WaitInterval> wait_interval_;
 
@@ -326,6 +306,14 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl : public SyncScheduler {
   // We will cancel this task before starting a new one.
   base::CancelableClosure pending_wakeup_;
 
+  // Pending configure job storage.  Note that
+  // (mode_ != CONFIGURATION_MODE) \implies !pending_configure_job_.
+  scoped_ptr<SyncSessionJob> pending_configure_job_;

[tim (not reviewing), 2013/04/04 00:22:11]

Why does this belong here? Unless I'm mistaken this introduces a state that
should not logically exist. Can't we avoid that entropy? We can only have a
pending config job if it is part of the WaitInterval, and it'd be clearer if the
ownership model reflected it in some way.  Are you planning to address that
somehow?

[rlarocque, 2013/04/04 00:59:48]

I'm not that worried about it.  We have plenty of DCHECKs when heading into our
out of CONFIGURATION_MODE to catch misuse of this variable.  It's not that hard
to enforce our assumptions.  It will be even easier to enforce them when this
pending_configure_job_ is replaced with an instance of ConfigurationParams
(which is the only one of this SyncSessionJob's members that we really care
about).

I could make the same argument that WaitInterval shouldn't have a
pending_configure_job_ member when we're not in CONFIGURATION_MODE.  (That
member was removed in this patch, btw.)

If we really wanted to fix it, we'd need to use something like this:
http://en.wikipedia.org/wiki/State_pattern.  I'd rather not go down that road,
though.  That pattern makes it difficult to implement transitions between
states, and requires a lot of boilerplate code.  I think a few DCHECKs is the
least bad solution here.

[tim (not reviewing), 2013/04/04 18:07:30]

Ok. A small concern I have is as the need to add nudge or config or new-job
specific state arises it will become increasingly hard to reason about what
members are valid when; hopefully "the next guy" will carefully consider and
replicate the DCHECKs you've added for whatever members happen to be the only
config state today. A comment near member declaration, similar to what you've
said here pointing out what applies to what mode/scheduler state may help (maybe
when you get around to removing SyncSchedulerJob).

+
+  // Pending nudge job storage.  These jobs can exist in CONFIGURATION_MODE, but
+  // they will be run only in NORMAL_MODE.
+  scoped_ptr<SyncSessionJob> pending_nudge_job_;
+
   // Invoked to run through the sync cycle.
   scoped_ptr<Syncer> syncer_;