sync: Fix handling of data type throttling

This change attempts to fix two semi-related issues in data type throttling that were introduced in r194766. Data type throttling has always had a few rough edge cases, but that commit introduced some issues that must be fixed. 1: Prior to r194766, data type throttling would block sync cycles only if the source was LOCAL and all types were throttled. This meant that a refresh request or notification could override data type throttling, if it arrived at the right time. That change made this behaviour much more deterministic. It would fail to perform any sync cycles as long as the only local nudges that were currently tracked were for throttled types. Even notificiations and local refresh requests for these types will not be sufficient to force a sync cycle. This change tries to find a better balance. It will allow sync cycles in the presence of unthrottled data types if one of the following is true: - There is an unserviced invalidation for any type. - There is an unservice refresh request for any type. - There is a local nudge for any non-throttled type. 2: The other problem has to do with exponential backoff in the presence of per-type throttling. In r194766, we made the assumption that we can early-exit from attempted sync cycle only if there was some other reason to perform a sync cycle later. Common causes of early-exit include throttling, exponential backoff, and network connectivity issues, all of which should be able to get back into a normal state on their own. However, one of those early-exit causes is data type throttling. In that case, there is no timer or notification that will restart sync once the issue is resolved. This is a problem because DoNudgeSyncSessionJob assumes that it can return without scheduling a retry in the "if (!CanRunNudgeJobNow())" case, while the code that manages the exponential backoff assumes the function that invokes DoNudgeSyncSessionJob, TryCanaryJob(), will always either clear or restart the wait interval. To fix this issue, this change adds a small function to invoke the exponential backoff retry callback, and gives it responsibility for making sure the WaitInterval is always managed correctly. BUG=239693 Review URL: https://chromiumcodereview.appspot.com/14710006 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@200655 0039d316-1c4b-4281-b951-d872f2087c98
author: rlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2013-05-16 23:37:30 +0000
committer: rlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2013-05-16 23:37:30 +0000
commit: 580cda659ad02527243597e3311581d3368d7432 (patch)
tree: 04c2d79c1aee579b6f317bd200dbcd18ed52aad6
parent: 9eb51473ac82c68749a977bc7b303ffab5c4ceae (diff)
download: chromium_src-580cda659ad02527243597e3311581d3368d7432.zip
chromium_src-580cda659ad02527243597e3311581d3368d7432.tar.gz
chromium_src-580cda659ad02527243597e3311581d3368d7432.tar.bz2
7 files changed, 167 insertions, 27 deletions
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index 626a695..55adcc5 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -334,13 +334,25 @@ bool SyncSchedulerImpl::CanRunNudgeJobNow(JobPriority priority) {
     return false;
   }
 
-  // If all types are throttled, do not continue.  Today, we don't treat a
-  // per-datatype "unthrottle" event as something that should force a canary
-  // job. For this reason, there's no good time to reschedule this job to run
-  // -- we'll lazily wait for an independent event to trigger a sync.
+  // Per-datatype throttling will prevent a sync cycle if the only reason to
+  // perform this cycle is to commit a local change.  If there is any other
+  // reason to perform the sync cycle (such as an invalidation for the throttled
+  // type or a local change to non-throttled type) then the cycle will be
+  // allowed to proceed.
+  //
+  // If the cycle is prevented, we will drop it without scheduling another.
+  // Unlike global throttling, we have no timer to tell us when to wake us up
+  // when a type is unthrottled.
+  //
+  // If the sync cycle is not prevented, the commit message will be filtered to
+  // prevent the commit of any items of the throttled data type.  However, if
+  // the sync cycle succeeds, the nudge tracker will be told to assume that all
+  // outstanding commits (including the ones for throttled types) were completed
+  // successfully.  It does not yet have the ability to selectively clear the
+  // uncommitted status of only some types.
   ModelTypeSet throttled_types =
       session_context_->throttled_data_type_tracker()->GetThrottledTypes();
-  if (!nudge_tracker_.GetLocallyModifiedTypes().Empty() &&
+  if (!nudge_tracker_.IsGetUpdatesRequired() &&
       throttled_types.HasAll(nudge_tracker_.GetLocallyModifiedTypes())) {
     // TODO(sync): Throttled types should be pruned from the sources list.
     SDVLOG(1) << "Not running a nudge because we're fully datatype throttled.";
@@ -378,7 +390,7 @@ void SyncSchedulerImpl::ScheduleLocalRefreshRequest(
   DCHECK(!types.Empty());
 
   SDVLOG_LOC(nudge_location, 2)
-      << "Scheduling sync because of local refresch request for "
+      << "Scheduling sync because of local refresh request for "
       << ModelTypeSetToString(types);
   nudge_tracker_.RecordLocalRefreshRequest(types);
   ScheduleNudgeImpl(desired_delay, nudge_location);
@@ -543,8 +555,12 @@ void SyncSchedulerImpl::HandleFailure(
   if (IsSyncingCurrentlySilenced()) {
     SDVLOG(2) << "Was throttled during previous sync cycle.";
     RestartWaiting();
-  } else {
-    UpdateExponentialBackoff(model_neutral_state);
+  } else if (!IsBackingOff()) {
+    // Setup our backoff if this is our first such failure.
+    TimeDelta length = delay_provider_->GetDelay(
+        delay_provider_->GetInitialDelay(model_neutral_state));
+    wait_interval_.reset(
+        new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF, length));
     SDVLOG(2) << "Sync cycle failed.  Will back off for "
         << wait_interval_->length.InMilliseconds() << "ms.";
     RestartWaiting();
@@ -638,22 +654,11 @@ void SyncSchedulerImpl::RestartWaiting() {
     pending_wakeup_timer_.Start(
         FROM_HERE,
         wait_interval_->length,
-        base::Bind(&SyncSchedulerImpl::TryCanaryJob,
+        base::Bind(&SyncSchedulerImpl::ExponentialBackoffRetry,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
-void SyncSchedulerImpl::UpdateExponentialBackoff(
-    const sessions::ModelNeutralState& model_neutral_state) {
-  DCHECK(CalledOnValidThread());
-
-  TimeDelta length = delay_provider_->GetDelay(
-      IsBackingOff() ? wait_interval_->length :
-          delay_provider_->GetInitialDelay(model_neutral_state));
-  wait_interval_.reset(new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF,
-                                        length));
-}
-
 void SyncSchedulerImpl::RequestStop(const base::Closure& callback) {
   syncer_->RequestEarlyExit();  // Safe to call from any thread.
   DCHECK(weak_handle_this_.IsInitialized());
@@ -728,6 +733,22 @@ void SyncSchedulerImpl::Unthrottle() {
   TryCanaryJob();
 }
 
+void SyncSchedulerImpl::ExponentialBackoffRetry() {
+  TryCanaryJob();
+
+  if (IsBackingOff()) {
+    // If we succeeded, our wait interval would have been cleared.  If it hasn't
+    // been cleared, then we should increase our backoff interval and schedule
+    // another retry.
+    TimeDelta length = delay_provider_->GetDelay(wait_interval_->length);
+    wait_interval_.reset(
+      new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF, length));
+    SDVLOG(2) << "Sync cycle failed.  Will back off for "
+        << wait_interval_->length.InMilliseconds() << "ms.";
+    RestartWaiting();
+  }
+}
+
 void SyncSchedulerImpl::Notify(SyncEngineEvent::EventCause cause) {
   DCHECK(CalledOnValidThread());
   session_context_->NotifyListeners(SyncEngineEvent(cause));
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index d5ee56c..46f5c35 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -172,10 +172,6 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   // Helper to restart waiting with |wait_interval_|'s timer.
   void RestartWaiting();
 
-  // Helper to adjust our wait interval when we expereince a transient failure.
-  void UpdateExponentialBackoff(
-      const sessions::ModelNeutralState& model_neutral_state);
-
   // Determines if we're allowed to contact the server right now.
   bool CanRunJobNow(JobPriority priority);
 
@@ -209,6 +205,9 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   // Transitions out of the THROTTLED WaitInterval then calls TryCanaryJob().
   void Unthrottle();
 
+  // Attempts to exit EXPONENTIAL_BACKOFF by calling TryCanaryJob().
+  void ExponentialBackoffRetry();
+
   // Called when the root cause of the current connection error is fixed.
   void OnServerConnectionErrorFixed();
 
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 21e1430..3820102 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -225,6 +225,10 @@ class SyncSchedulerTest : public testing::Test {
 
   SyncSessionContext* context() { return context_.get(); }
 
+  ThrottledDataTypeTracker* throttled_data_type_tracker() {
+    return throttled_data_type_tracker_.get();
+  }
+
  private:
   syncable::Directory* directory() {
     return dir_maker_.directory();
@@ -778,6 +782,89 @@ TEST_F(SyncSchedulerTest, ThrottlingExpiresFromConfigure) {
   StopSyncScheduler();
 }
 
+TEST_F(SyncSchedulerTest, TypeThrottlingBlocksNudge) {
+  UseMockDelayProvider();
+  EXPECT_CALL(*delay(), GetDelay(_))
+      .WillRepeatedly(Return(zero()));
+
+  TimeDelta poll(TimeDelta::FromDays(1));
+  TimeDelta throttle1(TimeDelta::FromSeconds(60));
+  scheduler()->OnReceivedLongPollIntervalUpdate(poll);
+
+  const ModelTypeSet types(BOOKMARKS);
+
+  ::testing::InSequence seq;
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(DoAll(
+          WithArg<0>(
+              sessions::test_util::SimulateTypesThrottled(types, throttle1)),
+          Return(true)))
+      .RetiresOnSaturation();
+
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+  scheduler()->ScheduleLocalNudge(zero(), types, FROM_HERE);
+  PumpLoop();
+  EXPECT_TRUE(throttled_data_type_tracker()->GetThrottledTypes().HasAll(types));
+
+  // This won't cause a sync cycle because the types are throttled.
+  scheduler()->ScheduleLocalNudge(zero(), types, FROM_HERE);
+  PumpLoop();
+
+  StopSyncScheduler();
+}
+
+TEST_F(SyncSchedulerTest, TypeThrottlingDoesntBlockOtherSources) {
+  UseMockDelayProvider();
+  EXPECT_CALL(*delay(), GetDelay(_))
+      .WillRepeatedly(Return(zero()));
+
+  SyncShareRecords records;
+  TimeDelta poll(TimeDelta::FromDays(1));
+  TimeDelta throttle1(TimeDelta::FromSeconds(60));
+  scheduler()->OnReceivedLongPollIntervalUpdate(poll);
+
+  const ModelTypeSet throttled_types(BOOKMARKS);
+  const ModelTypeSet unthrottled_types(PREFERENCES);
+
+  ::testing::InSequence seq;
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(DoAll(
+          WithArg<0>(
+              sessions::test_util::SimulateTypesThrottled(
+                  throttled_types, throttle1)),
+          Return(true)))
+      .RetiresOnSaturation();
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+                            WithArg<0>(RecordSyncShare(&records))));
+
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+  scheduler()->ScheduleLocalNudge(zero(), throttled_types, FROM_HERE);
+  PumpLoop();
+  EXPECT_EQ(0U, records.snapshots.size());
+  EXPECT_TRUE(throttled_data_type_tracker()->GetThrottledTypes().HasAll(
+          throttled_types));
+
+  // This invalidaiton will cause a sync even though the types are throttled.
+  ModelTypeInvalidationMap invalidation_map =
+      ModelTypeSetToInvalidationMap(throttled_types, "test");
+  scheduler()->ScheduleInvalidationNudge(zero(), invalidation_map, FROM_HERE);
+  RunLoop();
+  EXPECT_EQ(1U, records.snapshots.size());
+
+  // Refresh requests will cause a sync, too.
+  scheduler()->ScheduleLocalRefreshRequest(zero(), throttled_types, FROM_HERE);
+  RunLoop();
+  EXPECT_EQ(2U, records.snapshots.size());
+
+  // Even local nudges for other data types will trigger a sync.
+  scheduler()->ScheduleLocalNudge(zero(), unthrottled_types, FROM_HERE);
+  RunLoop();
+  EXPECT_EQ(3U, records.snapshots.size());
+
+  StopSyncScheduler();
+}
+
 // Test nudges / polls don't run in config mode and config tasks do.
 TEST_F(SyncSchedulerTest, ConfigurationMode) {
   TimeDelta poll(TimeDelta::FromMilliseconds(15));
diff --git a/sync/sessions/nudge_tracker.cc b/sync/sessions/nudge_tracker.cc
index 7eae5ec..b481a39 100644
--- a/sync/sessions/nudge_tracker.cc
+++ b/sync/sessions/nudge_tracker.cc
@@ -21,12 +21,20 @@ NudgeTracker::NudgeTracker()
 
 NudgeTracker::~NudgeTracker() { }
 
+bool NudgeTracker::IsGetUpdatesRequired() {
+  if (!refresh_requested_counts_.empty()) {
+    return true;
+  } else if (!payload_list_map_.empty()) {
+    return true;
+  } else {
+    return false;
+  }
+}
+
 bool NudgeTracker::IsSyncRequired() {
   if (!local_nudge_counts_.empty()) {
     return true;
-  } else if (!refresh_requested_counts_.empty()) {
-    return true;
-  } else if (!payload_list_map_.empty()) {
+  } else if (IsGetUpdatesRequired()) {
     return true;
   } else {
     return false;
diff --git a/sync/sessions/nudge_tracker.h b/sync/sessions/nudge_tracker.h
index c093250..0e557f3 100644
--- a/sync/sessions/nudge_tracker.h
+++ b/sync/sessions/nudge_tracker.h
@@ -28,6 +28,11 @@ class SYNC_EXPORT_PRIVATE NudgeTracker {
   NudgeTracker();
   ~NudgeTracker();
 
+  // Returns true if one of the main reasons for performing the sync cycle is to
+  // fetch updates.  This is true when we have pending invalidations or refresh
+  // requests.
+  bool IsGetUpdatesRequired();
+
   // Returns true if there is a good reason for performing a sync cycle.
   // This does not take into account whether or not this is a good *time* to
   // perform a sync cycle; that's the scheduler's job.
diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc
index 3b4b6c9..03f6de2 100644
--- a/sync/sessions/test_util.cc
+++ b/sync/sessions/test_util.cc
@@ -4,6 +4,8 @@
 
 #include "sync/sessions/test_util.h"
 
+#include "sync/engine/throttled_data_type_tracker.h"
+
 namespace syncer {
 namespace sessions {
 namespace test_util {
@@ -64,6 +66,16 @@ void SimulateThrottledImpl(sessions::SyncSession* session,
   session->delegate()->OnSilencedUntil(base::TimeTicks::Now() + delta);
 }
 
+void SimulateTypesThrottledImpl(
+    sessions::SyncSession* session,
+    ModelTypeSet types,
+    const base::TimeDelta& delta) {
+  session->mutable_status_controller()->set_last_download_updates_result(
+      SERVER_RETURN_THROTTLED);
+  session->context()->throttled_data_type_tracker()->
+      SetUnthrottleTime(types, base::TimeTicks::Now() + delta);
+}
+
 void SimulatePollIntervalUpdateImpl(sessions::SyncSession* session,
     const base::TimeDelta& new_poll) {
   SimulateSuccess(session, SYNCER_BEGIN, SYNCER_END);
diff --git a/sync/sessions/test_util.h b/sync/sessions/test_util.h
index 38638e1..6ab32b1 100644
--- a/sync/sessions/test_util.h
+++ b/sync/sessions/test_util.h
@@ -27,6 +27,10 @@ void SimulateSuccess(sessions::SyncSession* session,
                      SyncerStep begin, SyncerStep end);
 void SimulateThrottledImpl(sessions::SyncSession* session,
     const base::TimeDelta& delta);
+void SimulateTypesThrottledImpl(
+    sessions::SyncSession* session,
+    ModelTypeSet types,
+    const base::TimeDelta& delta);
 void SimulatePollIntervalUpdateImpl(sessions::SyncSession* session,
     const base::TimeDelta& new_poll);
 void SimulateSessionsCommitDelayUpdateImpl(sessions::SyncSession* session,
@@ -36,6 +40,10 @@ ACTION_P(SimulateThrottled, throttle) {
   SimulateThrottledImpl(arg0, throttle);
 }
 
+ACTION_P2(SimulateTypesThrottled, types, throttle) {
+  SimulateTypesThrottledImpl(arg0, types, throttle);
+}
+
 ACTION_P(SimulatePollIntervalUpdate, poll) {
   SimulatePollIntervalUpdateImpl(arg0, poll);
 }
author	rlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2013-05-16 23:37:30 +0000
committer	rlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2013-05-16 23:37:30 +0000
commit	580cda659ad02527243597e3311581d3368d7432 (patch)
tree	04c2d79c1aee579b6f317bd200dbcd18ed52aad6
parent	9eb51473ac82c68749a977bc7b303ffab5c4ceae (diff)
download	chromium_src-580cda659ad02527243597e3311581d3368d7432.zip chromium_src-580cda659ad02527243597e3311581d3368d7432.tar.gz chromium_src-580cda659ad02527243597e3311581d3368d7432.tar.bz2