Client-side changes to support retry GU.

sync/engine/sync_scheduler_impl.cc

Index: sync/engine/sync_scheduler_impl.cc
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index 6a4f1efa80a89cdec7e12a25534cbf0b772997b5..2b111c0a28d55e00780cdebe077f22a50746498b 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -14,6 +14,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
+#include "base/time/default_clock.h"
 #include "sync/engine/backoff_delay_provider.h"
 #include "sync/engine/syncer.h"
 #include "sync/notifier/object_id_invalidation_map.h"
@@ -165,6 +166,7 @@ SyncSchedulerImpl::SyncSchedulerImpl(const std::string& name,
           TimeDelta::FromSeconds(kDefaultSessionsCommitDelaySeconds)),
       mode_(NORMAL_MODE),
       delay_provider_(delay_provider),
+      nudge_tracker_(new base::DefaultClock),
       syncer_(syncer),
       session_context_(context),
       no_scheduling_allowed_(false),
@@ -234,7 +236,7 @@ void SyncSchedulerImpl::Start(Mode mode) {
 
   if (old_mode != mode_ &&
       mode_ == NORMAL_MODE &&
-      nudge_tracker_.IsSyncRequired() &&
+      (nudge_tracker_.IsSyncRequired() || nudge_tracker_.IsRetryRequired()) &&
       CanRunNudgeJobNow(NORMAL_PRIORITY)) {
     // We just got back to normal mode.  Let's try to run the work that was
     // queued up while we were configuring.
@@ -447,40 +449,6 @@ const char* SyncSchedulerImpl::GetModeString(SyncScheduler::Mode mode) {
   return "";
 }
 
-void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(CanRunNudgeJobNow(priority));
-
-  DVLOG(2) << "Will run normal mode sync cycle with types "
-           << ModelTypeSetToString(session_context_->enabled_types());
-  scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this));
-  bool premature_exit = !syncer_->NormalSyncShare(
-      GetEnabledAndUnthrottledTypes(),
-      nudge_tracker_,
-      session.get());
-  AdjustPolling(FORCE_RESET);
-  // Don't run poll job till the next time poll timer fires.
-  do_poll_after_credentials_updated_ = false;
-
-  bool success = !premature_exit
-      && !sessions::HasSyncerError(
-          session->status_controller().model_neutral_state());
-
-  if (success) {
-    // That cycle took care of any outstanding work we had.
-    SDVLOG(2) << "Nudge succeeded.";
-    nudge_tracker_.RecordSuccessfulSyncCycle();
-    scheduled_nudge_time_ = base::TimeTicks();
-
-    // If we're here, then we successfully reached the server.  End all backoff.
-    wait_interval_.reset();
-    NotifyRetryTime(base::Time());
-    return;
-  } else {
-    HandleFailure(session->status_controller().model_neutral_state());
-  }
-}
-
 void SyncSchedulerImpl::DoConfigurationSyncSessionJob(JobPriority priority) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(mode_, CONFIGURATION_MODE);
@@ -546,34 +514,38 @@ void SyncSchedulerImpl::HandleFailure(
   }
 }
 
-void SyncSchedulerImpl::DoPollSyncSessionJob() {
-  base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
+bool SyncSchedulerImpl::DoNudgeSyncSessionJob(SyncSession* session) {
+  DCHECK(CalledOnValidThread());
 
-  if (!CanRunJobNow(NORMAL_PRIORITY)) {
-    SDVLOG(2) << "Unable to run a poll job right now.";
-    return;
-  }
+  DVLOG(2) << "Will run normal mode sync cycle with types "
+           << ModelTypeSetToString(GetEnabledAndUnthrottledTypes());
+  return syncer_->NormalSyncShare(GetEnabledAndUnthrottledTypes(),
+                                  nudge_tracker_,
+                                  session);
+}
 
-  if (mode_ != NORMAL_MODE) {
-    SDVLOG(2) << "Not running poll job in configure mode.";
-    return;
-  }
+bool SyncSchedulerImpl::DoPollSyncSessionJob(SyncSession* session) {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(mode_, NORMAL_MODE);
+
+  base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
 
   SDVLOG(2) << "Polling with types "
-            << ModelTypeSetToString(session_context_->enabled_types());
-  scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this));
-  syncer_->PollSyncShare(
-      GetEnabledAndUnthrottledTypes(),
-      session.get());
+            << ModelTypeSetToString(GetEnabledAndUnthrottledTypes());
+  return syncer_->PollSyncShare(GetEnabledAndUnthrottledTypes(),
+                                session);
+}
 
-  AdjustPolling(FORCE_RESET);
+bool SyncSchedulerImpl::DoRetrySyncSessionJob(SyncSession* session) {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(mode_, NORMAL_MODE);
 
-  if (IsCurrentlyThrottled()) {
-    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();
-  }
+  base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
+
+  SDVLOG(2) << "Retry with types "
+            << ModelTypeSetToString(GetEnabledAndUnthrottledTypes());
+  return syncer_->RetrySyncShare(GetEnabledAndUnthrottledTypes(),
+                                 session);
 }
 
 void SyncSchedulerImpl::UpdateNudgeTimeRecords(ModelTypeSet types) {
@@ -685,21 +657,24 @@ void SyncSchedulerImpl::TrySyncSessionJobImpl() {
     }
   } else {
     DCHECK(mode_ == NORMAL_MODE);
-    if (nudge_tracker_.IsSyncRequired() && CanRunNudgeJobNow(priority)) {
+    if (!CanRunNudgeJobNow(priority))
+      return;
+
+    scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this));

[rlarocque, 2014/01/06 23:00:33]

I'm strongly against this.  I spent a lot of time trying to eliminate the
SyncSession.  It's brittle and has caused lots of hard to detect crashes in the
past.

Looking to the future, I think an important goal will be to remove the concept
of the sync cycle.  Otherwise we'll never be able to properly handle
no-precommit-GU cycles.  The SyncSession is an obstacle that prevents this
change, and that's why I want to eliminate it.

This change would make it much harder to remove the SyncSession.  We should find
some other way to implement this.

[haitaol1, 2014/01/07 19:03:37]

Done.

+    bool tried_sync = false;
+
+    if (nudge_tracker_.IsSyncRequired()) {
       SDVLOG(2) << "Found pending nudge job";
-      DoNudgeSyncSessionJob(priority);
+      tried_sync = DoNudgeSyncSessionJob(session.get());
+    } else if (nudge_tracker_.IsRetryRequired()) {
+      tried_sync = DoRetrySyncSessionJob(session.get());
     } else if (do_poll_after_credentials_updated_ ||
-        ((base::TimeTicks::Now() - last_poll_reset_) >= GetPollInterval())) {
-      DoPollSyncSessionJob();
-      // Poll timer fires infrequently. Usually by this time access token is
-      // already expired and poll job will fail with auth error. Set flag to
-      // retry poll once ProfileSyncService gets new access token, TryCanaryJob
-      // will be called after access token is retrieved.
-      if (HttpResponse::SYNC_AUTH_ERROR ==
-          session_context_->connection_manager()->server_status()) {
-        do_poll_after_credentials_updated_ = true;
-      }
+        (base::TimeTicks::Now() - last_poll_reset_) >= GetPollInterval()) {
+      tried_sync = DoPollSyncSessionJob(session.get());
     }
+
+    if (tried_sync)
+      SyncSessionPostProcessing(session.get());
   }
 
   if (priority == CANARY_PRIORITY) {
@@ -721,6 +696,30 @@ void SyncSchedulerImpl::TrySyncSessionJobImpl() {
   }
 }
 
+void SyncSchedulerImpl::SyncSessionPostProcessing(SyncSession* session) {
+  AdjustPolling(FORCE_RESET);
+  // Don't run poll job till the next time poll timer fires.
+  do_poll_after_credentials_updated_ = false;
+
+  bool success = !sessions::HasSyncerError(
+          session->status_controller().model_neutral_state());
+
+  if (success) {
+    // That cycle took care of any outstanding work we had.
+    SDVLOG(2) << "Sync session succeeded.";
+    nudge_tracker_.RecordSuccessfulSyncCycle();

[rlarocque, 2014/01/06 23:00:33]

The SyncScheduler used to have code that looks like this.  I've tried to move it
in a different direction, and this would be a step backward.

At some point, we're going to have to let the NudgeTracker know when the syncer
has completed a GU, Commit, or both.  It will have to update it's state
differently in each case.  Calling "RecordSuccessfulSyncCycle()" may update the
status wrongly if this was a commit-only sync cycle.  There are also some
strange edge cases, like when a GU succeeds but the commit fails with a
transient error.

The syncer, about:sync and a few other components are still not quite ready for
that change.  However, when that change does come, we'll need to have failure
handling logic that is unique to each kind of job in the SyncScheduler.  This is
why I moved the error handling logic into separate functions and why I want to
keep it there.

[haitaol1, 2014/01/07 19:03:37]

Done.

+    scheduled_nudge_time_ = base::TimeTicks();
+
+    // If we're here, then we successfully reached the server.  End all backoff.
+    wait_interval_.reset();
+    NotifyRetryTime(base::Time());
+
+    return;
+  } else {
+    HandleFailure(session->status_controller().model_neutral_state());
+  }
+}
+
 void SyncSchedulerImpl::PollTimerCallback() {
   DCHECK(CalledOnValidThread());
   if (no_scheduling_allowed_) {
@@ -737,6 +736,10 @@ void SyncSchedulerImpl::PollTimerCallback() {
   TrySyncSessionJob();
 }
 
+void SyncSchedulerImpl::RetryTimerCallback() {
+  TrySyncSessionJob();
+}
+
 void SyncSchedulerImpl::Unthrottle() {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(WaitInterval::THROTTLED, wait_interval_->mode);
@@ -890,6 +893,13 @@ void SyncSchedulerImpl::OnSyncProtocolError(
     OnActionableError(snapshot);
 }
 
+void SyncSchedulerImpl::OnReceivedGuRetryDelaySeconds(int delay_seconds) {
+  nudge_tracker_.set_next_retry_time(
+      base::Time::Now() + base::TimeDelta::FromSeconds(delay_seconds));
+  retry_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(delay_seconds),
+                     this, &SyncSchedulerImpl::RetryTimerCallback);
+}
+
 void SyncSchedulerImpl::SetNotificationsEnabled(bool notifications_enabled) {
   DCHECK(CalledOnValidThread());
   session_context_->set_notifications_enabled(notifications_enabled);