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..632eb147dadb1dac7f445d5fe2946d86cd18fac0 100644
--- a/chrome/browser/sync/engine/syncer_thread2.h
+++ b/chrome/browser/sync/engine/syncer_thread2.h
@@ -43,6 +43,76 @@ class SyncerThread : public sessions::SyncSession::Delegate,
     NORMAL_MODE,
   };
 
+  enum JobProcessDecision {

[tim (not reviewing), 2011/04/13 21:40:18]

This should not be public.  Only things that the public API (which is just
start/stop/schedule*) can do should be public.

Also, comment above the enum

[lipalani1, 2011/04/13 22:45:14]

yep they were for UTs. Now I made all UTs friends.
On 2011/04/13 21:40:18, timsteele wrote:

+    // 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,
+  };
+
+  struct SyncSessionJob {

[tim (not reviewing), 2011/04/13 21:40:18]

this should not be public.  Was this for tests?

[lipalani1, 2011/04/13 22:45:14]

Done.

+    // 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,
+    };
+
+    SyncSessionJob();
+    SyncSessionJob(SyncSessionJobPurpose purpose, base::TimeTicks start,
+        linked_ptr<sessions::SyncSession> session, bool is_canary_job,
+        const tracked_objects::Location& nudge_location);
+    ~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 {

[tim (not reviewing), 2011/04/13 21:40:18]

this should not be public.

[lipalani1, 2011/04/13 22:45:14]

Same reason as above. Moved to private and made all uts friends.
On 2011/04/13 21:40:18, timsteele wrote:

+    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,
+    };
+    WaitInterval();
+    ~WaitInterval();
+
+    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 +168,12 @@ class SyncerThread : public sessions::SyncSession::Delegate,
   // TODO(tim): schedule a nudge when valid connection detected? in 1 minute?
   virtual void OnServerConnectionEvent(const ServerConnectionEvent2& event);
 
+

[tim (not reviewing), 2011/04/13 21:40:18]

remove newline

[lipalani1, 2011/04/13 22:45:14]

Done.

  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 class SyncerThread2WhiteboxTest;
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest,
+      DropNudgeWhileExponentialBackOff);
 
   // A component used to get time delays associated with exponential backoff.
   // Encapsulated into a class to facilitate testing.
@@ -137,7 +188,7 @@ class SyncerThread : public sessions::SyncSession::Delegate,
 
   // Helper to assemble a job and post a delayed task to sync.
   void ScheduleSyncSessionJob(const base::TimeDelta& delay,
-                              SyncSessionJobPurpose purpose,
+    SyncSessionJob::SyncSessionJobPurpose purpose,
                               sessions::SyncSession* session,
                               const tracked_objects::Location& nudge_location);
 
@@ -161,22 +212,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|.

[tim (not reviewing), 2011/04/13 21:40:18]

"to run |job| by checking current operational mode, ...."

[lipalani1, 2011/04/13 22:45:14]

Done.

+  // 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);
+
+  // Decide whether we should run, save or discard the job.

[tim (not reviewing), 2011/04/13 21:40:18]

You should use consistent terminology here with the enum.
Either both should be RUN SAVE DISCARD
or both should say CONTINUE SAVE DROP

[lipalani1, 2011/04/13 22:45:14]

Done.

+  JobProcessDecision DecideOnJob(const SyncSessionJob& job);
+
+  // Decide on whether to run, save or discard the job when we are in
+  // back off mode.

[tim (not reviewing), 2011/04/13 21:40:18]

nit - 'backoff' is one word, please check the new code for that.

[lipalani1, 2011/04/13 22:45:14]

Done.

+  JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job);
+
+  // Saves the job for future executio.

[tim (not reviewing), 2011/04/13 21:40:18]

execution. typo

[lipalani1, 2011/04/13 22:45:14]

Done.

+  void SaveJob(const SyncSessionJob& job);
+
+  // Coalesces the current job with the pending nudge.
+  void InitOrCoalesceJob(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,12 +253,20 @@ class SyncerThread : public sessions::SyncSession::Delegate,
   void DoCanaryJob();
   void Unthrottle();
 
+  void DoPendingJobIfPossible(bool is_canary_job);

[tim (not reviewing), 2011/04/13 21:40:18]

comment

[lipalani1, 2011/04/13 22:45:14]

Done.

+
+  // The pointer is owned by the caller.
+  browser_sync::sessions::SyncSession* CreateSyncSession(
+    const browser_sync::sessions::SyncSourceInfo& info);

[tim (not reviewing), 2011/04/13 21:40:18]

indent

[lipalani1, 2011/04/13 22:45:14]

Done.

+
+  void ExecuteJobByMakingACopy(SyncSessionJob* job);

[tim (not reviewing), 2011/04/13 21:40:18]

remove

[lipalani1, 2011/04/13 22:45:14]

Done.

+
   // Creates a session for a poll and performs the sync.
   void PollTimerCallback();
 
   // Assign |start| and |end| to appropriate SyncerStep values for the
   // specified |purpose|.
-  void SetSyncerStepsForPurpose(SyncSessionJobPurpose purpose,
+  void SetSyncerStepsForPurpose(SyncSessionJob::SyncSessionJobPurpose purpose,
                                 SyncerStep* start,
                                 SyncerStep* end);