Client-side changes to support retry GU.

sync/sessions/nudge_tracker.cc

Index: sync/sessions/nudge_tracker.cc
diff --git a/sync/sessions/nudge_tracker.cc b/sync/sessions/nudge_tracker.cc
index d109294a2170c34f52cf7af1d3b683e67758719d..25ee2bc932378dcca28e50d6fb38df4b86b07398 100644
--- a/sync/sessions/nudge_tracker.cc
+++ b/sync/sessions/nudge_tracker.cc
@@ -15,8 +15,9 @@ 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();
@@ -56,8 +57,16 @@ bool NudgeTracker::IsGetUpdatesRequired() const {
   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_;