sync: GU retry with less explicit TimeTicks logic

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 ac169ec18f4c394994c8230fffb3a2b757801138..0a3ee33f84652d2ed888411f16e4dc859cb6a99a 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -170,6 +170,7 @@ SyncSchedulerImpl::SyncSchedulerImpl(const std::string& name,
       no_scheduling_allowed_(false),
       do_poll_after_credentials_updated_(false),
       next_sync_session_job_priority_(NORMAL_PRIORITY),
+      received_pending_retry_(false),
       weak_ptr_factory_(this),
       weak_ptr_factory_for_weak_handle_(this) {
   weak_handle_this_ = MakeWeakHandle(
@@ -234,8 +235,7 @@ void SyncSchedulerImpl::Start(Mode mode) {
 
   if (old_mode != mode_ &&
       mode_ == NORMAL_MODE &&
-      (nudge_tracker_.IsSyncRequired() ||
-          nudge_tracker_.IsRetryRequired(base::TimeTicks::Now())) &&
+      (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.
@@ -452,6 +452,9 @@ void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
   DCHECK(CalledOnValidThread());
   DCHECK(CanRunNudgeJobNow(priority));
 
+  // Clear pending retry flag in case it was stale.
+  ClearReceivedGuRetryDelayFlag();
+
   DVLOG(2) << "Will run normal mode sync cycle with types "
            << ModelTypeSetToString(session_context_->enabled_types());
   scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this));
@@ -470,13 +473,15 @@ void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
   if (success) {
     // That cycle took care of any outstanding work we had.
     SDVLOG(2) << "Nudge succeeded.";
-    nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+    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;
+
+    // Set the next GU retry time, if the server requested it.
+    ProcessReceivedGuRetryDelay();

[haitaol1, 2014/01/27 23:17:07]

Previously retry may be scheduled even when sync failed (partially successful).
Retry should depend on server command only.

[rlarocque, 2014/01/28 01:06:34]

Interesting.  I think that makes a few corner cases a bit easier to handle.

   } else {
     HandleFailure(session->status_controller().model_neutral_state());
   }
@@ -580,7 +585,7 @@ void SyncSchedulerImpl::DoRetrySyncSessionJob() {
                               session.get()) &&
       !sessions::HasSyncerError(
           session->status_controller().model_neutral_state())) {
-    nudge_tracker_.RecordSuccessfulSyncCycle(base::TimeTicks::Now());
+    nudge_tracker_.RecordSuccessfulSyncCycle();
   } else {
     HandleFailure(session->status_controller().model_neutral_state());
   }
@@ -687,6 +692,8 @@ void SyncSchedulerImpl::TrySyncSessionJobImpl() {
   JobPriority priority = next_sync_session_job_priority_;
   next_sync_session_job_priority_ = NORMAL_PRIORITY;
 
+  nudge_tracker_.ToggleRetryFlagIfRequired(base::TimeTicks::Now());
+
   DCHECK(CalledOnValidThread());
   if (mode_ == CONFIGURATION_MODE) {
     if (pending_configure_params_) {
@@ -697,7 +704,7 @@ void SyncSchedulerImpl::TrySyncSessionJobImpl() {
     if (nudge_tracker_.IsSyncRequired()) {
       SDVLOG(2) << "Found pending nudge job";
       DoNudgeSyncSessionJob(priority);
-    } else if (nudge_tracker_.IsRetryRequired(base::TimeTicks::Now())) {
+    } else if (nudge_tracker_.IsRetryRequired()) {
       DoRetrySyncSessionJob();
     } else if (do_poll_after_credentials_updated_ ||
         ((base::TimeTicks::Now() - last_poll_reset_) >= GetPollInterval())) {
@@ -906,9 +913,21 @@ void SyncSchedulerImpl::OnSyncProtocolError(
 }
 
 void SyncSchedulerImpl::OnReceivedGuRetryDelay(const base::TimeDelta& delay) {
-  nudge_tracker_.set_next_retry_time(base::TimeTicks::Now() + delay);
-  retry_timer_.Start(FROM_HERE, delay, this,
-                     &SyncSchedulerImpl::RetryTimerCallback);
+  // We'll deal with this after the sync cycle is complete.
+  received_pending_retry_ = true;

[haitaol1, 2014/01/27 23:17:07]

Why change this? Previous implementation is simple and easy to understand. This
change is complicated and requires coordinating calls in several places. I don't
think there's a need for that.

I see your argument about NudgeTracker::IsRetryRequired() being consistent
before/after. I'm not sure that's true. E.g. if sync cycle starts before retry
timeout and finishes after, shouldn't IsRetryRequired() be false before and true
after.

[rlarocque, 2014/01/28 01:06:34]

Agreed, it is complicated.  Pavel made similar comments.  I'll have to change
this.
I was trying to prevent a scenario where the NudgeTracker was modified while the
sync cycle is in progress.  The NudgeTracker is always referenced as a 'const
NudgeTracker&' while the sync cycle is in progress.  To use the
SyncSessionDelegate to escape the sync cycle, return to this context, and modify
it anyway seems like cheating.  

Depending on some details of how the NudgeTracker keeps track of the retry state
and what the semantics of GU retries should be, this could cause subtle bugs.  

For example, let's say the syncer is fetching updates because of a GU retry.  If
the server decides to return more than one batch of items and also sets a new
retry delay, should the second sync cycle (the one that fetches the next batch
of the items) also have the is_retry flag set?  I would think yes.  If this
callback can change the NudgeTracker's IsRetryRequired(), we'd need to be very
careful about how we handle this scenario.

+  pending_retry_delay_ = delay;
+}
+
+void SyncSchedulerImpl::ProcessReceivedGuRetryDelay() {
+  if (received_pending_retry_) {
+    nudge_tracker_.SetNextRetryTime(TimeTicks::Now() + pending_retry_delay_);
+    retry_timer_.Start(FROM_HERE, pending_retry_delay_, this,
+                       &SyncSchedulerImpl::RetryTimerCallback);
+  }

[pavely, 2014/01/27 11:54:05]

Not required for this change but I would put "received_pending_retry_ = false;"
here to show that all actions were performed and next call to
ProcessReceivedGURetryDelay will be no-op.

+}
+
+void SyncSchedulerImpl::ClearReceivedGuRetryDelayFlag() {
+  received_pending_retry_ = false;
 }
 
 void SyncSchedulerImpl::SetNotificationsEnabled(bool notifications_enabled) {