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 {

[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();
 
   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);
 
+

[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.
   class DelayProvider {
    public:
     DelayProvider();
     virtual base::TimeDelta GetDelay(const base::TimeDelta& last_delay);
     virtual ~DelayProvider();
    private:
     DISALLOW_COPY_AND_ASSIGN(DelayProvider);
   };
 
   // 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);
 
   // Invoke the Syncer to perform a sync.
   void DoSyncSessionJob(const SyncSessionJob& job);
 
   // Called after the Syncer has performed the sync represented by |job|, to
   // reset our state.
   void FinishSyncSessionJob(const SyncSessionJob& job);
 
   // Record important state that might be needed in future syncs, such as which
   // data types may require cleanup.
   void UpdateCarryoverSessionState(const SyncSessionJob& old_job);
 
   // 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|.

[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.
   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 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);
 
   // Initializes the hookup between the ServerConnectionManager and us.
   void WatchConnectionManager();
 
   // Used to update |server_connection_ok_|, see below.
   void CheckServerConnectionManagerStatus(
       HttpResponse::ServerConnectionCode code);
 
@@ skipping 40 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_