sync: return short initial backoff delay for NETWORK_CONNECTION_UNAVAILABLE

BUG=chromium-os:35073


Review URL: https://chromiumcodereview.appspot.com/11098084

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@161481 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 32 insertions, 0 deletions

diff --git a/sync/engine/backoff_delay_provider.cc b/sync/engine/backoff_delay_provider.cc
index af70442..979a7b4db 100644
--- a/sync/engine/backoff_delay_provider.cc
+++ b/sync/engine/backoff_delay_provider.cc
@@ -61,8 +61,32 @@ TimeDelta BackoffDelayProvider::GetDelay(const base::TimeDelta& last_delay) {
 
 TimeDelta BackoffDelayProvider::GetInitialDelay(
     const sessions::ModelNeutralState& state) const {
+  // NETWORK_CONNECTION_UNAVAILABLE implies we did not even manage to hit the
+  // wire; the failure occurred locally. Note that if commit_result is *not*
+  // UNSET, this implies download_updates_result succeeded.  Also note that
+  // last_get_key_result is coupled to last_download_updates_result in that
+  // they are part of the same GetUpdates request, so we only check if
+  // the download request is CONNECTION_UNAVAILABLE.
+  //
+  // TODO(tim): Should we treat NETWORK_IO_ERROR similarly? It's different
+  // from CONNECTION_UNAVAILABLE in that a request may well have succeeded
+  // in contacting the server (e.g we got a 200 back), but we failed
+  // trying to parse the response (actual content length != HTTP response
+  // header content length value).  For now since we're considering
+  // merging this code to branches and I haven't audited all the
+  // NETWORK_IO_ERROR cases carefully, I'm going to target the fix
+  // very tightly (see bug chromium-os:35073). DIRECTORY_LOOKUP_FAILED is
+  // another example of something that shouldn't backoff, though the
+  // scheduler should probably be handling these cases differently. See
+  // the TODO(rlarocque) in ScheduleNextSync.
+  if (state.commit_result == NETWORK_CONNECTION_UNAVAILABLE ||
+      state.last_download_updates_result == NETWORK_CONNECTION_UNAVAILABLE) {
+    return short_initial_backoff_;
+  }
+
   if (SyncerErrorIsError(state.last_get_key_result))
     return default_initial_backoff_;
+
   // Note: If we received a MIGRATION_DONE on download updates, then commit
   // should not have taken place.  Moreover, if we receive a MIGRATION_DONE
   // on commit, it means that download updates succeeded.  Therefore, we only
diff --git a/sync/engine/backoff_delay_provider_unittest.cc b/sync/engine/backoff_delay_provider_unittest.cc
index 31ed515..4a9be53 100644
--- a/sync/engine/backoff_delay_provider_unittest.cc
+++ b/sync/engine/backoff_delay_provider_unittest.cc
@@ -45,6 +45,10 @@ TEST_F(BackoffDelayProviderTest, GetInitialDelay) {
   EXPECT_EQ(kInitialBackoffShortRetrySeconds,
             delay->GetInitialDelay(state).InSeconds());
 
+  state.last_download_updates_result = NETWORK_CONNECTION_UNAVAILABLE;
+  EXPECT_EQ(kInitialBackoffShortRetrySeconds,
+            delay->GetInitialDelay(state).InSeconds());
+
   state.last_download_updates_result = SERVER_RETURN_TRANSIENT_ERROR;
   EXPECT_EQ(kInitialBackoffRetrySeconds,
             delay->GetInitialDelay(state).InSeconds());
@@ -64,6 +68,10 @@ TEST_F(BackoffDelayProviderTest, GetInitialDelay) {
   state.commit_result = SERVER_RETURN_MIGRATION_DONE;
   EXPECT_EQ(kInitialBackoffShortRetrySeconds,
             delay->GetInitialDelay(state).InSeconds());
+
+  state.commit_result = NETWORK_CONNECTION_UNAVAILABLE;
+  EXPECT_EQ(kInitialBackoffShortRetrySeconds,
+            delay->GetInitialDelay(state).InSeconds());
 }
 
 TEST_F(BackoffDelayProviderTest, GetInitialDelayWithOverride) {