sync: Make nudge + config jobs reliable in SyncerThread2

chrome/browser/sync/engine/syncer_thread2.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // A class to run the syncer on a thread.
 #ifndef CHROME_BROWSER_SYNC_ENGINE_SYNCER_THREAD2_H_
 #define CHROME_BROWSER_SYNC_ENGINE_SYNCER_THREAD2_H_
 #pragma once
 
 #include "base/callback.h"
@@ skipping 25 matching lines @@
     // In this mode, the thread only performs configuration tasks.  This is
     // designed to make the case where we want to download updates for a
     // specific type only, and not continue syncing until we are moved into
     // normal mode.
     CONFIGURATION_MODE,
     // Resumes polling and allows nudges, drops configuration tasks.  Runs
     // through entire sync cycle.
     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();
 
   typedef Callback0::Type ModeChangeCallback;
 
   // Change the mode of operation.
   // We don't use a lock when changing modes, so we won't cause currently
   // scheduled jobs to adhere to the new mode.  We could protect it, but it
   // doesn't buy very much as a) a session could already be in progress and it
@@ skipping 35 matching lines @@
   virtual void OnReceivedShortPollIntervalUpdate(
       const base::TimeDelta& new_interval);
   virtual void OnReceivedLongPollIntervalUpdate(
       const base::TimeDelta& new_interval);
   virtual void OnShouldStopSyncingPermanently();
 
   // ServerConnectionEventListener implementation.
   // 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.
   class DelayProvider {
    public:
     DelayProvider();
     virtual base::TimeDelta GetDelay(const base::TimeDelta& last_delay);
     virtual ~DelayProvider();
    private:
     DISALLOW_COPY_AND_ASSIGN(DelayProvider);
@@ skipping 18 matching lines @@
 
   // Helper to FinishSyncSessionJob to schedule the next sync operation.
   void ScheduleNextSync(const SyncSessionJob& old_job);
 
   // Helper to configure polling intervals. Used by Start and ScheduleNextSync.
   void AdjustPolling(const SyncSessionJob* old_job);
 
   // 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.
   bool IsBackingOff() const;
 
   // Helper to signal all listeners registered with |session_context_|.
   void Notify(SyncEngineEvent::EventCause cause);
 
   // Callback to change backoff state.
   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();
 
   // Assign |start| and |end| to appropriate SyncerStep values for the
   // specified |purpose|.
   void SetSyncerStepsForPurpose(SyncSessionJobPurpose purpose,
                                 SyncerStep* start,
                                 SyncerStep* end);
 
   // Initializes the hookup between the ServerConnectionManager and us.
@@ skipping 46 matching lines @@
 
 }  // namespace s3
 
 }  // namespace browser_sync
 
 // The SyncerThread manages its own internal thread and thus outlives it. We
 // don't need refcounting for posting tasks to this internal thread.
 DISABLE_RUNNABLE_METHOD_REFCOUNT(browser_sync::s3::SyncerThread);
 
 #endif  // CHROME_BROWSER_SYNC_ENGINE_SYNCER_THREAD2_H_