sync: Finish the SyncScheduler refactor

sync/engine/sync_scheduler_impl.h

 // Copyright 2012 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.
 
 #ifndef SYNC_ENGINE_SYNC_SCHEDULER_IMPL_H_
 #define SYNC_ENGINE_SYNC_SCHEDULER_IMPL_H_
 
 #include <map>
 #include <string>
 
 #include "base/callback.h"
 #include "base/cancelable_callback.h"
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time.h"
 #include "base/timer.h"
 #include "sync/base/sync_export.h"
 #include "sync/engine/net/server_connection_manager.h"
 #include "sync/engine/nudge_source.h"
+#include "sync/engine/nudge_tracker.h"
 #include "sync/engine/sync_scheduler.h"
-#include "sync/engine/sync_session_job.h"
 #include "sync/engine/syncer.h"
 #include "sync/internal_api/public/base/model_type_invalidation_map.h"
 #include "sync/internal_api/public/engine/polling_constants.h"
 #include "sync/internal_api/public/util/weak_handle.h"
 #include "sync/sessions/sync_session.h"
 #include "sync/sessions/sync_session_context.h"
 
 namespace syncer {
 
 class BackoffDelayProvider;
 
+namespace sessions {
+struct ModelNeutralState;
+}  // namespace sessions

[tim (not reviewing), 2013/04/15 22:37:49]

nit - don't need comment.

[rlarocque, 2013/04/16 01:30:26]

Done.

+
 class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
     : public SyncScheduler,
       public base::NonThreadSafe {
  public:
   // |name| is a display string to identify the syncer thread.  Takes
   // |ownership of |syncer| and |delay_provider|.
   SyncSchedulerImpl(const std::string& name,
                     BackoffDelayProvider* delay_provider,
                     sessions::SyncSessionContext* context,
                     Syncer* syncer);
@@ skipping 29 matching lines @@
       const base::TimeDelta& new_interval) OVERRIDE;
   virtual void OnReceivedLongPollIntervalUpdate(
       const base::TimeDelta& new_interval) OVERRIDE;
   virtual void OnReceivedSessionsCommitDelay(
       const base::TimeDelta& new_delay) OVERRIDE;
   virtual void OnShouldStopSyncingPermanently() OVERRIDE;
   virtual void OnSyncProtocolError(
       const sessions::SyncSessionSnapshot& snapshot) OVERRIDE;
 
  private:
-  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,
-  };
-
   enum JobPriority {
     // Non-canary jobs respect exponential backoff.
     NORMAL_PRIORITY,
     // Canary jobs bypass exponential backoff, so use with extreme caution.
     CANARY_PRIORITY
   };
 
   friend class SyncSchedulerTest;
   friend class SyncSchedulerWhiteboxTest;
   friend class SyncerTest;
 
+  FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest, NoNudgesInConfigureMode);
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest,
       DropNudgeWhileExponentialBackOff);
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest, SaveNudge);
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest,
                            SaveNudgeWhileTypeThrottled);
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest, ContinueNudge);
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest, ContinueConfiguration);
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest,
                            SaveConfigurationWhileThrottled);
   FRIEND_TEST_ALL_PREFIXES(SyncSchedulerWhiteboxTest,
@@ skipping 22 matching lines @@
     WaitInterval(Mode mode, base::TimeDelta length);
 
     static const char* GetModeString(Mode mode);
 
     Mode mode;
     base::TimeDelta length;
   };
 
   static const char* GetModeString(Mode mode);
 
-  static const char* GetDecisionString(JobProcessDecision decision);
-
-  // Invoke the syncer to perform a non-POLL job.
-  bool DoSyncSessionJobImpl(scoped_ptr<SyncSessionJob> job,
-                            JobPriority priority);
-
   // Invoke the syncer to perform a nudge job.
   void DoNudgeSyncSessionJob(JobPriority priority);
 
   // Invoke the syncer to perform a configuration job.
   bool DoConfigurationSyncSessionJob(JobPriority priority);
 
-  // Returns whether or not it's safe to run a poll job at this time.
-  bool ShouldPoll();
-
   // Invoke the Syncer to perform a poll job.
   void DoPollSyncSessionJob();
 
-  // Called after the Syncer has performed the sync represented by |job|, to
-  // reset our state.  |exited_prematurely| is true if the Syncer did not
-  // cycle from job.start_step() to job.end_step(), likely because the
-  // scheduler was forced to quit the job mid-way through.
-  bool FinishSyncSessionJob(SyncSessionJob* job,
-                            bool exited_prematurely,
-                            sessions::SyncSession* session);
-
-  // Helper to schedule retries of a failed configure or nudge job.
-  void ScheduleNextSync(scoped_ptr<SyncSessionJob> finished_job,
-                        sessions::SyncSession* session);
-
-  // Helper to configure polling intervals. Used by Start and ScheduleNextSync.
-  void AdjustPolling(const SyncSessionJob* old_job);
+  // Helper to configure polling intervals.
+  void AdjustPolling(bool force_reset);

[tim (not reviewing), 2013/04/15 22:37:49]

nit - enum vs bool parameter as the intent of the parameter isn't immediately
evident from the method name.

[rlarocque, 2013/04/16 01:30:26]

I'd prefer to not add another enum in that style.  Instead, I did what I should
have done in the first place.  I split this into two separate functions, and
factored out the common code.  

It would probably make even more sense to update the poll interval only when
Start() is called, or in OnReceived*PollIntervalUpdate(), or when we receive an
update to our notification status.  I'm not going to press my luck that far,
though.  I'd prefer to maintain the existing code structure.  

 
   // Helper to restart waiting with |wait_interval_|'s timer.
   void RestartWaiting();
 
-  // Helper to ScheduleNextSync in case of consecutive sync errors.
-  void HandleContinuationError(scoped_ptr<SyncSessionJob> old_job,
-                               sessions::SyncSession* session);
+  // Helper to adjust our wait interval when we expereince a transient failure.
+  void UpdateExponentialBackoff(
+      const sessions::ModelNeutralState& model_neutral_state);
 
-  // Decide whether we should CONTINUE, SAVE or DROP the job.
-  JobProcessDecision DecideOnJob(const SyncSessionJob& job,
-                                 JobPriority priority);
+  // Determines if we're allowed to contact the server right now.
+  bool CanRunJobNow(JobPriority priority);
 
-  // Decide on whether to CONTINUE, SAVE or DROP the job when we are in
-  // backoff mode.
-  JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job,
-                                               JobPriority priority);
+  // Determines if we're allowed to contact the server right now.
+  bool CanRunNudgeJobNow(JobPriority priority);
 
-  // 'Impl' here refers to real implementation of public functions, running on
-  // |thread_|.
+  // 'Impl' here refers to real implementation of public functions.
   void StopImpl(const base::Closure& callback);
 
   // If the scheduler's current state supports it, this will create a job based
   // on the passed in parameters and coalesce it with any other pending jobs,
   // then post a delayed task to run it.  It may also choose to drop the job or
   // save it for later, depending on the scheduler's current state.
   void ScheduleNudgeImpl(
       const base::TimeDelta& delay,
       sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source,
       const ModelTypeInvalidationMap& invalidation_map,
@@ skipping 14 matching lines @@
 
   // Transitions out of the THROTTLED WaitInterval then calls TryCanaryJob().
   void Unthrottle();
 
   // Called when the root cause of the current connection error is fixed.
   void OnServerConnectionErrorFixed();
 
   // Creates a session for a poll and performs the sync.
   void PollTimerCallback();
 
-  // Called once the first time thread_ is started to broadcast an initial
-  // session snapshot containing data like initial_sync_ended.  Important when
-  // the client starts up and does not need to perform an initial sync.
+  // Called as we are started to broadcast an initial session snapshot
+  // containing data like initial_sync_ended.  Important when the client starts
+  // up and does not need to perform an initial sync.
   void SendInitialSnapshot();
 
   // This is used for histogramming and analysis of ScheduleNudge* APIs.
   // SyncScheduler is the ultimate choke-point for all such invocations (with
   // and without InvalidationState variants, all NudgeSources, etc) and as such
   // is the most flexible place to do this bookkeeping.
   void UpdateNudgeTimeRecords(const sessions::SyncSourceInfo& info);
 
   virtual void OnActionableError(const sessions::SyncSessionSnapshot& snapshot);
 
@@ skipping 27 matching lines @@
   Mode mode_;
 
   // Current wait state.  Null if we're not in backoff and not throttled.
   scoped_ptr<WaitInterval> wait_interval_;
 
   scoped_ptr<BackoffDelayProvider> delay_provider_;
 
   // The event that will wake us up.
   base::OneShotTimer<SyncSchedulerImpl> pending_wakeup_timer_;
 
-  // Pending configure job storage.  Note that
-  // (mode_ != CONFIGURATION_MODE) \implies !pending_configure_job_.
-  scoped_ptr<SyncSessionJob> pending_configure_job_;
+  // Storage for variables related to an in-progress configure request.  Note
+  // that (mode_ != CONFIGURATION_MODE) \implies !pending_configure_params_.
+  scoped_ptr<ConfigurationParams> pending_configure_params_;
 
-  // Pending nudge job storage.  These jobs can exist in CONFIGURATION_MODE, but
-  // they will be run only in NORMAL_MODE.
-  scoped_ptr<SyncSessionJob> pending_nudge_job_;
+  // If we have a nudge pending to run soon, it will be listed here.
+  base::TimeTicks scheduled_nudge_time_;
+
+  // Keeps track of work that the syncer needs to handle.
+  NudgeTracker nudge_tracker_;
 
   // Invoked to run through the sync cycle.
   scoped_ptr<Syncer> syncer_;
 
   sessions::SyncSessionContext* session_context_;
 
   // A map tracking LOCAL NudgeSource invocations of ScheduleNudge* APIs,
   // organized by datatype. Each datatype that was part of the types requested
   // in the call will have its TimeTicks value updated.
   typedef std::map<ModelType, base::TimeTicks> ModelTypeTimeMap;
   ModelTypeTimeMap last_local_nudges_by_model_type_;
 
   // Used as an "anti-reentrancy defensive assertion".
   // While true, it is illegal for any new scheduling activity to take place.
   // Ensures that higher layers don't break this law in response to events that
   // take place during a sync cycle. We call this out because such violations
   // could result in tight sync loops hitting sync servers.
   bool no_scheduling_allowed_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncSchedulerImpl);
 };
 
 }  // namespace syncer
 
 #endif  // SYNC_ENGINE_SYNC_SCHEDULER_IMPL_H_