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"
 #include "sync/protocol/sync.pb.h"
 
 namespace syncer {
 namespace sessions {
 
 size_t NudgeTracker::kDefaultMaxPayloadsPerType = 10;
 
 NudgeTracker::NudgeTracker()
     : updates_source_(sync_pb::GetUpdatesCallerInfo::UNKNOWN),
       invalidations_enabled_(false),
-      invalidations_out_of_sync_(true) {
+      invalidations_out_of_sync_(true),
+      is_retry_required_(false) {
   ModelTypeSet protocol_types = ProtocolTypes();
   // Default initialize all the type trackers.
   for (ModelTypeSet::Iterator it = protocol_types.First(); it.Good();
        it.Inc()) {
     invalidation::ObjectId id;
     if (!RealModelTypeToObjectId(it.Get(), &id)) {
       NOTREACHED();
     } else {
       type_trackers_.insert(std::make_pair(it.Get(), DataTypeTracker(id)));
     }
   }
 }
 
 NudgeTracker::~NudgeTracker() { }
 
 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 {
+  return is_retry_required_;
 }
 
-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.
+  is_retry_required_ = false;
+
+  // We don't unset the next_retry_time_ flag here, since it may have been
+  // updated during the sync cycle that just completed.
 
   // 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::ToggleRetryFlagIfRequired(base::TimeTicks now) {
+  // Assume we have nothing to do.
+  is_retry_required_ = false;
+
+  // Nothing to do if no retries are scheduled.
+  if (next_retry_time_.is_null())
+    return;
+
+  if (next_retry_time_ < now) {

[haitaol1, 2014/01/28 20:09:20]

where is next_retry_time nulled? Seems nudge tracker is stuck in retry without
that.

I feel storing start time is easy and may be useful in the future, too.

[rlarocque, 2014/01/28 22:24:38]

Thanks!

I had a nagging feeling that this code was wrong somehow, since I had concluded
on Friday that it was impossible to write this code correctly with so little
state.  But my unit tests passed and seemed to cover all the cases, so I figured
it must be correct...  Looks like I *was* missing something obvious.

I've updated the tests to catch this.

+    // The time has come.  We should perform a GU retry as soon as possible.
+    is_retry_required_ = true;
+  }
+}
+
 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