Don't retry failed attempts to poll sync server

chrome/browser/sync/engine/sync_scheduler.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 "chrome/browser/sync/engine/sync_scheduler.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/bind.h"
@@ skipping 883 matching lines @@
     SDVLOG(2) << "Job succeeded so not scheduling more jobs";
     return;
   }
 
   // TODO(rlarocque): There's no reason why we should blindly backoff and retry
   // if we don't succeed.  Some types of errors are not likely to disappear on
   // their own.  With the return values now available in the old_job.session, we
   // should be able to detect such errors and only retry when we detect
   // transient errors.
 
-  // We are in backoff mode and our time did not run out. That means we had
-  // a local change, notification from server or a network connection change
-  // notification. In any case set had_nudge = true so we dont retry next
-  // nudge. Note: we will keep retrying network connection changes though as
-  // they are treated as canary jobs. Also we check the mode here because
-  // we want to do this only in normal mode. For config mode jobs we dont
-  // have anything similar to had_nudge.
-  if (IsBackingOff() && wait_interval_->timer.IsRunning() &&
+  if (old_job.purpose == SyncSessionJob::POLL) {
+    // We don't retry POLL jobs.

[tim (not reviewing), 2012/02/14 01:05:32]

Hm, maybe our chat on IM confused me a bit. DecideOnJob is responsible for all
the logic to determine what jobs have what permissions. Why do we need to
special-case here?

[rlarocque, 2012/02/14 01:59:36]

If we didn't have this if statement, we would save the failed poll in the
wait_interval_ and start a backoff timer.  The job would be dropped as soon as
that timer fires, because DecideWhileInWaitInterval() drops jobs with purpose ==
POLL.  

The original intent of this code was that the poll job would be converted to a
NUDGE after it failed for the first time (so we actually would retry failed
polls), but that logic was unreachable (issue 111229).  

I added the if branch because it was the most direct way to avoid scheduling
backoff for poll failures.  

We might be able to rewrite it so it leverages the existing logic, but I'm not
sure if it's safe to use SyncScheduler::DecideOnJob() to decide what to do with
jobs that are meant to be run in the future.  I thought that function assumes it
has access to all the current state of 'this' when making decisions.

[tim (not reviewing), 2012/02/15 02:21:29]

It does rely on whether or not we're in a wait interval, which we haven't
figured out by this point since we might hit the HandleContinuationError case
below.  So yeah, good point. I'd like to find a way to avoid special casing job
purpose in any new locations, though.  So, there are only two cases below... in
one, a wait interval exists. In the other, it doesn't, but it actually seems
like HandleContinuationError should itself be the thing that checks
DecideOnJobWhileInWaitInterval before scheduling a POLL job for retry, no?  It
feels like a bug that it doesn't, to me... unless I'm missing something.

If it did that, we could probably call DecideOnJob above and handle all the
cases that way.

If that turns out to be too much of a headache for some reason, let me know...
and if so, instead of the empty if, we should probably have an early return. (So
leave the code as is from before, but add an if (purpose = POLL) return; right
after your todo).

[rlarocque, 2012/02/17 20:51:02]

Lots of minor points:
All the branches, including HandleConinuationError, end up putting is into a
wait interval.  

It does rely on the state of the wait interval, though, so the point still
stands.
Actually, that's not what the two branches are for.  The logic on line 906
mirrors that in DecideWhileInWaitInterval.  The condition for that branch could
be described as "Was this job executed because DecideWhileInWaitInterval
returned CONTINUE on line 352?"

All other failures go through HandleContinuationError.

I think the bug you're looking for was issue 112229.  I suspect the original
author never intended for HandleContinuatoinError (formerly known as
HandleConsecutiveContinuationError()) to be processing POLL jobs.  It was
intended that a POLL would be converted to a NUDGE, then run through this
function on subsequent failures.  However, in practice it did process POLL jobs
(wrongly).  

Calling DecideOnJobWhileInWaitInterval() from HandleContinuationError() probably
won't work, because the wait interval may not exist yet, and the Decide..()
function tries to access its members.  

We could move the if (purpose == POLL) check to HandleContinuationError(), but
I'd really prefer to put all the rescheduling logic in one place.  
I think that's the best solution.  Should I do something about the comments,
too?  I tried to make the purpose of each branch more clear, but apparently
didn't do a good job of it.

+  } else if (IsBackingOff() && wait_interval_->timer.IsRunning() &&
       mode_ == NORMAL_MODE) {
+    // When in normal mode, we allow up to one nudge per backoff interval.  It
+    // appears that this was our nudge for this interval, and it failed.
+    //
+    // Note: This does not prevent us from running canary jobs.  For example, an
+    // IP address change might still result in another nudge being executed
+    // during this backoff interval.
     SDVLOG(2) << "A nudge during backoff failed";
-    // We weren't continuing but we're in backoff; must have been a nudge.
+
     DCHECK_EQ(SyncSessionJob::NUDGE, old_job.purpose);
     DCHECK(!wait_interval_->had_nudge);
+
     wait_interval_->had_nudge = true;
-    // Old job did not finish. So make it the pending job.
     InitOrCoalescePendingJob(old_job);
-    // Resume waiting.
     RestartWaiting();
   } else {
+    // Either this is the first failure or a consecutive failure after our
+    // backoff timer expired.  We handle it the same way in either case.
     SDVLOG(2) << "Non-'backoff nudge' SyncShare job failed";
-    // We don't seem to have made forward progress. Start or extend backoff.
     HandleContinuationError(old_job);
   }
 }
 
 void SyncScheduler::AdjustPolling(const SyncSessionJob* old_job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
   TimeDelta poll  = (!session_context_->notifications_enabled()) ?
       syncer_short_poll_interval_seconds_ :
       syncer_long_poll_interval_seconds_;
@@ skipping 265 matching lines @@
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // browser_sync