AU: Implement exponential back off for 500 and 503 HTTP response codes.

AU: Implement exponential back off for 500 and 503 HTTP response codes.

Also refactors the automatic update checks into a separate
scheduler class and adds unit tests for them.

update_check_scheduler.cc                  59 /   59: 100.0%
update_check_scheduler.h                    1 /    1: 100.0%

Note: because the unit tests for this CL use the
UpdateAttempter class, the CL brings in several untested
modules into the test report reducing the overall test
coverage to ~82%.

BUG=2394
TEST=unit tests, gmerged on device and inspected logs

Change-Id: I078b1727b5338f6fc34e51f5e04a375518d63cef

[adlr, 2010-08-30 19:14:45]

Re coverage: i guess it's okay that it goes down a lot, since it's more honest.

http://codereview.chromium.org/3215006/diff/8001/9008
File update_check_scheduler.cc (right):

http://codereview.chromium.org/3215006/diff/8001/9008#newcode14
update_check_scheduler.cc:14: const int UpdateCheckScheduler::kTimeoutMaxBackoff
= 4 * 60 * 60;  // 4 hours
where did you get this number from? i would have assumed we wanted max backoff
into the days range...

http://codereview.chromium.org/3215006/diff/8001/9008#newcode66
update_check_scheduler.cc:66: interval = utils::FuzzInt(interval, fuzz);
maybe a quick check that interval > 0?

http://codereview.chromium.org/3215006/diff/8001/9008#newcode78
update_check_scheduler.cc:78: CHECK(me->update_attempter_->status() !=
UPDATE_STATUS_IDLE ||
log a warning rather than crash?

http://codereview.chromium.org/3215006/diff/8001/9008#newcode89
update_check_scheduler.cc:89: if (update_attempter_->http_response_code() == 500
||
should we just say everything in the 5xx range is an error deserving of backoff?

http://codereview.chromium.org/3215006/diff/8001/9010
File update_check_scheduler_unittest.cc (right):

http://codereview.chromium.org/3215006/diff/8001/9010#newcode28
update_check_scheduler_unittest.cc:28: // GLib and utils in tests. There're
explicit unit test for the wrapper methods.
So, in general I've been avoiding this pattern, since it means if you change a
method name in the real class, but forget to in the Mock, that you end up
testing the real code, as opposed to getting a compiler error.

What do you think?

http://codereview.chromium.org/3215006/diff/8001/9010#newcode121
update_check_scheduler_unittest.cc:121: // Invokes the actual utils wrapper
method rather than the subclass mock.
if this comment is true, how does this work?

[petkov, 2010-08-30 19:45:11]

PTAL

http://codereview.chromium.org/3215006/diff/8001/9008
File update_check_scheduler.cc (right):

http://codereview.chromium.org/3215006/diff/8001/9008#newcode14
update_check_scheduler.cc:14: const int UpdateCheckScheduler::kTimeoutMaxBackoff
= 4 * 60 * 60;  // 4 hours
On 2010/08/30 19:14:45, adlr wrote:
> where did you get this number from? i would have assumed we wanted max backoff
> into the days range...

From the rate limit site.

http://codereview.chromium.org/3215006/diff/8001/9008#newcode66
update_check_scheduler.cc:66: interval = utils::FuzzInt(interval, fuzz);
On 2010/08/30 19:14:45, adlr wrote:
> maybe a quick check that interval > 0?

Done. And added a unit test.

http://codereview.chromium.org/3215006/diff/8001/9008#newcode78
update_check_scheduler.cc:78: CHECK(me->update_attempter_->status() !=
UPDATE_STATUS_IDLE ||
On 2010/08/30 19:14:45, adlr wrote:
> log a warning rather than crash?

Actually, I think it's better if it's a crash. If we leave it as a warning, then
no more automatic update checks will be scheduled. A crash would cause the
update_engine daemon to restarted (while it's in an idle state).

http://codereview.chromium.org/3215006/diff/8001/9008#newcode89
update_check_scheduler.cc:89: if (update_attempter_->http_response_code() == 500
||
On 2010/08/30 19:14:45, adlr wrote:
> should we just say everything in the 5xx range is an error deserving of
backoff?

I've gone with what the rate limit site says. We can probably extend to 502 but
I'm not sure we get the code from the update server or the proxy.

http://codereview.chromium.org/3215006/diff/8001/9010
File update_check_scheduler_unittest.cc (right):

http://codereview.chromium.org/3215006/diff/8001/9010#newcode28
update_check_scheduler_unittest.cc:28: // GLib and utils in tests. There're
explicit unit test for the wrapper methods.
On 2010/08/30 19:14:45, adlr wrote:
> So, in general I've been avoiding this pattern, since it means if you change a
> method name in the real class, but forget to in the Mock, that you end up
> testing the real code, as opposed to getting a compiler error.
> 
> What do you think?

If you end up unit testing the real code, you'll see test failures, so you'll
catch the issue. While this way you can do better unit testing on the rest of
the code.

In general, I think we should have a global C++ mockable abstraction for GLib
and D-Bus and maybe even for utils. This will really help with unit testing.

http://codereview.chromium.org/3215006/diff/8001/9010#newcode121
update_check_scheduler_unittest.cc:121: // Invokes the actual utils wrapper
method rather than the subclass mock.
On 2010/08/30 19:14:45, adlr wrote:
> if this comment is true, how does this work?

The compiler invokes UpdateCheckScheduler::IsOfficialBuild (rather than the
method from the subclass) directly, not through the virtual pointer. It still
passes an instance of UpdateCheckSchedulerUnderTest as "this". So, if there's
another call to IsOfficialBuild down the call chain, it will go to the
subclassed method.

Long story short -- it works and is being tested :-)

[petkov, 2010-08-30 20:02:27]

http://codereview.chromium.org/3215006/diff/8001/9008
File update_check_scheduler.cc (right):

http://codereview.chromium.org/3215006/diff/8001/9008#newcode78
update_check_scheduler.cc:78: CHECK(me->update_attempter_->status() !=
UPDATE_STATUS_IDLE ||
On 2010/08/30 19:45:11, petkov wrote:
> On 2010/08/30 19:14:45, adlr wrote:
> > log a warning rather than crash?
> 
> Actually, I think it's better if it's a crash. If we leave it as a warning,
then
> no more automatic update checks will be scheduled. A crash would cause the
> update_engine daemon to restarted (while it's in an idle state).
> 

I should elaborate here a bit. This check should never fail. If it fails, it
means there's a serious bug somewhere, and we'll stop scheduling further
automatic update checks in this update_engine session. It seems better to crash
in such case and restart update_engine into, supposedly, a known good state.

[adlr, 2010-08-30 20:17:20]

LGTM

Thanks for the explanations.

-andrew

On Mon, Aug 30, 2010 at 1:02 PM, <petkov@chromium.org> wrote:

>
> http://codereview.chromium.org/3215006/diff/8001/9008
> File update_check_scheduler.cc (right):
>
> http://codereview.chromium.org/3215006/diff/8001/9008#newcode78
> update_check_scheduler.cc:78: CHECK(me->update_attempter_->status() !=
> UPDATE_STATUS_IDLE ||
> On 2010/08/30 19:45:11, petkov wrote:
>
>> On 2010/08/30 19:14:45, adlr wrote:
>> > log a warning rather than crash?
>>
>
>  Actually, I think it's better if it's a crash. If we leave it as a
>>
> warning, then
>
>> no more automatic update checks will be scheduled. A crash would cause
>>
> the
>
>> update_engine daemon to restarted (while it's in an idle state).
>>
>
>
> I should elaborate here a bit. This check should never fail. If it
> fails, it means there's a serious bug somewhere, and we'll stop
> scheduling further automatic update checks in this update_engine
> session. It seems better to crash in such case and restart update_engine
> into, supposedly, a known good state.
>
> http://codereview.chromium.org/3215006/show
>

[petkov, 2010-08-30 20:29:37]

On 2010/08/30 20:17:20, adlr wrote:
> LGTM
> 
> Thanks for the explanations.

Pushing... I've added the CHECK motivation as a comment in the code.

> 
> -andrew
> 
> On Mon, Aug 30, 2010 at 1:02 PM, <mailto:petkov@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.org/3215006/diff/8001/9008
> > File update_check_scheduler.cc (right):
> >
> > http://codereview.chromium.org/3215006/diff/8001/9008#newcode78
> > update_check_scheduler.cc:78: CHECK(me->update_attempter_->status() !=
> > UPDATE_STATUS_IDLE ||
> > On 2010/08/30 19:45:11, petkov wrote:
> >
> >> On 2010/08/30 19:14:45, adlr wrote:
> >> > log a warning rather than crash?
> >>
> >
> >  Actually, I think it's better if it's a crash. If we leave it as a
> >>
> > warning, then
> >
> >> no more automatic update checks will be scheduled. A crash would cause
> >>
> > the
> >
> >> update_engine daemon to restarted (while it's in an idle state).
> >>
> >
> >
> > I should elaborate here a bit. This check should never fail. If it
> > fails, it means there's a serious bug somewhere, and we'll stop
> > scheduling further automatic update checks in this update_engine
> > session. It seems better to crash in such case and restart update_engine
> > into, supposedly, a known good state.
> >
> > http://codereview.chromium.org/3215006/show
> >
>