Client-side changes to support retry GU.

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),
+NudgeTracker::NudgeTracker(base::Clock* clock)
+    : clock_(clock),
+      updates_source_(sync_pb::GetUpdatesCallerInfo::UNKNOWN),
       invalidations_enabled_(false),
       invalidations_out_of_sync_(true) {
   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 {
@@ skipping 19 matching lines @@
     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() const {
+  return !next_retry_time_.is_null() && next_retry_time_ < clock_->Now();

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

I'd strongly prefer to have the base::Time value passed in rather than have a
clock_ member.  Making the current time a function parameter makes testing
easier and allows us more flexibility in responding to timeouts.

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

I feel the other way. Using a clock member has the same effect in test and is
more consistent than passing clock every time. Also it makes the interface
cleaner.

On 2014/01/06 23:00:33, rlarocque wrote:

[rlarocque, 2014/01/07 19:43:41]

It's not quite the same, though.

One trick we can use to make timer behavior very deterministic is to not query
the time when a timeout expires, but instead to assume the current time is
exactly when the timeout was scheduled for.

Consider three functions:

Example 1.

void onTimeout(base::Time scheduled_expiry) {
  if (nudge_tracker.IsRetryRequired(scheduled_expiry)) {
    ...
  }
}

Example 2.

void onTimeout() {
  if (nudge_tracker.IsRetryRequired(base::Time::Now())) {
    ...
  }
}

Example 3.

void onTimeout() {
  if (nudge_tracker.IsRetryRequired()) {
    ...
  }
}

The first and second forms are subtly different.  In the first case, we can bind
the expiry time to the timeout expiry function and guarantee that late timeouts
will not affect behavior in any way.  That can be a useful property, since it
prevents us from relying on timing assumptions.  

If we end up with more complicated scenarios, for example where one timeout
expiry results in a task that posts another timer for later, then this sort of
deterministic behavior could make it easier to test.  There are cases where it
would be hard to reset the mock clock's time part way through the test.

I'm currently using something similar to example 1 in
SyncSchedulerImpl::TypeUnthrottle().

It's probably not necessary to use the form from example 1 in your code. 
However, I think you should at least use the form from example 2, just to make
it easier to switch to something like example 1 in the future.

+}
+
 void NudgeTracker::RecordSuccessfulSyncCycle() {
   updates_source_ = sync_pb::GetUpdatesCallerInfo::UNKNOWN;
+  last_successful_sync_time_ = clock_->Now();
+
+  if (next_retry_time_ < clock_->Now())
+    next_retry_time_ = base::Time();
 
   // 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 158 matching lines @@
 
 void NudgeTracker::SetHintBufferSize(size_t size) {
   for (TypeTrackerMap::iterator it = type_trackers_.begin();
        it != type_trackers_.end(); ++it) {
     it->second.UpdatePayloadBufferSize(size);
   }
 }
 
 }  // namespace sessions
 }  // namespace syncer