sync: GU retry with less explicit TimeTicks logic

sync/sessions/nudge_tracker.cc

 // Copyright (c) 2013 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/sessions/nudge_tracker.h"
 
 #include "base/basictypes.h"
 #include "sync/internal_api/public/base/invalidation.h"
 #include "sync/notifier/invalidation_util.h"
 #include "sync/notifier/object_id_invalidation_map.h"
@@ skipping 26 matching lines @@
 bool NudgeTracker::IsSyncRequired() const {
   for (TypeTrackerMap::const_iterator it = type_trackers_.begin();
        it != type_trackers_.end(); ++it) {
     if (it->second.IsSyncRequired()) {
       return true;
     }
   }
   return false;
 }
 
-bool NudgeTracker::IsGetUpdatesRequired(base::TimeTicks now) const {
+bool NudgeTracker::IsGetUpdatesRequired() const {
   if (invalidations_out_of_sync_)
     return true;
 
-  if (IsRetryRequired(now))
+  if (IsRetryRequired())
     return true;
 
   for (TypeTrackerMap::const_iterator it = type_trackers_.begin();
        it != type_trackers_.end(); ++it) {
     if (it->second.IsGetUpdatesRequired()) {
       return true;
     }
   }
   return false;
 }
 
-bool NudgeTracker::IsRetryRequired(base::TimeTicks now) const {
-  return !next_retry_time_.is_null() && next_retry_time_ < now;
+bool NudgeTracker::IsRetryRequired() const {
+  if (sync_cycle_start_time_.is_null())
+    return false;
+
+  if (current_retry_time_.is_null())
+    return false;
+
+  return current_retry_time_ < sync_cycle_start_time_;
 }
 
-void NudgeTracker::RecordSuccessfulSyncCycle(base::TimeTicks now) {
+void NudgeTracker::RecordSuccessfulSyncCycle() {
   updates_source_ = sync_pb::GetUpdatesCallerInfo::UNKNOWN;
-  last_successful_sync_time_ = now;
 
-  if (next_retry_time_ < now)
-    next_retry_time_ = base::TimeTicks();
+  // If a retry was required, we've just serviced it.  Unset the flag.
+  if (IsRetryRequired())
+    current_retry_time_ = base::TimeTicks();
 
   // A successful cycle while invalidations are enabled puts us back into sync.
   invalidations_out_of_sync_ = !invalidations_enabled_;
 
   for (TypeTrackerMap::iterator it = type_trackers_.begin();
        it != type_trackers_.end(); ++it) {
     it->second.RecordSuccessfulSyncCycle();
   }
 }
 
@@ skipping 149 matching lines @@
     sync_pb::GetUpdateTriggers* msg) const {
   DCHECK(type_trackers_.find(type) != type_trackers_.end());
 
   // Fill what we can from the global data.
   msg->set_invalidations_out_of_sync(invalidations_out_of_sync_);
 
   // Delegate the type-specific work to the DataTypeTracker class.
   type_trackers_.find(type)->second.FillGetUpdatesTriggersMessage(msg);
 }
 
+void NudgeTracker::SetSyncCycleStartTime(base::TimeTicks now) {
+  sync_cycle_start_time_ = now;
+
+  // We store updates to our GU retry timer in next_retry_time_.  The one
+  // currently in effect is stored in current_retry_time_.  Here is where we
+  // apply the pending update from next_retry_time_ to current_retry_time_.  The
+  // current_retry_time_ will be overwritten in the process.
+  if (!next_retry_time_.is_null()) {

[haitaol1, 2014/01/28 23:07:23]

Probably don't set current_retry_time if it's not null, which means last retry
GU failed. So current_retry_time always stores the time of earliest retry that
hasn't succeeded.

[rlarocque, 2014/01/28 23:17:09]

Correct.  That was my intention.  At least, that's the behavior that makes the
most sense to me.

If a retry GU fails, then we'll go into exponential backoff.  It would make
sense to me that IsRetryRequired() will be true when that backoff expires and we
make another attempt at the GU retry.

Leaving current_retry_time_ alone in the failure case seems to me a not-too-bad
way of achieving that goal.

Am I missing some obvious side-effects of this choice?  Or is my understanding
of the desired behavior wrong?

+    current_retry_time_ = next_retry_time_;
+    next_retry_time_ = base::TimeTicks();
+  }
+}
+
 void NudgeTracker::SetHintBufferSize(size_t size) {
   for (TypeTrackerMap::iterator it = type_trackers_.begin();
        it != type_trackers_.end(); ++it) {
     it->second.UpdatePayloadBufferSize(size);
   }
 }
 
+void NudgeTracker::SetNextRetryTime(base::TimeTicks retry_time) {
+  next_retry_time_ = retry_time;
+}
+
 }  // namespace sessions
 }  // namespace syncer