sync: Handle POLL jobs in separate a code path

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 325 matching lines @@
   }
 
   return true;
 }
 
 SyncSchedulerImpl::JobProcessDecision
 SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob& job,
                                              JobPriority priority) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(wait_interval_.get());
+  DCHECK_NE(job.purpose(), SyncSessionJob::POLL);
 
   SDVLOG(2) << "DecideWhileInWaitInterval with WaitInterval mode "
             << WaitInterval::GetModeString(wait_interval_->mode)
             << (wait_interval_->had_nudge ? " (had nudge)" : "")
             << ((priority == CANARY_PRIORITY) ? " (canary)" : "");
 
-  if (job.purpose() == SyncSessionJob::POLL)
-    return DROP;
-
   // 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 throttled, there's a clock ticking to unthrottle. We want to get
   // on the same train.
   if (wait_interval_->mode == WaitInterval::THROTTLED)
     return SAVE;
@@ skipping 11 matching lines @@
       return CONTINUE;
   }
   return (priority == CANARY_PRIORITY) ? CONTINUE : SAVE;
 }
 
 SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
     const SyncSessionJob& job,
     JobPriority priority) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
+  // POLL jobs do not call this function.
+  DCHECK(job.purpose() == SyncSessionJob::NUDGE ||
+         job.purpose() == SyncSessionJob::CONFIGURATION);
+
   // See if our type is throttled.
   ModelTypeSet throttled_types =
       session_context_->throttled_data_type_tracker()->GetThrottledTypes();
   if (job.purpose() == SyncSessionJob::NUDGE &&
       job.session()->source().updates_source == GetUpdatesCallerInfo::LOCAL) {
     ModelTypeSet requested_types;
     for (ModelTypeInvalidationMap::const_iterator i =
          job.session()->source().types.begin();
          i != job.session()->source().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 (wait_interval_.get())
     return DecideWhileInWaitInterval(job, priority);
 
   if (mode_ == CONFIGURATION_MODE) {
     if (job.purpose() == SyncSessionJob::NUDGE)
       return SAVE;  // Running requires a mode switch.
-    else if (job.purpose() == SyncSessionJob::CONFIGURATION)
+    else  // Implies job.purpose() == SyncSessionJob::CONFIGURATION.
       return CONTINUE;
-    else
-      return DROP;
   }
 
   // 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 POLL with a |scheduled_start| time that is less than
-  //    the time that the last successful all-datatype NUDGE completed.
-  //
-  // 2) |job| is a NUDGE (for any combination of types) with a
+  // 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.
   //
-  // 3) |job| is a NUDGE with a |scheduled_start| time that is less than
+  // 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.
   //
-  //  Case 1 can occur if the POLL timer fires *after* a call to
-  //  ScheduleSyncSessionJob for a NUDGE, but *before* the thread actually
-  //  picks the resulting posted task off of the MessageLoop. The NUDGE will
-  //  run first and complete at a time greater than the POLL scheduled_start.
-  //  However, this case (and POLLs in general) is so rare that we ignore it (
-  //  and avoid the required bookeeping to simplify code).
-  //
-  //  We avoid cases 2 and 3 by externally synchronizing NUDGE requests --
+  //  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.
@@ skipping 97 matching lines @@
       << "source " << GetNudgeSourceString(source) << ", "
       << "payloads "
       << ModelTypeInvalidationMapToString(invalidation_map);
 
   SyncSchedulerImpl::ScheduleNudgeImpl(desired_delay,
                                        GetUpdatesFromNudgeSource(source),
                                        invalidation_map,
                                        nudge_location);
 }
 
+
+// TODO(zea): Consider adding separate throttling/backoff for datatype
+// refresh requests.
 void SyncSchedulerImpl::ScheduleNudgeImpl(
     const TimeDelta& delay,
     GetUpdatesCallerInfo::GetUpdatesSource source,
     const ModelTypeInvalidationMap& invalidation_map,
     const tracked_objects::Location& nudge_location) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(!invalidation_map.empty()) << "Nudge scheduled for no types!";
 
+  if (no_scheduling_allowed_) {
+    NOTREACHED() << "Illegal to schedule job while session in progress.";
+    return;
+  }
+
   if (!started_) {
     SDVLOG_LOC(nudge_location, 2)
         << "Dropping nudge, scheduler is not running.";
     return;
   }
 
   SDVLOG_LOC(nudge_location, 2)
       << "In ScheduleNudgeImpl with delay "
       << delay.InMilliseconds() << " ms, "
       << "source " << GetUpdatesSourceString(source) << ", "
@@ skipping 39 matching lines @@
     // was previously unscheduled and giving it wings, so take care to reset
     // unscheduled nudge storage.
     job = pending_nudge_->CloneAndAbandon();
     pending_nudge_ = NULL;
     unscheduled_nudge_storage_.reset();
     // It's also possible we took a canary job, since we allow one nudge
     // per backoff interval.
     DCHECK(!wait_interval_ || !wait_interval_->had_nudge);
   }
 
-  // TODO(zea): Consider adding separate throttling/backoff for datatype
-  // refresh requests.
-  ScheduleSyncSessionJob(nudge_location, job.Pass());
+  TimeDelta run_delay = job->scheduled_start() - TimeTicks::Now();
+  if (run_delay < TimeDelta::FromMilliseconds(0))
+    run_delay = TimeDelta::FromMilliseconds(0);
+  SDVLOG_LOC(nudge_location, 2)
+      << "Scheduling a nudge with "
+      << run_delay.InMilliseconds() << " ms delay";
+
+  pending_nudge_ = job.get();
+  PostDelayedTask(nudge_location, "DoSyncSessionJob",
+      base::Bind(base::IgnoreResult(&SyncSchedulerImpl::DoSyncSessionJob),
+                                    weak_ptr_factory_.GetWeakPtr(),
+                                    base::Passed(&job),
+                                    NORMAL_PRIORITY),
+      run_delay);
 }
 
 const char* SyncSchedulerImpl::GetModeString(SyncScheduler::Mode mode) {
   switch (mode) {
     ENUM_CASE(CONFIGURATION_MODE);
     ENUM_CASE(NORMAL_MODE);
   }
   return "";
 }
 
@@ skipping 25 matching lines @@
   SDVLOG_LOC(from_here, 3) << "Posting " << name << " task with "
                            << delay.InMilliseconds() << " ms delay";
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   if (!started_) {
     SDVLOG(1) << "Not posting task as scheduler is stopped.";
     return;
   }
   sync_loop_->PostDelayedTask(from_here, task, delay);
 }
 
-void SyncSchedulerImpl::ScheduleSyncSessionJob(
-    const tracked_objects::Location& loc,
-    scoped_ptr<SyncSessionJob> job) {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  if (no_scheduling_allowed_) {
-    NOTREACHED() << "Illegal to schedule job while session in progress.";
-    return;
-  }
-
-  TimeDelta delay = job->scheduled_start() - TimeTicks::Now();
-  if (delay < TimeDelta::FromMilliseconds(0))
-    delay = TimeDelta::FromMilliseconds(0);
-  SDVLOG_LOC(loc, 2)
-      << "In ScheduleSyncSessionJob with "
-      << SyncSessionJob::GetPurposeString(job->purpose())
-      << " job and " << delay.InMilliseconds() << " ms delay";
-
-  DCHECK(job->purpose() == SyncSessionJob::NUDGE ||
-         job->purpose() == SyncSessionJob::POLL);
-  if (job->purpose() == SyncSessionJob::NUDGE) {
-    SDVLOG_LOC(loc, 2) << "Resetting pending_nudge to ";
-    DCHECK(!pending_nudge_ || pending_nudge_->session() ==
-           job->session());
-    pending_nudge_ = job.get();
-  }
-
-  PostDelayedTask(loc, "DoSyncSessionJob",
-      base::Bind(base::IgnoreResult(&SyncSchedulerImpl::DoSyncSessionJob),
-                                    weak_ptr_factory_.GetWeakPtr(),
-                                    base::Passed(&job),
-                                    NORMAL_PRIORITY),
-      delay);
-}
-
 bool SyncSchedulerImpl::DoSyncSessionJob(scoped_ptr<SyncSessionJob> job,
                                          JobPriority priority) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   if (job->purpose() == SyncSessionJob::NUDGE) {
     if (pending_nudge_ == NULL ||
         pending_nudge_->session() != job->session()) {
       // |job| is abandoned.
       SDVLOG(2) << "Dropping a nudge in "
                 << "DoSyncSessionJob because another nudge was scheduled";
       return false;
@@ skipping 23 matching lines @@
     return false;
   }
 
   SDVLOG(2) << "Calling SyncShare with "
             << SyncSessionJob::GetPurposeString(job->purpose()) << " job";
   bool premature_exit = !syncer_->SyncShare(job->mutable_session(),
                                             job->start_step(),
                                             job->end_step());
   SDVLOG(2) << "Done SyncShare, returned: " << premature_exit;
 
-  return FinishSyncSessionJob(job.Pass(), premature_exit);
+  bool success = FinishSyncSessionJob(job.get(), premature_exit);
+
+  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).
+    scoped_ptr<SyncSessionJob> clone = job->Clone();
+    if (clone->purpose() == SyncSessionJob::NUDGE)
+      pending_nudge_ = clone.get();
+    else if (clone->purpose() == SyncSessionJob::CONFIGURATION)
+      wait_interval_->pending_configure_job = clone.get();
+    else
+      NOTREACHED();
+
+    RestartWaiting(clone.Pass());
+    return success;
+  }
+
+  if (!success)
+    ScheduleNextSync(job.Pass());
+
+  return success;
+}
+
+bool SyncSchedulerImpl::ShouldPoll() {
+  if (wait_interval_.get()) {
+    SDVLOG(2) << "Not running poll in wait interval.";
+    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(scoped_ptr<SyncSessionJob> job) {
+  DCHECK_EQ(job->purpose(), SyncSessionJob::POLL);
+
+  base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
+
+  if (!ShouldPoll())
+    return;
+
+  SDVLOG(2) << "Calling SyncShare with "
+            << SyncSessionJob::GetPurposeString(job->purpose()) << " job";
+  bool premature_exit = !syncer_->SyncShare(job->mutable_session(),
+                                            job->start_step(),
+                                            job->end_step());
+  SDVLOG(2) << "Done SyncShare, returned: " << premature_exit;
+
+  FinishSyncSessionJob(job.get(), premature_exit);
+
+  if (IsSyncingCurrentlySilenced()) {
+    // This will start the countdown to unthrottle.  Other kinds of jobs would
+    // schedule themselves as the post-unthrottle canary.  A poll job is not
+    // that urgent, so it does not get to be the canary.  We still need to start
+    // the timer regardless.  Otherwise there could be no one to clear the
+    // WaitInterval when the throttling expires.
+    RestartWaiting(scoped_ptr<SyncSessionJob>());
+  }
 }
 
 void SyncSchedulerImpl::UpdateNudgeTimeRecords(const SyncSourceInfo& info) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
   // 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(scoped_ptr<SyncSessionJob> job,
+bool SyncSchedulerImpl::FinishSyncSessionJob(SyncSessionJob* job,
                                              bool exited_prematurely) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
   // 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);
   }
 
   SDVLOG(2) << "Updating the next polling time after SyncMain";
-  ScheduleNextSync(job.Pass(), succeeded);
-  return succeeded;
-}
 
-void SyncSchedulerImpl::ScheduleNextSync(
-    scoped_ptr<SyncSessionJob> finished_job, bool succeeded) {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-
-  AdjustPolling(finished_job.get());
+  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;
   }
 
-  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).
-    scoped_ptr<SyncSessionJob> clone = finished_job->Clone();
-    if (clone->purpose() == SyncSessionJob::NUDGE)
-      pending_nudge_ = clone.get();
-    else if (clone->purpose() == SyncSessionJob::CONFIGURATION)
-      wait_interval_->pending_configure_job = clone.get();
-    else
-      clone.reset();  // Unthrottling is enough, no need to force a canary.
+  return succeeded;
+}
 
-    RestartWaiting(clone.Pass());
-    return;
-  }
-
-  if (finished_job->purpose() == SyncSessionJob::POLL) {
-    return; // We don't retry POLL jobs.
-  }
+void SyncSchedulerImpl::ScheduleNextSync(
+    scoped_ptr<SyncSessionJob> finished_job) {
+  DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  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.
 
   if (IsBackingOff() && wait_interval_->timer.IsRunning() &&
       mode_ == NORMAL_MODE) {
     // When in normal mode, we allow up to one nudge per backoff interval.  It
@@ skipping 124 matching lines @@
 void SyncSchedulerImpl::DoCanaryJob(scoped_ptr<SyncSessionJob> to_be_canary) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   SDVLOG(2) << "Do canary job";
 
   if (to_be_canary->purpose() == SyncSessionJob::NUDGE) {
     // TODO(tim): Bug 158313.  Remove this check.
     if (pending_nudge_ == NULL ||
         pending_nudge_->session() != to_be_canary->session()) {
       // |job| is abandoned.
       SDVLOG(2) << "Dropping a nudge in "
-                << "DoSyncSessionJob because another nudge was scheduled";
+                << "DoCanaryJob because another nudge was scheduled";
       return;
     }
     DCHECK_EQ(pending_nudge_->session(), to_be_canary->session());
   }
 
   // This is the only place where we invoke DoSyncSessionJob with canary
   // privileges.  Everyone else should use NORMAL_PRIORITY.
   DoSyncSessionJob(to_be_canary.Pass(), CANARY_PRIORITY);
 }
 
@@ skipping 34 matching lines @@
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   ModelSafeRoutingInfo r;
   ModelTypeInvalidationMap invalidation_map =
       ModelSafeRoutingInfoToInvalidationMap(r, std::string());
   SyncSourceInfo info(GetUpdatesCallerInfo::PERIODIC, invalidation_map);
   scoped_ptr<SyncSession> s(CreateSyncSession(info));
   scoped_ptr<SyncSessionJob> job(new SyncSessionJob(SyncSessionJob::POLL,
                                                     TimeTicks::Now(),
                                                     s.Pass(),
                                                     ConfigurationParams()));
-  ScheduleSyncSessionJob(FROM_HERE, job.Pass());
+  if (no_scheduling_allowed_) {
+    // The no_scheduling_allowed_ flag is set by a function-scoped AutoReset in
+    // functions that are called only on the sync thread.  This function is also
+    // called only on the sync thread, and only when it is posted by an expiring
+    // timer.  If we find that no_scheduling_allowed_ is set here, then
+    // something is very wrong.  Maybe someone mistakenly called us directly, or
+    // mishandled the book-keeping for no_scheduling_allowed_.
+    NOTREACHED() << "Illegal to schedule job while session in progress.";
+    return;
+  }
+
+  DoPollSyncSessionJob(job.Pass());
 }
 
 void SyncSchedulerImpl::Unthrottle(scoped_ptr<SyncSessionJob> to_be_canary) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK_EQ(WaitInterval::THROTTLED, wait_interval_->mode);
   DCHECK(!to_be_canary.get() || pending_nudge_ == to_be_canary.get() ||
          wait_interval_->pending_configure_job == to_be_canary.get());
   SDVLOG(2) << "Unthrottled " << (to_be_canary.get() ? "with " : "without ")
            << "canary.";
 
@@ skipping 102 matching lines @@
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // namespace syncer