sync: Make nudge + config jobs reliable in SyncerThread2

chrome/browser/sync/engine/syncer_thread2.h

Index: chrome/browser/sync/engine/syncer_thread2.h
diff --git a/chrome/browser/sync/engine/syncer_thread2.h b/chrome/browser/sync/engine/syncer_thread2.h
index 3fbdd7f26d437f92a98009ef33933bf9ef1050a4..a078bb20ffe34f65755ef501988ed05f6b721bdf 100644
--- a/chrome/browser/sync/engine/syncer_thread2.h
+++ b/chrome/browser/sync/engine/syncer_thread2.h
@@ -43,6 +43,68 @@ class SyncerThread : public sessions::SyncSession::Delegate,
     NORMAL_MODE,
   };
 
+  enum JobProcessDecision {
+    // Indicates we should continue with the current job.
+    CONTINUE,
+    // Indicates that we should save it to be processed later.
+    SAVE,
+    // Indicates we should drop this job.
+    DROP,
+  };
+
+  // An enum used to describe jobs for scheduling purposes.
+  enum SyncSessionJobPurpose {

[tim (not reviewing), 2011/04/09 23:52:58]

This was originally in the header file, and was called SyncSessionJob::Purpose. 
It was then pulled out of the struct only so that the struct could be moved to
the cc file for clang (http://codereview.chromium.org/6272025), but now you've
brought that back.  I'll let you look at that review and figure out what to do,
but if they are both to be in the header file, this should be declared inside
SyncSessionJob as 'Purpose'.

[lipalani1, 2011/04/12 02:33:00]

Done.

+    // 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,
+    // The user invoked a function in the UI to clear their entire account
+    // and stop syncing (globally).
+    CLEAR_USER_DATA,
+    // Typically used for fetching updates for a subset of the enabled types
+    // during initial sync or reconfiguration.  We don't run all steps of
+    // the sync cycle for these (e.g. CleanupDisabledTypes is skipped).
+    CONFIGURATION,
+  };
+
+  struct SyncSessionJob {
+    SyncSessionJobPurpose purpose;
+    base::TimeTicks scheduled_start;
+    linked_ptr<sessions::SyncSession> session;
+    bool is_canary_job;
+
+    // This is the location the nudge came from. used for debugging purpose.
+    // In case of multiple nudges getting coalesced this stores the first nudge
+    // that came in.
+    tracked_objects::Location nudge_location;
+  };
+
+  struct WaitInterval {
+    enum Mode {
+      // A wait interval whose duration has been affected by exponential
+      // backoff.
+      // EXPONENTIAL_BACKOFF intervals are nudge-rate limited to 1 per interval.
+      EXPONENTIAL_BACKOFF,
+      // A server-initiated throttled interval.  We do not allow any syncing
+      // during such an interval.
+      THROTTLED,
+    };
+    Mode mode;
+
+    // This bool is set to true if we have observed a nudge during this
+    // interval and mode == EXPONENTIAL_BACKOFF.
+    bool had_nudge;
+    base::TimeDelta length;
+    base::OneShotTimer<SyncerThread> timer;
+
+    // Configure jobs are saved only when backing off or throttling. So we
+    // expose the pointer here.
+    scoped_ptr<SyncSessionJob> pending_configure_job;
+    WaitInterval(Mode mode, base::TimeDelta length);
+  };
+
   // Takes ownership of both |context| and |syncer|.
   SyncerThread(sessions::SyncSessionContext* context, Syncer* syncer);
   virtual ~SyncerThread();
@@ -98,31 +160,10 @@ class SyncerThread : public sessions::SyncSession::Delegate,
   // TODO(tim): schedule a nudge when valid connection detected? in 1 minute?
   virtual void OnServerConnectionEvent(const ServerConnectionEvent2& event);
 
+
  private:
   friend class SyncerThread2Test;
-
-  // State pertaining to exponential backoff or throttling periods.
-  struct WaitInterval;
-
-  // An enum used to describe jobs for scheduling purposes.
-  enum SyncSessionJobPurpose {
-    // 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,
-    // The user invoked a function in the UI to clear their entire account
-    // and stop syncing (globally).
-    CLEAR_USER_DATA,
-    // Typically used for fetching updates for a subset of the enabled types
-    // during initial sync or reconfiguration.  We don't run all steps of
-    // the sync cycle for these (e.g. CleanupDisabledTypes is skipped).
-    CONFIGURATION,
-  };
-
-  // Internal state for every sync task that is scheduled.
-  struct SyncSessionJob;
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2Test, DropNudgeWhileExponentialBackOff);
 
   // A component used to get time delays associated with exponential backoff.
   // Encapsulated into a class to facilitate testing.
@@ -161,22 +202,35 @@ class SyncerThread : public sessions::SyncSession::Delegate,
   // Helper to ScheduleNextSync in case of consecutive sync errors.
   void HandleConsecutiveContinuationError(const SyncSessionJob& old_job);
 
-  // Determines if it is legal to run a sync job for |purpose| at
-  // |scheduled_start|.  This checks current operational mode, backoff or
+  // Determines if it is legal to run a sync |job|.
+  // This checks current operational mode, backoff or
   // throttling, freshness (so we don't make redundant syncs), and connection.
-  bool ShouldRunJob(SyncSessionJobPurpose purpose,
-                    const base::TimeTicks& scheduled_start);
+  bool ShouldRunJob(const SyncSessionJob& job);
+
+  // Saves the job for future executio.
+  void SaveJob(const SyncSessionJob& job);
+
+  // Decide on whether to run, save or discard the job when we are in
+  // back off mode.
+  JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job);

[tim (not reviewing), 2011/04/09 23:52:58]

the functions in this file are roughly ordered in dependency order, e.g. if
DecideOnJob calls DecideWhileInWaitInterval, please order accordingly.

[lipalani1, 2011/04/12 02:33:00]

Done.

+
+  // Decide whether we should run, save or discard the job.
+  JobProcessDecision DecideOnJob(const SyncSessionJob& job);
+
+  // Coalesces the current job with the pending nudge.
+  void UpdatePendingJob(const SyncSessionJob& job);
 
   // 'Impl' here refers to real implementation of public functions, running on
   // |thread_|.
   void StartImpl(Mode mode, linked_ptr<ModeChangeCallback> callback);
   void ScheduleNudgeImpl(
       const base::TimeDelta& delay,
-      NudgeSource source,
+      sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source,
       const syncable::ModelTypePayloadMap& types_with_payloads,
-      const tracked_objects::Location& nudge_location);
+      bool is_canary_job, const tracked_objects::Location& nudge_location);
   void ScheduleConfigImpl(const ModelSafeRoutingInfo& routing_info,
-                          const std::vector<ModelSafeWorker*>& workers);
+      const std::vector<ModelSafeWorker*>& workers,
+      const sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source);
   void ScheduleClearUserDataImpl();
 
   // Returns true if the client is currently in exponential backoff.
@@ -189,6 +243,10 @@ class SyncerThread : public sessions::SyncSession::Delegate,
   void DoCanaryJob();
   void Unthrottle();
 
+  void ExecutePendingJob();

[tim (not reviewing), 2011/04/09 23:52:58]

How is execute different from 'DoSyncSessionJob'.

[lipalani1, 2011/04/12 02:33:00]

Done.

+
+  void ExecuteJobByMakingACopy(SyncSessionJob* job);

[tim (not reviewing), 2011/04/09 23:52:58]

These two functions together are confusing, especially when there's a
DoSyncSessionJob.  I think there should only be two functions:

DoSyncSessionJob(job)
and 
DoPendingJobIfPossible()

the 'ByMakingACopy' variant I think adds more questions than it's worth; i'd
inline it.  You could probably do

SyncSessionJob job;
job.is_canary = true;

if CONFIG
  ...
else
  ...

instead, in DoPendingJob.

[lipalani1, 2011/04/12 02:33:00]

Done.

+
   // Creates a session for a poll and performs the sync.
   void PollTimerCallback();