[Sync] Sync types never recovers from throttle

components/sync/engine_impl/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 "components/sync/engine_impl/sync_scheduler_impl.h"
 
 #include <algorithm>
 #include <cstring>
 #include <utility>
 
@@ skipping 603 matching lines @@
   SDVLOG(1) << "Updating polling delay to " << poll_delay.InMinutes()
             << " minutes.";
 
   // Adjust poll rate. Start will reset the timer if it was already running.
   poll_timer_.Start(FROM_HERE, poll_delay, this,
                     &SyncSchedulerImpl::PollTimerCallback);
 }
 
 void SyncSchedulerImpl::RestartWaiting() {
   if (wait_interval_.get()) {
     // Global throttling or backoff

[skym, 2017/04/21 16:06:34]

Can you add a comment explaining why you overwrite existing
pending_wakeup_timer_ jobs. Also punctuation :)

[Gang Wu, 2017/04/25 00:52:05]

Done.

     NotifyRetryTime(base::Time::Now() + wait_interval_->length);
     SDVLOG(2) << "Starting WaitInterval timer of length "
               << wait_interval_->length.InMilliseconds() << "ms.";
     if (wait_interval_->mode == WaitInterval::THROTTLED) {
       pending_wakeup_timer_.Start(FROM_HERE, wait_interval_->length,
                                   base::Bind(&SyncSchedulerImpl::Unthrottle,
                                              weak_ptr_factory_.GetWeakPtr()));
     } else {
       pending_wakeup_timer_.Start(
           FROM_HERE, wait_interval_->length,
           base::Bind(&SyncSchedulerImpl::ExponentialBackoffRetry,
                      weak_ptr_factory_.GetWeakPtr()));
     }
   } else if (nudge_tracker_.IsAnyTypeBlocked()) {
     // Per-datatype throttled or backed off.
     TimeDelta time_until_next_unblock =
         nudge_tracker_.GetTimeUntilNextUnblock();
+    TimeTicks incoming_run_time = TimeTicks::Now() + time_until_next_unblock;

[skym, 2017/04/21 16:06:34]

I think this logic already exists in this file, maybe refactor into a helper
method?

[Gang Wu, 2017/04/25 00:52:05]

Done.

+    if (pending_wakeup_timer_.IsRunning() &&
+        (pending_wakeup_timer_.desired_run_time() < incoming_run_time)) {
+      // Old job arrives sooner than this one.  Don't reschedule it.
+      return;
+    }
     pending_wakeup_timer_.Start(FROM_HERE, time_until_next_unblock,
                                 base::Bind(&SyncSchedulerImpl::OnTypesUnblocked,
                                            weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
 void SyncSchedulerImpl::Stop() {
   DCHECK(CalledOnValidThread());
   SDVLOG(2) << "Stop called";
 
@@ skipping 74 matching lines @@
   CHECK(!syncer_->IsSyncing());
 
   TrySyncCycleJob();
 }
 
 void SyncSchedulerImpl::RetryTimerCallback() {
   TrySyncCycleJob();
 }
 
 void SyncSchedulerImpl::Unthrottle() {
   DCHECK(CalledOnValidThread());

[skym, 2017/04/21 16:06:34]

Can you audit these CalledOnValidThread()? I think they should definitely be in
public methods, which we're missing in some places. Also, I think you could
argue we want them in methods that we pass as references outside of this class,
and I think you could argue we shouldn't (one shot timer always calls on current
task runner). Lets be consistent though. This method checks, PerformDelayedNudge
does not.

[Gang Wu, 2017/04/25 00:52:05]

Done.

   DCHECK_EQ(WaitInterval::THROTTLED, wait_interval_->mode);
 
   // We're no longer throttled, so clear the wait interval.
   wait_interval_.reset();
   NotifyRetryTime(base::Time());
   NotifyBlockedTypesChanged(nudge_tracker_.GetBlockedTypes());
 
+  // There may have some datatypes are in backoff or throttled before this
+  // global throttling, so rechedule for them.
+  RestartWaiting();

[skym, 2017/04/21 16:06:34]

Sprinkling RestartWaiting()s all over the place, and then when the next
developer comes in here and makes changes and needs to update things... seems
really error prone. I think it'd be better if the pending_wakeup_timer_ was
always scheduled with the same method, that has a priority curried into it, and
only it needs to call RestartWaiting() at the end.

I'd also like to see the whole block that calls RestartWaiting removed from
SyncSchedulerImpl::TrySyncCycleJobImpl(). Yes, changes can jump in between
posting and running TrySyncCycleJobImpl(), but it shouldn't be responsible for
reacting to these kinds of things.

[Gang Wu, 2017/04/25 00:52:05]

Done.

+
   // We treat this as a 'canary' in the sense that it was originally scheduled
   // to run some time ago, failed, and we now want to retry, versus a job that
   // was just created (e.g via ScheduleNudgeImpl). The main implication is
   // that we're careful to update routing info (etc) with such potentially
   // stale canary jobs.
   TryCanaryJob();
 }
 
 void SyncSchedulerImpl::OnTypesUnblocked() {
   DCHECK(CalledOnValidThread());
   nudge_tracker_.UpdateTypeThrottlingAndBackoffState();

[skym, 2017/04/21 16:06:34]

This function makes no sense to me. I am unable to follow it. Why doesn't
unblock_time_ get reset if we're now is past unblock_time_ but we're also in
exponential backoff?

[Gang Wu, 2017/04/25 00:52:05]

Each datatype may have different backoff time, for example, BOOKMARK may be got
throttled at 1pm for 2 hours, and SESSION bay got throttled at 2pm for 3 hours.
So when time goes to 3pm, also BOOKMARK will be unblocked, but SESSIONS is
blocked still, and need to schedule another unblock job for SESSIONS. 

[skym, 2017/04/25 23:16:17]

Yes, but |unblock_time_| is inside of DataTypeTracker, it's already scoped to a
single data type.

[Gang Wu, 2017/04/29 15:06:16]

You mean something like BOOKMARK got in throttling and exponential backoff? no,
that is not gonna happen. Once a datatype got in one of status of throttling or
backoff, client will not sent any update for that datatype, then the datatype
will not receive another error status.

[skym, 2017/05/01 17:59:14]

No, I'm saying that the code checks |unblock_time_| for both exponential backoff
and throttling cases, but it only clears it for throttling. Why does it clear it
for (and only for) throttling?

[Gang Wu, 2017/05/01 23:01:40]

oh, I see. we cannot clear for backoff case since if we clear backoff state, it
is hard to exponential. 
Throttling is cleared because throttling is one time thing, so un-throttling
every time when throttling is expired.

   NotifyBlockedTypesChanged(nudge_tracker_.GetBlockedTypes());
 
   RestartWaiting();

[skym, 2017/04/21 16:06:34]

Why isn't RestartWaiting() the last thing done in this function?

[Gang Wu, 2017/04/25 00:52:06]

Done.

 
   // Maybe this is a good time to run a nudge job.  Let's try it.
   if (nudge_tracker_.IsSyncRequired() && CanRunNudgeJobNow(NORMAL_PRIORITY))
     TrySyncCycleJob();
 }
 
 void SyncSchedulerImpl::PerformDelayedNudge() {
   // Circumstances may have changed since we scheduled this delayed nudge.
   // We must check to see if it's OK to run the job before we do so.
   if (CanRunNudgeJobNow(NORMAL_PRIORITY))
     TrySyncCycleJob();
 
-  // We're not responsible for setting up any retries here.  The functions that
-  // first put us into a state that prevents successful sync cycles (eg. global
-  // throttling, type throttling, network errors, transient errors) will also
-  // setup the appropriate retry logic (eg. retry after timeout, exponential
-  // backoff, retry when the network changes).
+  // Since PerformDelayedNudge share pending_wakeup_timer_ with
+  // OnTypesUnblocked, Unthrottle and ExponentialBackoffRetry, we should check
+  // if there are any of those functions needed to be scheduled.
+  RestartWaiting();

[skym, 2017/04/21 16:06:34]

Why isn't RestartWaiting() the last thing done in this function?

[Gang Wu, 2017/04/25 00:52:05]

Where to run RestartWaiting() is not a matter, we just schedule timer
somewhere is fine.
Anyway, I remove RestartWaiting() from all place and put at the end of
TrySyncCycleJobImpl. 

 }
 
 void SyncSchedulerImpl::ExponentialBackoffRetry() {
   TryCanaryJob();
+
+  if (!IsBackingOff()) {
+    // There may have some datatypes are in backoff or throttled before this
+    // global backoff, so rechedule for them.
+    RestartWaiting();
+  }
 }
 
 void SyncSchedulerImpl::NotifyRetryTime(base::Time retry_time) {
   for (auto& observer : *cycle_context_->listeners())
     observer.OnRetryTimeChanged(retry_time);
 }
 
 void SyncSchedulerImpl::NotifyBlockedTypesChanged(ModelTypeSet types) {
   ModelTypeSet throttled_types;
   ModelTypeSet backed_off_types;
@@ skipping 113 matching lines @@
   }
   if (IsActionableError(sync_protocol_error)) {
     SDVLOG(2) << "OnActionableError";
     for (auto& observer : *cycle_context_->listeners())
       observer.OnActionableError(sync_protocol_error);
   }
 }
 
 void SyncSchedulerImpl::OnReceivedGuRetryDelay(const TimeDelta& delay) {
   nudge_tracker_.SetNextRetryTime(TimeTicks::Now() + delay);
   retry_timer_.Start(FROM_HERE, delay, this,

[skym, 2017/04/21 16:06:34]

Why do we have a separate timer for this? Why is this not also using the same
shared timer?

[Gang Wu, 2017/04/25 00:52:05]

There are a lot of logic here.
poll_timer_ is for poll jobs, which will run every 8 or 12 hours, or
defined by server.
retry_timer_ is for retry jobs, which is set by server. when client received a
command from server, client will schedule a job for it.

There are three of timers, and a lot of logic here, I do not think I can
refactor them in
this CL, so I create a TODO for it.
 

                      &SyncSchedulerImpl::RetryTimerCallback);
 }
 
 void SyncSchedulerImpl::OnReceivedMigrationRequest(ModelTypeSet types) {
   for (auto& observer : *cycle_context_->listeners())
     observer.OnMigrationRequested(types);
 }
 
 void SyncSchedulerImpl::SetNotificationsEnabled(bool notifications_enabled) {
   DCHECK(CalledOnValidThread());
   cycle_context_->set_notifications_enabled(notifications_enabled);
   if (notifications_enabled)
     nudge_tracker_.OnInvalidationsEnabled();
   else
     nudge_tracker_.OnInvalidationsDisabled();
 }
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // namespace syncer