sync: make scheduling logic and job ownership more obvious.

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 cf5e24c519c2ac9f5920b2a1b850becfa4dfcdb5..85c9de792fe68eee979992e3af46edcbb0669be7 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -20,6 +20,7 @@
 #include "sync/engine/net/server_connection_manager.h"
 #include "sync/engine/nudge_source.h"
 #include "sync/engine/sync_scheduler.h"
+#include "sync/engine/sync_session_job.h"
 #include "sync/engine/syncer.h"
 #include "sync/internal_api/public/base/model_type_state_map.h"
 #include "sync/internal_api/public/engine/polling_constants.h"
@@ -87,40 +88,6 @@ class SyncSchedulerImpl : public SyncScheduler {
     DROP,
   };
 
-  struct SyncSessionJob {
-    // An enum used to describe jobs for scheduling purposes.
-    enum SyncSessionJobPurpose {
-      // Uninitialized state, should never be hit in practice.
-      UNKNOWN = -1,
-      // Our poll timer schedules POLL jobs periodically based on a server
-      // assigned poll interval.
-      POLL,
-      // A nudge task can come from a variety of components needing to force
-      // a sync.  The source is inferable from |session.source()|.
-      NUDGE,
-      // Typically used for fetching updates for a subset of the enabled types
-      // during initial sync or reconfiguration.
-      CONFIGURATION,
-    };
-    SyncSessionJob();
-    SyncSessionJob(SyncSessionJobPurpose purpose, base::TimeTicks start,
-        linked_ptr<sessions::SyncSession> session, bool is_canary_job,
-        const ConfigurationParams& config_params,
-        const tracked_objects::Location& nudge_location);
-    ~SyncSessionJob();
-    static const char* GetPurposeString(SyncSessionJobPurpose purpose);
-
-    SyncSessionJobPurpose purpose;
-    base::TimeTicks scheduled_start;
-    linked_ptr<sessions::SyncSession> session;
-    bool is_canary_job;
-    ConfigurationParams config_params;
-
-    // This is the location the job came from.  Used for debugging.
-    // In case of multiple nudges getting coalesced this stores the
-    // first location that came in.
-    tracked_objects::Location from_here;
-  };
   friend class SyncSchedulerTest;
   friend class SyncSchedulerWhiteboxTest;
   friend class SyncerTest;
@@ -172,20 +139,14 @@ class SyncSchedulerImpl : public SyncScheduler {
     base::OneShotTimer<SyncSchedulerImpl> timer;
 
     // Configure jobs are saved only when backing off or throttling. So we
-    // expose the pointer here.
-    scoped_ptr<SyncSessionJob> pending_configure_job;
+    // expose the pointer here (does not own, similar to pending_nudge).
+    SyncSessionJob* pending_configure_job;
   };
 
   static const char* GetModeString(Mode mode);
 
   static const char* GetDecisionString(JobProcessDecision decision);
 
-  // Assign |start| and |end| to appropriate SyncerStep values for the
-  // specified |purpose|.
-  static void SetSyncerStepsForPurpose(
-      SyncSessionJob::SyncSessionJobPurpose purpose,
-      SyncerStep* start, SyncerStep* end);
-
   // Helpers that log before posting to |sync_loop_|.  These will only post
   // the task in between calls to Start/Stop.
   void PostTask(const tracked_objects::Location& from_here,
@@ -197,44 +158,43 @@ class SyncSchedulerImpl : public SyncScheduler {
                        base::TimeDelta delay);
 
   // Helper to assemble a job and post a delayed task to sync.
-  void ScheduleSyncSessionJob(const SyncSessionJob& job);
+  void ScheduleSyncSessionJob(scoped_ptr<SyncSessionJob> job);
 
   // Invoke the Syncer to perform a sync.
-  void DoSyncSessionJob(const SyncSessionJob& job);
+  bool DoSyncSessionJob(scoped_ptr<SyncSessionJob> job);
 
   // Called after the Syncer has performed the sync represented by |job|, to
-  // reset our state.
-  void FinishSyncSessionJob(const SyncSessionJob& job);
+  // reset our state.  |premature_exit| is true if the Syncer did not manage
+  // to cycle from job.start_step() to job.end_step(), likely because the
+  // scheduler was forced to quit the job mid-way through.
+  void FinishSyncSessionJob(SyncSessionJob* job, bool exited_prematurely);
 
   // Helper to FinishSyncSessionJob to schedule the next sync operation.
-  void ScheduleNextSync(const SyncSessionJob& old_job);
+  void ScheduleNextSync(const SyncSessionJob* old_job);
 
   // Helper to configure polling intervals. Used by Start and ScheduleNextSync.
   void AdjustPolling(const SyncSessionJob* old_job);
 
   // Helper to restart waiting with |wait_interval_|'s timer.
-  void RestartWaiting();
+  void RestartWaiting(scoped_ptr<SyncSessionJob> job);
 
   // Helper to ScheduleNextSync in case of consecutive sync errors.
-  void HandleContinuationError(const SyncSessionJob& old_job);
+  void HandleContinuationError(const SyncSessionJob* old_job);
 
   // Determines if it is legal to run |job| by checking current
-  // operational mode, backoff or throttling, freshness
-  // (so we don't make redundant syncs), and connection.
-  bool ShouldRunJob(const SyncSessionJob& job);
+  // operational mode, backoff or throttling and connection. If the job is
+  // not fit to run at this time, this function returns false, and the caller
+  // should not proceed to run job. If that happens (the function returns
+  // false), the caller can rest assured that the job was rescheduled for
+  // later if it was deemed important, so the caller is not responsible.
+  bool ShouldRunJobSaveIfNecessary(SyncSessionJob* job);
 
   // Decide whether we should CONTINUE, SAVE or DROP the job.
-  JobProcessDecision DecideOnJob(const SyncSessionJob& job);
+  JobProcessDecision DecideOnJob(const SyncSessionJob* job);
 
   // Decide on whether to CONTINUE, SAVE or DROP the job when we are in
   // backoff mode.
-  JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job);
-
-  // Saves the job for future execution. Note: It drops all the poll jobs.
-  void SaveJob(const SyncSessionJob& job);
-
-  // Coalesces the current job with the pending nudge.
-  void InitOrCoalescePendingJob(const SyncSessionJob& job);
+  JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob* job);
 
   // 'Impl' here refers to real implementation of public functions, running on
   // |thread_|.
@@ -243,7 +203,7 @@ class SyncSchedulerImpl : public SyncScheduler {
       const base::TimeDelta& delay,
       sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source,
       const ModelTypeStateMap& type_state_map,
-      bool is_canary_job, const tracked_objects::Location& nudge_location);
+      const tracked_objects::Location& nudge_location);
 
   // Returns true if the client is currently in exponential backoff.
   bool IsBackingOff() const;
@@ -251,20 +211,28 @@ class SyncSchedulerImpl : public SyncScheduler {
   // Helper to signal all listeners registered with |session_context_|.
   void Notify(SyncEngineEvent::EventCause cause);
 
-  // Callback to change backoff state.
-  void DoCanaryJob();
-  void Unthrottle();
-
-  // Executes the pending job. Called whenever an event occurs that may
-  // change conditions permitting a job to run. Like when network connection is
-  // re-established, mode changes etc.
-  void DoPendingJobIfPossible(bool is_canary_job);
+  // 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();
 
   // Called when the root cause of the current connection error is fixed.
   void OnServerConnectionErrorFixed();
 
   // The pointer is owned by the caller.

[rlarocque, 2012/09/24 21:15:14]

I think this comment is no longer necessary.

[tim (not reviewing), 2012/10/08 00:20:03]

Done.

-  sessions::SyncSession* CreateSyncSession(
+  scoped_ptr<sessions::SyncSession> CreateSyncSession(
       const sessions::SyncSourceInfo& info);
 
   // Creates a session for a poll and performs the sync.
@@ -323,8 +291,18 @@ class SyncSchedulerImpl : public SyncScheduler {
   // The latest connection code we got while trying to connect.
   HttpResponse::ServerConnectionCode connection_code_;
 
-  // Tracks in-flight nudges so we can coalesce.
-  scoped_ptr<SyncSessionJob> pending_nudge_;
+  // 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_;

[rlarocque, 2012/09/24 22:01:46]

I didn't see anywhere in this patch where we read from this member, other than
in ShouldRunJobSaveIfNecessary, where we first check that it's NULL then
overwrite it on the line before we read it.

[tim (not reviewing), 2012/10/08 00:20:03]

Right, as I said in the comment, we should not read from it.  Jobs become owned
by the fact that they're scheduled.  In rare some cases, we can't schedule a job
because there's no known point in time when we know we should try running it
again (it may be dependent on the user entering credentials, for example).  So
this just stores the job. 

 
   // Current wait state.  Null if we're not in backoff and not throttled.
   scoped_ptr<WaitInterval> wait_interval_;