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..0aa0331dcffa8ff3626c2df6ec28a838a0307e4f 100644
--- a/chrome/browser/sync/engine/syncer_thread2.h
+++ b/chrome/browser/sync/engine/syncer_thread2.h
@@ -99,30 +99,69 @@ class SyncerThread : public sessions::SyncSession::Delegate,
   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,
+  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,
   };
 
-  // Internal state for every sync task that is scheduled.
-  struct SyncSessionJob;
+  struct SyncSessionJob {
+    // 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;
+  };
+  friend class SyncerThread2Test;
+  friend class SyncerThread2WhiteboxTest;
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest,
+      DropNudgeWhileExponentialBackOff);
+

[tim (not reviewing), 2011/04/13 23:31:54]

extra newline

but also, are you sure it's not possible to change the test around to use the
test class versus each having to be made a friend?

[lipalani1, 2011/04/14 00:51:07]

I am assuming you mean to use TEST() rather than TEST_F but then I wont be able
to get the fixtures.
On 2011/04/13 23:31:54, timsteele wrote:

[tim (not reviewing), 2011/04/14 23:09:44]

I meant adding methods to the class like

SetFoo() instead of having the individual tests do syncer_thread_->foo_ =

so that we don't need alllllll these friend_tests 

+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest, SaveNudge);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest, ContinueNudge);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest, DropPoll);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest, ContinuePoll);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest, ContinueConfiguration);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest,
+                           SaveConfigurationWhileThrottled);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest,
+                           SaveNudgeWhileThrottled);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest,
+                           ContinueClearUserDataUnderAllCircumstances);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest,
+                           DropNudgeWhileExponentialBackOff);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest,
+                           ContinueCanaryJobConfig);
+  FRIEND_TEST_ALL_PREFIXES(SyncerThread2WhiteboxTest,
+      ContinueNudgeWhileExponentialBackOff);
 
   // A component used to get time delays associated with exponential backoff.
   // Encapsulated into a class to facilitate testing.
@@ -135,9 +174,36 @@ class SyncerThread : public sessions::SyncSession::Delegate,
     DISALLOW_COPY_AND_ASSIGN(DelayProvider);
   };
 
+  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,
+    };
+    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);
+  };
+
   // Helper to assemble a job and post a delayed task to sync.
   void ScheduleSyncSessionJob(const base::TimeDelta& delay,
-                              SyncSessionJobPurpose purpose,
+    SyncSessionJob::SyncSessionJobPurpose purpose,

[tim (not reviewing), 2011/04/13 23:31:54]

two more spaces indent, which means the |delay| param should be brought down to
line up, and the two params below should be dragged left to line up.

[lipalani1, 2011/04/14 00:51:07]

Done.

                               sessions::SyncSession* session,
                               const tracked_objects::Location& nudge_location);
 
@@ -161,22 +227,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 |job|.

[tim (not reviewing), 2011/04/13 23:31:54]

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

or jus get rid of 'Determines if it is legal to run job.', because it doesn't
help much on its own.

[lipalani1, 2011/04/14 00:51:07]

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 continue, save or discard the job.

[tim (not reviewing), 2011/04/13 23:31:54]

swap discard for drop

[lipalani1, 2011/04/14 00:51:07]

Done.

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

[tim (not reviewing), 2011/04/13 23:31:54]

CONTINUE, SAVE, DROP

[lipalani1, 2011/04/14 00:51:07]

Done.

+  // 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);
 
   // '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 +268,20 @@ class SyncerThread : public sessions::SyncSession::Delegate,
   void DoCanaryJob();
   void Unthrottle();
 
+  // Executes jobs that are pending. Called whenever a critical event

[tim (not reviewing), 2011/04/13 23:31:54]

There's only one pending job, so shouldn't be plural.
And rather than "critical event", I'd say "Called whenever an event occurs that
may change conditions permitting a job to run (i.e. network connectivity, mode
changes, etc)."

[lipalani1, 2011/04/14 00:51:07]

Done.

+  // happens. Like when network connection is established, mode changes etc.
+  void DoPendingJobIfPossible(bool is_canary_job);
+
+  // The pointer is owned by the caller.
+  browser_sync::sessions::SyncSession* CreateSyncSession(
+      const browser_sync::sessions::SyncSourceInfo& info);
+
   // 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);