sync: GU retry with less explicit TimeTicks logic

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 216 matching lines @@
   }
 
   DCHECK(!session_context_->account_name().empty());
   DCHECK(syncer_.get());
   Mode old_mode = mode_;
   mode_ = mode;
   AdjustPolling(UPDATE_INTERVAL);  // Will kick start poll timer if needed.
 
   if (old_mode != mode_ &&
       mode_ == NORMAL_MODE &&
-      (nudge_tracker_.IsSyncRequired() ||
-          nudge_tracker_.IsRetryRequired(base::TimeTicks::Now())) &&
+      (nudge_tracker_.IsSyncRequired() || nudge_tracker_.IsRetryRequired()) &&

[pavely, 2014/01/29 15:35:05]

SetSyncCycleStartTime isn't called in this case. If previous sync cycle called
SetRetryTime this call wouldn't notice it.

The semantic of variables is:
- current_retry_time : earliest unsatisfied retry time.
- next_retry_time : latest known retry time.

If the above semantic is right then SetNextRetryTime should also copy retry time
into current_retry_time if it is not set.

[rlarocque, 2014/01/29 18:27:42]

I had forgotten that this was a different code path.  I think this will need a
call to SetSyncCycleStartTime().  That would fix the behavior.

I'm not sure I agree with your suggestion about current_retry_time semantics.

I think the behavior we're trying to achieve is that the client should only act
on the most recently received retry.  There is a special case where the GU retry
attempt fails, in which case we should have the IsRetryRequired() flag set until
one of the exponential backoff attempts is successful.

It seems to me that if we set current_retry_time in SetNextRetryTime(), we won't
be able to override it if a new retry time arrives before it times out.

       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.
     TrySyncSessionJob();
   }
 }
 
 ModelTypeSet SyncSchedulerImpl::GetEnabledAndUnthrottledTypes() {
   ModelTypeSet enabled_types = session_context_->enabled_types();
   ModelTypeSet throttled_types = nudge_tracker_.GetThrottledTypes();
@@ skipping 214 matching lines @@
   // 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(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;
   } else {
     HandleFailure(session->status_controller().model_neutral_state());
   }
 }
 
 void SyncSchedulerImpl::DoConfigurationSyncSessionJob(JobPriority priority) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(mode_, CONFIGURATION_MODE);
   DCHECK(pending_configure_params_ != NULL);
 
@@ skipping 83 matching lines @@
 
   base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
 
   SDVLOG(2) << "Retrying with types "
             << ModelTypeSetToString(GetEnabledAndUnthrottledTypes());
   scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this));
   if (syncer_->RetrySyncShare(GetEnabledAndUnthrottledTypes(),
                               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());
   }
 }
 
 void SyncSchedulerImpl::UpdateNudgeTimeRecords(ModelTypeSet types) {
   DCHECK(CalledOnValidThread());
   base::TimeTicks now = TimeTicks::Now();
   // Update timing information for how often datatypes are triggering nudges.
   for (ModelTypeSet::Iterator iter = types.First(); iter.Good(); iter.Inc()) {
@@ skipping 86 matching lines @@
   // access token will be here.
   base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
       &SyncSchedulerImpl::TrySyncSessionJobImpl,
       weak_ptr_factory_.GetWeakPtr()));
 }
 
 void SyncSchedulerImpl::TrySyncSessionJobImpl() {
   JobPriority priority = next_sync_session_job_priority_;
   next_sync_session_job_priority_ = NORMAL_PRIORITY;
 
+  nudge_tracker_.SetSyncCycleStartTime(base::TimeTicks::Now());
+
   DCHECK(CalledOnValidThread());
   if (mode_ == CONFIGURATION_MODE) {
     if (pending_configure_params_) {
       SDVLOG(2) << "Found pending configure job";
       DoConfigurationSyncSessionJob(priority);
     }
   } else if (CanRunNudgeJobNow(priority)) {
     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())) {
       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()) {
@@ skipping 188 matching lines @@
   if (ShouldRequestEarlyExit(
           snapshot.model_neutral_state().sync_protocol_error)) {
     SDVLOG(2) << "Sync Scheduler requesting early exit.";
     Stop();
   }
   if (IsActionableError(snapshot.model_neutral_state().sync_protocol_error))
     OnActionableError(snapshot);
 }
 
 void SyncSchedulerImpl::OnReceivedGuRetryDelay(const base::TimeDelta& delay) {
-  nudge_tracker_.set_next_retry_time(base::TimeTicks::Now() + delay);
+  nudge_tracker_.SetNextRetryTime(TimeTicks::Now() + delay);
   retry_timer_.Start(FROM_HERE, delay, this,
                      &SyncSchedulerImpl::RetryTimerCallback);
 }
 
 void SyncSchedulerImpl::SetNotificationsEnabled(bool notifications_enabled) {
   DCHECK(CalledOnValidThread());
   session_context_->set_notifications_enabled(notifications_enabled);
   if (notifications_enabled)
     nudge_tracker_.OnInvalidationsEnabled();
   else
     nudge_tracker_.OnInvalidationsDisabled();
 }
 
 base::TimeDelta SyncSchedulerImpl::GetSessionsCommitDelay() const {
   DCHECK(CalledOnValidThread());
   return sessions_commit_delay_;
 }
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // namespace syncer