sync: Finish the SyncScheduler refactor

sync/engine/sync_scheduler_impl.cc

 // Copyright (c) 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.
 
 #include "sync/engine/sync_scheduler_impl.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/auto_reset.h"
@@ skipping 207 matching lines @@
             << thread_name << " with mode " << GetModeString(mode);
   if (!started_) {
     started_ = true;
     SendInitialSnapshot();
   }
 
   DCHECK(!session_context_->account_name().empty());
   DCHECK(syncer_.get());
   Mode old_mode = mode_;
   mode_ = mode;
-  AdjustPolling(NULL);  // Will kick start poll timer if needed.
+  AdjustPolling(false);  // Will kick start poll timer if needed.
 
-  if (old_mode != mode_ && mode_ == NORMAL_MODE && pending_nudge_job_) {
+  if (old_mode != mode_ && mode_ == NORMAL_MODE && !nudge_tracker_.IsEmpty()) {
     // We just got back to normal mode.  Let's try to run the work that was
     // queued up while we were configuring.
     DoNudgeSyncSessionJob(NORMAL_PRIORITY);
   }
 }
 
 void SyncSchedulerImpl::SendInitialSnapshot() {
   DCHECK(CalledOnValidThread());
   scoped_ptr<SyncSession> dummy(new SyncSession(
           session_context_, this, SyncSourceInfo()));
@@ skipping 26 matching lines @@
     const ConfigurationParams& params) {
   DCHECK(CalledOnValidThread());
   DCHECK(IsConfigRelatedUpdateSourceValue(params.source));
   DCHECK_EQ(CONFIGURATION_MODE, mode_);
   DCHECK(!params.ready_task.is_null());
   CHECK(started_) << "Scheduler must be running to configure.";
   SDVLOG(2) << "Reconfiguring syncer.";
 
   // Only one configuration is allowed at a time. Verify we're not waiting
   // for a pending configure job.
-  DCHECK(!pending_configure_job_);
+  DCHECK(!pending_configure_params_);
 
   ModelSafeRoutingInfo restricted_routes;
   BuildModelSafeParams(params.types_to_download,
                        params.routing_info,
                        &restricted_routes);
   session_context_->set_routing_info(restricted_routes);
 
   // Only reconfigure if we have types to download.
   if (!params.types_to_download.Empty()) {
-    DCHECK(!restricted_routes.empty());
-    pending_configure_job_.reset(new SyncSessionJob(
-        SyncSessionJob::CONFIGURATION,
-        TimeTicks::Now(),
-        SyncSourceInfo(params.source,
-                       ModelSafeRoutingInfoToInvalidationMap(
-                           restricted_routes,
-                           std::string())),
-        params));
+    pending_configure_params_.reset(new ConfigurationParams(params));
     bool succeeded = DoConfigurationSyncSessionJob(NORMAL_PRIORITY);
 
     // If we failed, the job would have been saved as the pending configure
     // job and a wait interval would have been set.
     if (!succeeded) {
-      DCHECK(pending_configure_job_);
-      return false;
+      DCHECK(pending_configure_params_);
     } else {
-      DCHECK(!pending_configure_job_);
+      DCHECK(!pending_configure_params_);
     }
+    return succeeded;
   } else {
     SDVLOG(2) << "No change in routing info, calling ready task directly.";
     params.ready_task.Run();
   }
 
   return true;
 }
 
-SyncSchedulerImpl::JobProcessDecision
-SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob& job,
-                                             JobPriority priority) {
+bool SyncSchedulerImpl::CanRunJobNow(JobPriority priority) {
   DCHECK(CalledOnValidThread());
-  DCHECK(wait_interval_.get());
-  DCHECK_NE(job.purpose(), SyncSessionJob::POLL);
+  if (wait_interval_ && wait_interval_->mode == WaitInterval::THROTTLED) {
+    SDVLOG(1) << "Unable to run a job because we're throttled.";
+    return false;
+  }
 
-  SDVLOG(2) << "DecideWhileInWaitInterval with WaitInterval mode "
-            << WaitInterval::GetModeString(wait_interval_->mode)
-            << ((priority == CANARY_PRIORITY) ? " (canary)" : "");
+  if (wait_interval_
+      && wait_interval_->mode == WaitInterval::EXPONENTIAL_BACKOFF
+      && priority != CANARY_PRIORITY) {
+    SDVLOG(1) << "Unable to run a job because we're backing off.";
+    return false;
+  }
 
-  // If we save a job while in a WaitInterval, there is a well-defined moment
-  // in time in the future when it makes sense for that SAVE-worthy job to try
-  // running again -- the end of the WaitInterval.
-  DCHECK(job.purpose() == SyncSessionJob::NUDGE ||
-         job.purpose() == SyncSessionJob::CONFIGURATION);
+  if (session_context_->connection_manager()->HasInvalidAuthToken()) {
+    SDVLOG(1) << "Unable to run a job because we have no valid auth token.";
+    return false;
+  }
 
-  // If throttled, there's a clock ticking to unthrottle. We want to get
-  // on the same train.
-  if (wait_interval_->mode == WaitInterval::THROTTLED)
-    return SAVE;
-
-  DCHECK_EQ(wait_interval_->mode, WaitInterval::EXPONENTIAL_BACKOFF);
-  if (job.purpose() == SyncSessionJob::NUDGE) {
-    if (mode_ == CONFIGURATION_MODE)
-      return SAVE;
-
-    if (priority == NORMAL_PRIORITY)
-      return DROP;
-    else // Either backoff has ended, or we have permission to bypass it.
-      return CONTINUE;
-  }
-  return (priority == CANARY_PRIORITY) ? CONTINUE : SAVE;
+  return true;
 }
 
-SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
-    const SyncSessionJob& job,
-    JobPriority priority) {
+bool SyncSchedulerImpl::CanRunNudgeJobNow(JobPriority priority) {
   DCHECK(CalledOnValidThread());
 
-  // POLL jobs do not call this function.
-  DCHECK(job.purpose() == SyncSessionJob::NUDGE ||
-         job.purpose() == SyncSessionJob::CONFIGURATION);
+  if (!CanRunJobNow(priority)) {
+    SDVLOG(1) << "Unable to run a nudge job right now";
+    return false;
+  }
 
-  // See if our type is throttled.
+  // If all types are throttled, do not continue.  Today, we don't treat a
+  // per-datatype "unthrottle" event as something that should force a canary
+  // job. For this reason, there's no good time to reschedule this job to run
+  // -- we'll lazily wait for an independent event to trigger a sync.
   ModelTypeSet throttled_types =
       session_context_->throttled_data_type_tracker()->GetThrottledTypes();
-  if (job.purpose() == SyncSessionJob::NUDGE &&
-      job.source_info().updates_source == GetUpdatesCallerInfo::LOCAL) {
-    ModelTypeSet requested_types;
-    for (ModelTypeInvalidationMap::const_iterator i =
-         job.source_info().types.begin(); i != job.source_info().types.end();
-         ++i) {
-      requested_types.Put(i->first);
-    }
-
-    // If all types are throttled, do not CONTINUE.  Today, we don't treat
-    // a per-datatype "unthrottle" event as something that should force a
-    // canary job. For this reason, there's no good time to reschedule this job
-    // to run -- we'll lazily wait for an independent event to trigger a sync.
-    // Note that there may already be such an event if we're in a WaitInterval,
-    // so we can retry it then.
-    if (!requested_types.Empty() && throttled_types.HasAll(requested_types))
-      return DROP;  // TODO(tim): Don't drop. http://crbug.com/177659
+  if (!nudge_tracker_.GetLocallyModifiedTypes().Empty() &&
+      throttled_types.HasAll(nudge_tracker_.GetLocallyModifiedTypes())) {

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

Can you add a todo that we should probably PruneThrottledTypes here?  This is
orthogonal to your change (I think), but I don't want to forget.

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

Done.

+    SDVLOG(1) << "Not running a nudge because we're fully datatype throttled.";
+    return false;
   }
 
-  if (wait_interval_.get())
-    return DecideWhileInWaitInterval(job, priority);
-
   if (mode_ == CONFIGURATION_MODE) {
-    if (job.purpose() == SyncSessionJob::NUDGE)
-      return SAVE;  // Running requires a mode switch.
-    else  // Implies job.purpose() == SyncSessionJob::CONFIGURATION.
-      return CONTINUE;
+    SDVLOG(1) << "Not running nudge because we're in configuration mode.";
+    return false;
   }
 
-  // We are in normal mode.
-  DCHECK_EQ(mode_, NORMAL_MODE);
-  DCHECK_NE(job.purpose(), SyncSessionJob::CONFIGURATION);
-
-  // Note about some subtle scheduling semantics.
-  //
-  // It's possible at this point that |job| is known to be unnecessary, and
-  // dropping it would be perfectly safe and correct. Consider
-  //
-  // 1) |job| is a NUDGE (for any combination of types) with a
-  //    |scheduled_start| time that is less than the time that the last
-  //    successful all-datatype NUDGE completed, and it has a NOTIFICATION
-  //    GetUpdatesCallerInfo value yet offers no new notification hint.
-  //
-  // 2) |job| is a NUDGE with a |scheduled_start| time that is less than
-  //    the time that the last successful matching-datatype NUDGE completed,
-  //    and payloads (hints) are identical to that last successful NUDGE.
-  //
-  //  We avoid cases 1 and 2 by externally synchronizing NUDGE requests --
-  //  scheduling a NUDGE requires command of the sync thread, which is
-  //  impossible* from outside of SyncScheduler if a NUDGE is taking place.
-  //  And if you have command of the sync thread when scheduling a NUDGE and a
-  //  previous NUDGE exists, they will be coalesced and the stale job will be
-  //  cancelled via the session-equality check in DoSyncSessionJob.
-  //
-  //  * It's not strictly "impossible", but it would be reentrant and hence
-  //  illegal. e.g. scheduling a job and re-entering the SyncScheduler is NOT a
-  //  legal side effect of any of the work being done as part of a sync cycle.
-  //  See |no_scheduling_allowed_| for details.
-
-  // Decision now rests on state of auth tokens.
-  if (!session_context_->connection_manager()->HasInvalidAuthToken())
-    return CONTINUE;
-
-  SDVLOG(2) << "No valid auth token. Using that to decide on job.";
-  // Running the job would require updated auth, so we can't honour
-  // job.scheduled_start().
-  return job.purpose() == SyncSessionJob::NUDGE ? SAVE : DROP;
+  return true;
 }
 
 void SyncSchedulerImpl::ScheduleNudgeAsync(
     const TimeDelta& desired_delay,
     NudgeSource source, ModelTypeSet types,
     const tracked_objects::Location& nudge_location) {
   DCHECK(CalledOnValidThread());
   SDVLOG_LOC(nudge_location, 2)
       << "Nudge scheduled with delay "
       << desired_delay.InMilliseconds() << " ms, "
@@ skipping 51 matching lines @@
   SDVLOG_LOC(nudge_location, 2)
       << "In ScheduleNudgeImpl with delay "
       << delay.InMilliseconds() << " ms, "
       << "source " << GetUpdatesSourceString(source) << ", "
       << "payloads "
       << ModelTypeInvalidationMapToString(invalidation_map);
 
   SyncSourceInfo info(source, invalidation_map);
   UpdateNudgeTimeRecords(info);
 
-  scoped_ptr<SyncSessionJob> job(new SyncSessionJob(
-      SyncSessionJob::NUDGE,
-      TimeTicks::Now() + delay,
-      info,
-      ConfigurationParams()));
-  JobProcessDecision decision = DecideOnJob(*job, NORMAL_PRIORITY);
-  SDVLOG(2) << "Should run "
-            << SyncSessionJob::GetPurposeString(job->purpose())
-            << " in mode " << GetModeString(mode_)
-            << ": " << GetDecisionString(decision);
-  if (decision == DROP) {
+  // Coalesce the new nudge information with any existing information.
+  nudge_tracker_.CoalesceSources(info);
+
+  if (!CanRunNudgeJobNow(NORMAL_PRIORITY))
+    return;
+
+  if (!started_) {
+    SDVLOG_LOC(nudge_location, 2)
+        << "Schedule not started; not running a nudge.";
     return;
   }
 
-  // Try to coalesce in both SAVE and CONTINUE cases.
-  if (pending_nudge_job_) {
-    pending_nudge_job_->CoalesceSources(job->source_info());
-    if (decision == CONTINUE) {
-      // Only update the scheduled_start if we're going to reschedule.
-      pending_nudge_job_->set_scheduled_start(
-          std::min(job->scheduled_start(),
-                   pending_nudge_job_->scheduled_start()));
-    }
-  } else {
-    pending_nudge_job_ = job.Pass();
-  }
-
-  if (decision == SAVE) {
+  TimeTicks incoming_run_time = TimeTicks::Now() + delay;
+  if (!scheduled_nudge_time_.is_null() &&
+    (scheduled_nudge_time_ < incoming_run_time)) {
+    // Old job arrives sooner than this one.  Don't reschedule it.
     return;
   }
 
-  TimeDelta run_delay =
-      pending_nudge_job_->scheduled_start() - TimeTicks::Now();
-  if (run_delay < TimeDelta::FromMilliseconds(0))
-    run_delay = TimeDelta::FromMilliseconds(0);
+  // Either there is no existing nudge in flight or the incoming nudge arrives
+  // sooner than it does.  We reschedule in either case.

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

'than it does' is confusing. '... or the incoming nudge should be made to arrive
first.' would be clearer.

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

Done.

   SDVLOG_LOC(nudge_location, 2)
       << "Scheduling a nudge with "
-      << run_delay.InMilliseconds() << " ms delay";
-
-  if (started_) {
-    pending_wakeup_timer_.Start(
-        nudge_location,
-        run_delay,
-        base::Bind(&SyncSchedulerImpl::DoNudgeSyncSessionJob,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   NORMAL_PRIORITY));
-  }
+      << delay.InMilliseconds() << " ms delay";
+  scheduled_nudge_time_ = incoming_run_time;
+  pending_wakeup_timer_.Start(
+      nudge_location,
+      delay,
+      base::Bind(&SyncSchedulerImpl::DoNudgeSyncSessionJob,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 NORMAL_PRIORITY));
 }
 
 const char* SyncSchedulerImpl::GetModeString(SyncScheduler::Mode mode) {
   switch (mode) {
     ENUM_CASE(CONFIGURATION_MODE);
     ENUM_CASE(NORMAL_MODE);
   }
   return "";
 }
 
-const char* SyncSchedulerImpl::GetDecisionString(
-    SyncSchedulerImpl::JobProcessDecision mode) {
-  switch (mode) {
-    ENUM_CASE(CONTINUE);
-    ENUM_CASE(SAVE);
-    ENUM_CASE(DROP);
+void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
+  DCHECK(CalledOnValidThread());
+
+  if (!CanRunNudgeJobNow(priority)) {
+    return;

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

nit - remain consistent with {} on single line ifs.  e.g on 428 you didn't use
them, which is the convention in this file.

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

Done.

   }
-  return "";
+
+  DVLOG(2) << "Will run normal mode sync cycle with routing info "
+           << ModelSafeRoutingInfoToString(session_context_->routing_info());
+  SyncSession session(session_context_, this, nudge_tracker_.source_info());
+  bool premature_exit = !syncer_->SyncShare(&session, SYNCER_BEGIN, SYNCER_END);
+  AdjustPolling(true);
+
+  if (!premature_exit
+      && !sessions::HasSyncerError(

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

nit - at least the && should go on the previous line

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

Done.

+          session.status_controller().model_neutral_state())) {
+    SDVLOG(2) << "Normal mode sync cycle succeeded";
+
+    // That cycle took care of any outstanding work we had.
+    nudge_tracker_.Reset();
+    scheduled_nudge_time_ = base::TimeTicks();
+
+    // If we're here, then we successfully reached the syncer.  End all backoff.

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

s/reached the syncer/reached the server

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

Done.

+    wait_interval_.reset();
+    NotifyRetryTime(base::Time());
+  } else if (IsSyncingCurrentlySilenced()) {
+    SDVLOG(2) << "Normal mode sync cycle got throttled.";
+    // The OnSilencedUntil() call set up the WaitInterval for us.  All we need
+    // to do is start the timer.
+    RestartWaiting();
+  } else {
+    UpdateExponentialBackoff(session.status_controller().model_neutral_state());
+    SDVLOG(2) << "Normal mode sync cycle failed.";
+    RestartWaiting();
+  }
 }
 
-bool SyncSchedulerImpl::DoSyncSessionJobImpl(scoped_ptr<SyncSessionJob> job,
-                                             JobPriority priority) {
+bool SyncSchedulerImpl::DoConfigurationSyncSessionJob(JobPriority priority) {
   DCHECK(CalledOnValidThread());
+  DCHECK_EQ(mode_, CONFIGURATION_MODE);
 
-  base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
-  JobProcessDecision decision = DecideOnJob(*job, priority);
-  SDVLOG(2) << "Should run "
-            << SyncSessionJob::GetPurposeString(job->purpose())
-            << " in mode " << GetModeString(mode_)
-            << " with source " << job->source_info().updates_source
-            << ": " << GetDecisionString(decision);
-  if (decision != CONTINUE) {
-    if (decision == SAVE) {
-      if (job->purpose() == SyncSessionJob::CONFIGURATION) {
-        pending_configure_job_ = job.Pass();
-      } else {
-        pending_nudge_job_ = job.Pass();
-      }
-    } else {
-      DCHECK_EQ(decision, DROP);
-    }
+  if (!CanRunJobNow(priority)) {
+    SDVLOG(2) << "Unable to run configure job right now.";
     return false;
   }
 
-  DVLOG(2) << "Creating sync session with routes "
-           << ModelSafeRoutingInfoToString(session_context_->routing_info())
-           << "and purpose " << job->purpose();
-  SyncSession session(session_context_, this, job->source_info());
+  SDVLOG(2) << "Will run configure SyncShare with routes "
+           << ModelSafeRoutingInfoToString(session_context_->routing_info());
+  SyncSourceInfo source_info(pending_configure_params_->source,
+                             ModelSafeRoutingInfoToInvalidationMap(
+                                 session_context_->routing_info(),
+                                 std::string()));
+  SyncSession session(session_context_, this, source_info);
   bool premature_exit = !syncer_->SyncShare(&session,
-                                            job->start_step(),
-                                            job->end_step());
-  SDVLOG(2) << "Done SyncShare, returned: " << premature_exit;
+                                            DOWNLOAD_UPDATES,
+                                            APPLY_UPDATES);
+  AdjustPolling(true);
 
-  bool success = FinishSyncSessionJob(job.get(),
-                                      premature_exit,
-                                      &session);
+  if (!premature_exit
+      && !sessions::HasSyncerError(
+          session.status_controller().model_neutral_state())) {
+    SDVLOG(2) << "Configure succeeded";
+    pending_configure_params_->ready_task.Run();
+    pending_configure_params_.reset();
 
-  if (IsSyncingCurrentlySilenced()) {
-    SDVLOG(2) << "We are currently throttled; scheduling Unthrottle.";
-    // If we're here, it's because |job| was silenced until a server specified
-    // time. (Note, it had to be |job|, because DecideOnJob would not permit
-    // any job through while in WaitInterval::THROTTLED).
-    if (job->purpose() == SyncSessionJob::NUDGE)
-      pending_nudge_job_ = job.Pass();
-    else if (job->purpose() == SyncSessionJob::CONFIGURATION)
-      pending_configure_job_ = job.Pass();
-    else
-      NOTREACHED();
-
+    // If we're here, then we successfully reached the server.  End all backoff.
+    wait_interval_.reset();
+    NotifyRetryTime(base::Time());
+    return true;
+  } else if (IsSyncingCurrentlySilenced()) {
+    // This shouldn't happen, but we try to respect the server's request anyway.

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

Why shouldn't this happen any more than it should in the nudge case?  We're
giving the server an unconditional lever to pull, right?

One honest disadvantage to splitting up this job-finishing code by job type is
that we want failsafes to be easily understood from a server perspective and as
dead simple of an implementation as possible client side. But we now have to
implement (and inspect, to answer "What happens if we return X?") backoff +
silencing code no less than 3 times, whereas it was 1 before. 

It really seems like this failsafe logic should be shared. None of the
error-case logic depends on job type except that you're not calling
UpdateExpoenentialBackoff for POLLs.  It would be easier to read-off how
backoff/throttle works if we passed control over to single fully inlined
function that always performed the same IsSilenced check and skipped backoff
reset for POLLs.

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

I was under the impression that the server only throttles for excessive commits,
not excessive GUs.  Per-data-type throttling will do little to prevent GetUpdate
requests; I presumed that global throttling was similar.

I agree, the repeated throttled checking isn't ideal.  What I'd like to do
eventually is move that logic into OnSilencedUntil().  Previously it would have
been difficult to do that because of the ownership rules around SyncSessionJob. 
I think that would be the most sensible place to put the code to setup the
throttling interval.

There is some logic that is not common across the job types right now.  For
example, the configure job needs to run its ready task, while the nudge job
wants to reset its nudge interval.  As the nudge tracker gets smarter, we'll
probably want to implement nudge-specific logic in both the success and failure
cases.  I expect this structure will make more sense in the future, but right
now it looks a bit odd because it's trying very hard to maintain similarity with
the old code.  

[tim (not reviewing), 2013/04/16 20:16:24]

Yeah, global throttling is more of a panic switch than per-data-type throttling.
It will always prevent GUs as well as Commits, and should be triggerable by
either path.

The logic you're talking about that is job-type specific doesn't apply in the
case of an error.  I think error handling (backoff, throttling) is agnostic to
job type.  

I don't think it'd be too hard to pull the error-handling block that is repeated
in each case into a single method to finish off the Do*Job methods?

[rlarocque, 2013/04/16 22:20:05]

I didn't think this would be a good idea, but I implemented it anyway to see
what it would look like.  That's uploaded as patch set 4.  Personally, I would
still prefer patch set #3.  Maybe I misunderstood your proposal?

+    SDVLOG(2) << "Was throttled during configure request.";
     RestartWaiting();
-    return success;
-  }
-
-  if (!success)
-    ScheduleNextSync(job.Pass(), &session);
-
-  return success;
-}
-
-void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
-  DoSyncSessionJobImpl(pending_nudge_job_.Pass(), priority);
-}
-
-bool SyncSchedulerImpl::DoConfigurationSyncSessionJob(JobPriority priority) {
-  return DoSyncSessionJobImpl(pending_configure_job_.Pass(), priority);
-}
-
-bool SyncSchedulerImpl::ShouldPoll() {
-  if (wait_interval_.get()) {
-    SDVLOG(2) << "Not running poll in wait interval.";
+    return false;
+  } else {
+    UpdateExponentialBackoff(session.status_controller().model_neutral_state());
+    SDVLOG(2) << "Configure failed.  Will back off for "
+        << wait_interval_->length.InMilliseconds() << "ms.";
+    RestartWaiting();
     return false;
   }
-
-  if (mode_ == CONFIGURATION_MODE) {
-    SDVLOG(2) << "Not running poll in configuration mode.";
-    return false;
-  }
-
-  // TODO(rlarocque): Refactor decision-making logic common to all types
-  // of jobs into a shared function.
-
-  if (session_context_->connection_manager()->HasInvalidAuthToken()) {
-    SDVLOG(2) << "Not running poll because auth token is invalid.";
-    return false;
-  }
-
-  return true;
 }
 
 void SyncSchedulerImpl::DoPollSyncSessionJob() {
   ModelSafeRoutingInfo r;
   ModelTypeInvalidationMap invalidation_map =
       ModelSafeRoutingInfoToInvalidationMap(r, std::string());
   SyncSourceInfo info(GetUpdatesCallerInfo::PERIODIC, invalidation_map);
-  scoped_ptr<SyncSessionJob> job(new SyncSessionJob(SyncSessionJob::POLL,
-                                                    TimeTicks::Now(),
-                                                    info,
-                                                    ConfigurationParams()));
-
   base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
 
-  if (!ShouldPoll())
+  if (!CanRunJobNow(NORMAL_PRIORITY)) {
+    SDVLOG(2) << "Unable to run a poll job right now.";
     return;
+  }
 
-  DVLOG(2) << "Polling with routes "
+  if (mode_ != NORMAL_MODE) {
+    SDVLOG(2) << "Not running poll job in configure mode.";
+    return;
+  }
+
+  SDVLOG(2) << "Polling with routes "
            << ModelSafeRoutingInfoToString(session_context_->routing_info());
-  SyncSession session(session_context_, this, job->source_info());
-  bool premature_exit = !syncer_->SyncShare(&session,
-                                            job->start_step(),
-                                            job->end_step());
-  SDVLOG(2) << "Done SyncShare, returned: " << premature_exit;
+  SyncSession session(session_context_, this, info);
+  syncer_->SyncShare(&session, SYNCER_BEGIN, SYNCER_END);
 
-  FinishSyncSessionJob(job.get(), premature_exit, &session);
+  AdjustPolling(false);
 
   if (IsSyncingCurrentlySilenced()) {
-    // Normally we would only call RestartWaiting() if we had a
-    // pending_nudge_job_ or pending_configure_job_ set.  In this case, it's
-    // possible that neither is set.  We create the wait interval anyway because
-    // we need it to make sure we get unthrottled on time.
+    SDVLOG(2) << "Poll request got us throttled.";
+    // The OnSilencedUntil() call set up the WaitInterval for us.  All we need
+    // to do is start the timer.
     RestartWaiting();
   }
 }
 
 void SyncSchedulerImpl::UpdateNudgeTimeRecords(const SyncSourceInfo& info) {
   DCHECK(CalledOnValidThread());
 
   // We are interested in recording time between local nudges for datatypes.
   // TODO(tim): Consider tracking LOCAL_NOTIFICATION as well.
   if (info.updates_source != GetUpdatesCallerInfo::LOCAL)
     return;
 
   base::TimeTicks now = TimeTicks::Now();
   // Update timing information for how often datatypes are triggering nudges.
   for (ModelTypeInvalidationMap::const_iterator iter = info.types.begin();
        iter != info.types.end();
        ++iter) {
     base::TimeTicks previous = last_local_nudges_by_model_type_[iter->first];
     last_local_nudges_by_model_type_[iter->first] = now;
     if (previous.is_null())
       continue;
 
 #define PER_DATA_TYPE_MACRO(type_str) \
     SYNC_FREQ_HISTOGRAM("Sync.Freq" type_str, now - previous);
     SYNC_DATA_TYPE_HISTOGRAM(iter->first);
 #undef PER_DATA_TYPE_MACRO
   }
 }
 
-bool SyncSchedulerImpl::FinishSyncSessionJob(SyncSessionJob* job,
-                                             bool exited_prematurely,
-                                             SyncSession* session) {
-  DCHECK(CalledOnValidThread());
-
-  // Let job know that we're through syncing (calling SyncShare) at this point.
-  bool succeeded = false;
-  {
-    base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
-    succeeded = job->Finish(exited_prematurely, session);
-  }
-
-  SDVLOG(2) << "Updating the next polling time after SyncMain";
-
-  AdjustPolling(job);
-
-  if (succeeded) {
-    // No job currently supported by the scheduler could succeed without
-    // successfully reaching the server.  Therefore, if we make it here, it is
-    // appropriate to reset the backoff interval.
-    wait_interval_.reset();
-    NotifyRetryTime(base::Time());
-    SDVLOG(2) << "Job succeeded so not scheduling more jobs";
-  }
-
-  return succeeded;
-}
-
-void SyncSchedulerImpl::ScheduleNextSync(
-    scoped_ptr<SyncSessionJob> finished_job,
-    SyncSession* session) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(finished_job->purpose() == SyncSessionJob::CONFIGURATION
-         || finished_job->purpose() == SyncSessionJob::NUDGE);
-
-  // TODO(rlarocque): There's no reason why we should blindly backoff and retry
-  // if we don't succeed.  Some types of errors are not likely to disappear on
-  // their own.  With the return values now available in the old_job.session,
-  // we should be able to detect such errors and only retry when we detect
-  // transient errors.
-
-  SDVLOG(2) << "SyncShare job failed; will start or update backoff";
-  HandleContinuationError(finished_job.Pass(), session);
-}
-
-void SyncSchedulerImpl::AdjustPolling(const SyncSessionJob* old_job) {
+void SyncSchedulerImpl::AdjustPolling(bool force_reset) {
   DCHECK(CalledOnValidThread());
 
   TimeDelta poll  = (!session_context_->notifications_enabled()) ?
       syncer_short_poll_interval_seconds_ :
       syncer_long_poll_interval_seconds_;
   bool rate_changed = !poll_timer_.IsRunning() ||
                        poll != poll_timer_.GetCurrentDelay();
 
-  if (old_job && old_job->purpose() != SyncSessionJob::POLL && !rate_changed)
+  if (force_reset && !rate_changed)
     poll_timer_.Reset();
 
   if (!rate_changed)
     return;
 
   // Adjust poll rate.
   poll_timer_.Stop();
   poll_timer_.Start(FROM_HERE, poll, this,
                     &SyncSchedulerImpl::PollTimerCallback);
 }
 
 void SyncSchedulerImpl::RestartWaiting() {
   CHECK(wait_interval_.get());
   DCHECK(wait_interval_->length >= TimeDelta::FromSeconds(0));
+  NotifyRetryTime(base::Time::Now() + wait_interval_->length);
+  SDVLOG(2) << "Starting WaitInterval timer of length "
+      << wait_interval_->length.InMilliseconds() << "ms.";
   if (wait_interval_->mode == WaitInterval::THROTTLED) {
     pending_wakeup_timer_.Start(
         FROM_HERE,
         wait_interval_->length,
         base::Bind(&SyncSchedulerImpl::Unthrottle,
                    weak_ptr_factory_.GetWeakPtr()));
   } else {
     pending_wakeup_timer_.Start(
         FROM_HERE,
         wait_interval_->length,
         base::Bind(&SyncSchedulerImpl::TryCanaryJob,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
-void SyncSchedulerImpl::HandleContinuationError(
-    scoped_ptr<SyncSessionJob> old_job,
-    SyncSession* session) {
+void SyncSchedulerImpl::UpdateExponentialBackoff(
+    const sessions::ModelNeutralState& model_neutral_state) {
   DCHECK(CalledOnValidThread());
 
   TimeDelta length = delay_provider_->GetDelay(
       IsBackingOff() ? wait_interval_->length :
-          delay_provider_->GetInitialDelay(
-              session->status_controller().model_neutral_state()));
-
-  SDVLOG(2) << "In handle continuation error with "
-            << SyncSessionJob::GetPurposeString(old_job->purpose())
-            << " job. The time delta(ms) is "
-            << length.InMilliseconds();
-
+          delay_provider_->GetInitialDelay(model_neutral_state));
   wait_interval_.reset(new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF,
                                         length));
-  NotifyRetryTime(base::Time::Now() + length);
-  old_job->set_scheduled_start(TimeTicks::Now() + length);
-  if (old_job->purpose() == SyncSessionJob::CONFIGURATION) {
-    SDVLOG(2) << "Configuration did not succeed, scheduling retry.";
-    // Config params should always get set.
-    DCHECK(!old_job->config_params().ready_task.is_null());
-    DCHECK(!pending_configure_job_);
-    pending_configure_job_ = old_job.Pass();
-  } else {
-    // We're not in configure mode so we should not have a configure job.
-    DCHECK(!pending_configure_job_);
-    DCHECK(!pending_nudge_job_);
-    pending_nudge_job_ = old_job.Pass();
-  }
-
-  RestartWaiting();
 }
 
 void SyncSchedulerImpl::RequestStop(const base::Closure& callback) {
   syncer_->RequestEarlyExit();  // Safe to call from any thread.
   DCHECK(weak_handle_this_.IsInitialized());
   SDVLOG(3) << "Posting StopImpl";
   weak_handle_this_.Call(FROM_HERE,
                          &SyncSchedulerImpl::StopImpl,
                          callback);
 }
 
 void SyncSchedulerImpl::StopImpl(const base::Closure& callback) {
   DCHECK(CalledOnValidThread());
   SDVLOG(2) << "StopImpl called";
 
   // Kill any in-flight method calls.
   weak_ptr_factory_.InvalidateWeakPtrs();
   wait_interval_.reset();
   NotifyRetryTime(base::Time());
   poll_timer_.Stop();
   pending_wakeup_timer_.Stop();
-  pending_nudge_job_.reset();
-  pending_configure_job_.reset();
+  pending_configure_params_.reset();
   if (started_) {
     started_ = false;
   }
   if (!callback.is_null())
     callback.Run();
 }
 
 // This is the only place where we invoke DoSyncSessionJob with canary
 // privileges.  Everyone else should use NORMAL_PRIORITY.
 void SyncSchedulerImpl::TryCanaryJob() {
   DCHECK(CalledOnValidThread());
 
-  if (mode_ == CONFIGURATION_MODE && pending_configure_job_) {
+  if (mode_ == CONFIGURATION_MODE && pending_configure_params_) {
     SDVLOG(2) << "Found pending configure job; will run as canary";
     DoConfigurationSyncSessionJob(CANARY_PRIORITY);
-  } else if (mode_ == NORMAL_MODE && pending_nudge_job_) {
+  } else if (mode_ == NORMAL_MODE && !nudge_tracker_.IsEmpty()) {
     SDVLOG(2) << "Found pending nudge job; will run as canary";
     DoNudgeSyncSessionJob(CANARY_PRIORITY);
   } else {
     SDVLOG(2) << "Found no work to do; will not run a canary";
   }
 }
 
 void SyncSchedulerImpl::PollTimerCallback() {
   DCHECK(CalledOnValidThread());
   if (no_scheduling_allowed_) {
@@ skipping 115 matching lines @@
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // namespace syncer