Don't retry failed attempts to poll sync server

Don't retry failed attempts to poll sync server

This commit doesn't change much of our intended behaviour, though it
does affect the details of how we achieve it.

A failed poll used to cause us to schedule a retry job.  However,
because this job was considered to be a POLL job, and we don't run POLL
jobs when we're in backoff, it would get rejected by the scheduler.  The
backoff timer never got reset after that, so this would have happened
only once per poll attempt.

This change makes it so we never backoff in response to a failed poll.
This might have an impact in some really strange edge cases (ie. a poll
recently failed then we receive an IP address change notification) but
those changes should be an improvement and most common use cases will be
unaffected.  Users will likely never notice the effects of this change.

Our tests, however, do care about these sorts of details.  This commit
fixes some existing tests and adds one new one.

BUG=112954
TEST=SyncSchedulerTest in sync_unit_test


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124034

[rlarocque, 2012-02-10 19:19:49]

Tim: please review.

I'm not sure that I've done a very good job of explaining the problem or the
solution.  Let me know if you have any questions.

In addition to fixing the problem as described in the commit message and bug,
I've also taken this opportunity to refactor the comments in
SyncScheduler::ScheduleNextSync().  That's not an essential part of this change;
I can revert if necessary.

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

http://codereview.chromium.org/9372051/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/9372051/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/sync_scheduler.cc:905: // We don't retry POLL jobs.
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]

http://codereview.chromium.org/9372051/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/9372051/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/sync_scheduler.cc:905: // We don't retry POLL jobs.
On 2012/02/14 01:05:32, timsteele wrote:
> 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?

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]

http://codereview.chromium.org/9372051/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/9372051/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/sync_scheduler.cc:905: // We don't retry POLL jobs.
On 2012/02/14 01:59:36, rlarocque wrote:
> On 2012/02/14 01:05:32, timsteele wrote:
> > 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?
> 
> 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.

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:01]

I agree with the thought behind your concerns, but I think practical issues get
in the way of addressing them at this time.  

I'm starting to think I should write a doc describing the SyncScheduler.  There
may be a lot of misconceptions about how it works in practice.  I find I often
refer to the explanations scribbled in my notebook; maybe that information could
be useful to others, too.

http://codereview.chromium.org/9372051/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/9372051/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/sync_scheduler.cc:905: // We don't retry POLL jobs.
On 2012/02/15 02:21:29, timsteele wrote:
> On 2012/02/14 01:59:36, rlarocque wrote:
> > On 2012/02/14 01:05:32, timsteele wrote:
> > > 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?
> > 
> > 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.
> 

Lots of minor points:

> 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.  

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.

> 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.
> 

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.  

> 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).

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.

[rlarocque, 2012-02-22 19:56:01]

Patch set 3 partially addresses Tim's comments and patch set 4 fixes a serious
memory issue that made some trybots fail.  

PTAL.

[tim (not reviewing), 2012-02-28 07:24:12]

On 2012/02/22 19:56:01, rlarocque wrote:
> Patch set 3 partially addresses Tim's comments and patch set 4 fixes a serious
> memory issue that made some trybots fail.  
> 
> PTAL.

This LGTM, I don't want to stand in the way of progress! I'd like to keep
thinking about using DecideOnJob if we can, though.  We can chat more in person
and file a tracking issue if it winds up seeming worthwhile.

[commit-bot: I haz the power, 2012-02-28 17:57:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9372051/19001

[commit-bot: I haz the power, 2012-02-28 21:50:36]

Change committed as 124034