[Sync] Sync client should to exponential backoff when receive partial failure

components/sync/engine_impl/syncer_unittest.cc

 // Copyright 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/syncer.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <limits>
@@ skipping 171 matching lines @@
         last_client_invalidation_hint_buffer_size_(10) {}
 
   // SyncCycle::Delegate implementation.
   void OnThrottled(const base::TimeDelta& throttle_duration) override {
     FAIL() << "Should not get silenced.";
   }
   void OnTypesThrottled(ModelTypeSet types,
                         const base::TimeDelta& throttle_duration) override {
     scheduler_->OnTypesThrottled(types, throttle_duration);
   }
+  void OnTypesBackedOff(ModelTypeSet types) override {
+    scheduler_->OnTypesBackedOff(types);
+  }
   bool IsCurrentlyThrottled() override { return false; }
   void OnReceivedLongPollIntervalUpdate(
       const base::TimeDelta& new_interval) override {
     last_long_poll_interval_received_ = new_interval;
   }
   void OnReceivedShortPollIntervalUpdate(
       const base::TimeDelta& new_interval) override {
     last_short_poll_interval_received_ = new_interval;
   }
   void OnReceivedCustomNudgeDelays(
@@ skipping 32 matching lines @@
       case SyncCycleEvent::SYNC_CYCLE_ENDED:
         return;
       default:
         FAIL() << "Handling unknown error type in unit tests!!";
     }
   }
 
   void OnActionableError(const SyncProtocolError& error) override {}
   void OnRetryTimeChanged(base::Time retry_time) override {}
   void OnThrottledTypesChanged(ModelTypeSet throttled_types) override {}
+  void OnBackedOffTypesChanged(ModelTypeSet backed_off_types) override {}
   void OnMigrationRequested(ModelTypeSet types) override {}
 
   void ResetCycle() { cycle_.reset(SyncCycle::Build(context_.get(), this)); }
 
   bool SyncShareNudge() {
     ResetCycle();
 
     // Pretend we've seen a local change, to make the nudge_tracker look normal.
     nudge_tracker_.RecordLocalChange(ModelTypeSet(BOOKMARKS));
 
@@ skipping 360 matching lines @@
     syncable::WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
     MutableEntry A(&wtrans, GET_BY_ID, ids_.FromNumber(1));
     ASSERT_TRUE(A.good());
     A.PutIsUnsynced(true);
     A.PutSpecifics(bookmark_data);
     A.PutNonUniqueName("bookmark");
   }
 
   // Now sync without enabling bookmarks.
   mock_server_->ExpectGetUpdatesRequestTypes(
-      Difference(context_->GetEnabledTypes(), ModelTypeSet(BOOKMARKS)));
+      Difference(context_->GetEnabledTypes(), throttled_types));
   ResetCycle();
   syncer_->NormalSyncShare(
-      Difference(context_->GetEnabledTypes(), ModelTypeSet(BOOKMARKS)),
-      &nudge_tracker_, cycle_.get());
+      Difference(context_->GetEnabledTypes(), throttled_types), &nudge_tracker_,
+      cycle_.get());
 
   {
     // Nothing should have been committed as bookmarks is throttled.
     syncable::ReadTransaction rtrans(FROM_HERE, directory());
     Entry entryA(&rtrans, GET_BY_ID, ids_.FromNumber(1));
     ASSERT_TRUE(entryA.good());
     EXPECT_TRUE(entryA.GetIsUnsynced());
   }
 
   // Sync again with bookmarks enabled.
@@ skipping 271 matching lines @@
     syncable::ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, false, false, false, 0, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(2, false, false, false, 1, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(3, false, false, false, 1, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(4, false, false, false, 0, 10, 10, ids_, &rtrans);
   }
 
   // Set BOOKMARKS throttled but PREFERENCES not,
   // then BOOKMARKS should not get synced but PREFERENCES should.
   ModelTypeSet throttled_types(BOOKMARKS);
-  mock_server_->set_partial_throttling(true);
-  mock_server_->SetThrottledTypes(throttled_types);
+  mock_server_->set_throttling(true);
+  mock_server_->SetPartialFailureTypes(throttled_types);
 
   mock_server_->AddUpdateSpecifics(1, 0, "E", 20, 20, true, 0, bookmark,
                                    foreign_cache_guid(), "-1");
   mock_server_->AddUpdateSpecifics(2, 1, "F", 20, 20, false, 2, bookmark,
                                    foreign_cache_guid(), "-2");
   mock_server_->AddUpdateSpecifics(3, 1, "G", 20, 20, false, 1, bookmark,
                                    foreign_cache_guid(), "-3");
   mock_server_->AddUpdateSpecifics(4, 0, "H", 20, 20, false, 0, pref);
   {
     syncable::WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
@@ skipping 10 matching lines @@
   {
     // BOOKMARKS throttled.
     syncable::ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, false, true, false, 0, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(2, false, true, false, 1, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(3, false, true, false, 1, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(4, false, false, false, 0, 21, 21, ids_, &rtrans);
   }
 
   // Unthrottled BOOKMARKS, then BOOKMARKS should get synced now.
-  mock_server_->set_partial_throttling(false);
+  mock_server_->set_throttling(false);
 
   mock_server_->AddUpdateSpecifics(1, 0, "E", 30, 30, true, 0, bookmark,
                                    foreign_cache_guid(), "-1");
   mock_server_->AddUpdateSpecifics(2, 1, "F", 30, 30, false, 2, bookmark,
                                    foreign_cache_guid(), "-2");
   mock_server_->AddUpdateSpecifics(3, 1, "G", 30, 30, false, 1, bookmark,
                                    foreign_cache_guid(), "-3");
   mock_server_->AddUpdateSpecifics(4, 0, "H", 30, 30, false, 0, pref);
   EXPECT_TRUE(SyncShareNudge());
   {
     // BOOKMARKS unthrottled.
     syncable::ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, false, false, false, 0, 31, 31, ids_, &rtrans);
     VERIFY_ENTRY(2, false, false, false, 1, 31, 31, ids_, &rtrans);
     VERIFY_ENTRY(3, false, false, false, 1, 31, 31, ids_, &rtrans);
     VERIFY_ENTRY(4, false, false, false, 0, 30, 30, ids_, &rtrans);
   }
 }
 
+TEST_F(SyncerTest, GetUpdatesPartialFailure) {
+  sync_pb::EntitySpecifics bookmark, pref;
+  bookmark.mutable_bookmark()->set_title("title");
+  pref.mutable_preference()->set_name("name");
+  AddDefaultFieldValue(BOOKMARKS, &bookmark);
+  AddDefaultFieldValue(PREFERENCES, &pref);
+
+  // Normal sync, all the data types should get synced.
+  mock_server_->AddUpdateSpecifics(1, 0, "A", 10, 10, true, 0, bookmark,
+                                   foreign_cache_guid(), "-1");
+  mock_server_->AddUpdateSpecifics(2, 1, "B", 10, 10, false, 2, bookmark,
+                                   foreign_cache_guid(), "-2");
+  mock_server_->AddUpdateSpecifics(3, 1, "C", 10, 10, false, 1, bookmark,
+                                   foreign_cache_guid(), "-3");
+  mock_server_->AddUpdateSpecifics(4, 0, "D", 10, 10, false, 0, pref);
+
+  EXPECT_TRUE(SyncShareNudge());
+  {
+    // Initial state. Everything is normal.
+    syncable::ReadTransaction rtrans(FROM_HERE, directory());
+    VERIFY_ENTRY(1, false, false, false, 0, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(2, false, false, false, 1, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(3, false, false, false, 1, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(4, false, false, false, 0, 10, 10, ids_, &rtrans);
+  }
+
+  // Set BOOKMARKS failure but PREFERENCES not,
+  // then BOOKMARKS should not get synced but PREFERENCES should.
+  ModelTypeSet failed_types(BOOKMARKS);
+  mock_server_->set_partial_failure(true);
+  mock_server_->SetPartialFailureTypes(failed_types);
+
+  mock_server_->AddUpdateSpecifics(1, 0, "E", 20, 20, true, 0, bookmark,
+                                   foreign_cache_guid(), "-1");
+  mock_server_->AddUpdateSpecifics(2, 1, "F", 20, 20, false, 2, bookmark,
+                                   foreign_cache_guid(), "-2");
+  mock_server_->AddUpdateSpecifics(3, 1, "G", 20, 20, false, 1, bookmark,
+                                   foreign_cache_guid(), "-3");
+  mock_server_->AddUpdateSpecifics(4, 0, "H", 20, 20, false, 0, pref);
+  {
+    syncable::WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
+    MutableEntry A(&wtrans, GET_BY_ID, ids_.FromNumber(1));
+    MutableEntry B(&wtrans, GET_BY_ID, ids_.FromNumber(2));
+    MutableEntry C(&wtrans, GET_BY_ID, ids_.FromNumber(3));
+    MutableEntry D(&wtrans, GET_BY_ID, ids_.FromNumber(4));
+    A.PutIsUnsynced(true);
+    B.PutIsUnsynced(true);
+    C.PutIsUnsynced(true);
+    D.PutIsUnsynced(true);
+  }
+  EXPECT_TRUE(SyncShareNudge());

[Nicolas Zea, 2016/11/12 01:04:35]

Shouldn't bookmarks be blocked when this happens?

[Gang Wu, 2016/11/14 19:14:51]

Partial failure does not block whole sync cycle, and is counted as success.
So SyncShareNudge() returning true means the whole sync cycle success,  line
1030-1032 will verify there are entries unsynced.

BTW, this test is copied from previous test GetUpdatesPartialThrottled.
 

+  {
+    // BOOKMARKS failed.
+    syncable::ReadTransaction rtrans(FROM_HERE, directory());
+    VERIFY_ENTRY(1, false, true, false, 0, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(2, false, true, false, 1, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(3, false, true, false, 1, 10, 10, ids_, &rtrans);
+    VERIFY_ENTRY(4, false, false, false, 0, 21, 21, ids_, &rtrans);
+  }
+
+  // Set BOOKMARKS not partial failed, then BOOKMARKS should get synced now.
+  mock_server_->set_partial_failure(false);
+
+  mock_server_->AddUpdateSpecifics(1, 0, "E", 30, 30, true, 0, bookmark,
+                                   foreign_cache_guid(), "-1");
+  mock_server_->AddUpdateSpecifics(2, 1, "F", 30, 30, false, 2, bookmark,
+                                   foreign_cache_guid(), "-2");
+  mock_server_->AddUpdateSpecifics(3, 1, "G", 30, 30, false, 1, bookmark,
+                                   foreign_cache_guid(), "-3");
+  mock_server_->AddUpdateSpecifics(4, 0, "H", 30, 30, false, 0, pref);
+  EXPECT_TRUE(SyncShareNudge());
+  {
+    // BOOKMARKS not failed.
+    syncable::ReadTransaction rtrans(FROM_HERE, directory());
+    VERIFY_ENTRY(1, false, false, false, 0, 31, 31, ids_, &rtrans);
+    VERIFY_ENTRY(2, false, false, false, 1, 31, 31, ids_, &rtrans);
+    VERIFY_ENTRY(3, false, false, false, 1, 31, 31, ids_, &rtrans);
+    VERIFY_ENTRY(4, false, false, false, 0, 30, 30, ids_, &rtrans);
+  }
+}
+
 // This test uses internal knowledge of the directory to test correctness of
 // GetCommitIds.  In almost every other test, the hierarchy is created from
 // parent to child order, and so parents always have metahandles that are
 // smaller than those of their children.  This makes it very difficult to test
 // some GetCommitIds edge cases, since it uses metahandle ordering as
 // a starting point.
 TEST_F(SyncerTest, GetCommitIds_VerifyDeletionCommitOrder) {
   {
     syncable::WriteTransaction trans(FROM_HERE, UNITTEST, directory());
 
@@ skipping 4858 matching lines @@
     EXPECT_EQ("xyz", final_monitor_records["xyz"].extension_id);
     EXPECT_EQ(2049U, final_monitor_records["ABC"].bookmark_write_count);
     EXPECT_EQ(4U, final_monitor_records["xyz"].bookmark_write_count);
   } else {
     EXPECT_TRUE(final_monitor_records.empty())
         << "Should not restore records after successful bookmark commit.";
   }
 }
 
 }  // namespace syncer