sync: Refactor job ownership in SyncScheduler

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>
 
@@ skipping 132 matching lines @@
 
     static const char* GetModeString(Mode mode);
 
     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<SyncSchedulerImpl> timer;
-
-    // Configure jobs are saved only when backing off or throttling. So we
-    // expose the pointer here (does not own, similar to pending_nudge).
-    SyncSessionJob* pending_configure_job;
   };
 
   static const char* GetModeString(Mode mode);
 
   static const char* GetDecisionString(JobProcessDecision decision);
 
   // Helper to cancel any existing delayed task and replace it with a new one.
   // It will not post any tasks if the scheduler is in a "stopped" state.
   void PostDelayedTask(const tracked_objects::Location& from_here,
                        const char* name,
                        const base::Closure& task,
                        base::TimeDelta delay);
 
-  // Invoke the Syncer to perform a non-poll job.
-  bool DoSyncSessionJob(scoped_ptr<SyncSessionJob> job,
-                        JobPriority priority);
+  // 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.
+  bool 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(scoped_ptr<SyncSessionJob> 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);
 
+  // Resumes an existing wait interval, taking into account the time already
+  // spent waiting.  Calling this funciton on a wait interval that is still
+  // active will have not effect, give or take a few small timing side-effects.
+  void ResumeWaiting();
+
   // Helper to restart waiting with |wait_interval_|'s timer.
-  void RestartWaiting(scoped_ptr<SyncSessionJob> job);
+  void RestartWaiting();
 
   // Helper to ScheduleNextSync in case of consecutive sync errors.
   void HandleContinuationError(scoped_ptr<SyncSessionJob> old_job,
                                sessions::SyncSession* session);
 
   // Decide whether we should CONTINUE, SAVE or DROP the job.
   JobProcessDecision DecideOnJob(const SyncSessionJob& job,
                                  JobPriority priority);
 
-  // If DecideOnJob decides that |job| should be SAVEd, this function will
-  // carry out the task of actually "saving" (or coalescing) the job.
-  void HandleSaveJobDecision(scoped_ptr<SyncSessionJob> job);
-
   // Decide on whether to CONTINUE, SAVE or DROP the job when we are in
   // backoff mode.
   JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job,
                                                JobPriority priority);
 
   // 'Impl' here refers to real implementation of public functions, running on
   // |thread_|.
   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,
       const tracked_objects::Location& nudge_location);
 
   // 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);
 
   // Helper to signal listeners about changed retry time
   void NotifyRetryTime(base::Time retry_time);
 
-  // Callback to change backoff state. |to_be_canary| in both cases is the job
-  // that should be granted canary privileges. Note: it is possible that the
-  // job that gets scheduled when this callback is scheduled is different from
-  // the job that will actually get executed, because other jobs may have been
-  // scheduled while we were waiting for the callback.
-  void DoCanaryJob(scoped_ptr<SyncSessionJob> to_be_canary);
-  void Unthrottle(scoped_ptr<SyncSessionJob> to_be_canary);
+  // Looks for pending work and, if it finds any, run this work at "canary"
+  // priority.
+  void TryCanaryJob();
 
-  // Returns a pending job that has potential to run given the state of the
-  // scheduler, if it exists. Useful whenever an event occurs that may
-  // change conditions that permit a job to run, such as re-establishing
-  // network connection, auth refresh, mode changes etc. Note that the returned
-  // job may have been scheduled to run at a later time, or may have been
-  // unscheduled.  In the former case, this will result in abandoning the old
-  // job and effectively cancelling it.
-  scoped_ptr<SyncSessionJob> TakePendingJobForCurrentMode();
+  // 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.
@@ skipping 33 matching lines @@
 
   // Server-tweakable sessions commit delay.
   base::TimeDelta sessions_commit_delay_;
 
   // Periodic timer for polling.  See AdjustPolling.
   base::RepeatingTimer<SyncSchedulerImpl> poll_timer_;
 
   // The mode of operation.
   Mode mode_;
 
-  // Tracks (does not own) in-flight nudges (scheduled or unscheduled),
-  // so we can coalesce. NULL if there is no pending nudge.
-  SyncSessionJob* pending_nudge_;
-
-  // There are certain situations where we want to remember a nudge, but
-  // there is no well defined moment in time in the future when that nudge
-  // should run, e.g. if it requires a mode switch or updated auth credentials.
-  // This member will own NUDGE jobs in those cases, until an external event
-  // (mode switch or fixed auth) occurs to trigger a retry.  Should be treated
-  // as opaque / not interacted with (i.e. we could build a wrapper to
-  // hide the type, but that's probably overkill).
-  scoped_ptr<SyncSessionJob> unscheduled_nudge_storage_;
-
   // Current wait state.  Null if we're not in backoff and not throttled.
   scoped_ptr<WaitInterval> wait_interval_;
 
   scoped_ptr<BackoffDelayProvider> delay_provider_;
 
   // We allow at most one PostedTask to be pending at one time.  This is it.
   // We will cancel this task before starting a new one.
   base::CancelableClosure pending_wakeup_;
 
+  // Pending configure job storage.  Note that
+  // (mode_ != CONFIGURATION_MODE) \implies !pending_configure_job_.
+  scoped_ptr<SyncSessionJob> pending_configure_job_;

[tim (not reviewing), 2013/04/04 00:22:11]

Why does this belong here? Unless I'm mistaken this introduces a state that
should not logically exist. Can't we avoid that entropy? We can only have a
pending config job if it is part of the WaitInterval, and it'd be clearer if the
ownership model reflected it in some way.  Are you planning to address that
somehow?

[rlarocque, 2013/04/04 00:59:48]

I'm not that worried about it.  We have plenty of DCHECKs when heading into our
out of CONFIGURATION_MODE to catch misuse of this variable.  It's not that hard
to enforce our assumptions.  It will be even easier to enforce them when this
pending_configure_job_ is replaced with an instance of ConfigurationParams
(which is the only one of this SyncSessionJob's members that we really care
about).

I could make the same argument that WaitInterval shouldn't have a
pending_configure_job_ member when we're not in CONFIGURATION_MODE.  (That
member was removed in this patch, btw.)

If we really wanted to fix it, we'd need to use something like this:
http://en.wikipedia.org/wiki/State_pattern.  I'd rather not go down that road,
though.  That pattern makes it difficult to implement transitions between
states, and requires a lot of boilerplate code.  I think a few DCHECKs is the
least bad solution here.

[tim (not reviewing), 2013/04/04 18:07:30]

Ok. A small concern I have is as the need to add nudge or config or new-job
specific state arises it will become increasingly hard to reason about what
members are valid when; hopefully "the next guy" will carefully consider and
replicate the DCHECKs you've added for whatever members happen to be the only
config state today. A comment near member declaration, similar to what you've
said here pointing out what applies to what mode/scheduler state may help (maybe
when you get around to removing SyncSchedulerJob).

+
+  // 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_;
+
   // 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_