AU: When server dies, don't retry forever

AU: When server dies, don't retry forever

BUG=4871
TEST=attached unittests

[adlr, 2010-07-16 21:03:04]

please review ASAP is this is blocking today's release.

Thanks!

[petkov, 2010-07-16 21:20:01]

I can't claim that I understand the whole context but the changes in
libcurl_http_fetcher.cc look functionally correct.

So, LGTM -- a few mostly style nits below.

http://codereview.chromium.org/3010009/diff/1/2
File http_fetcher_unittest.cc (right):

http://codereview.chromium.org/3010009/diff/1/2#newcode146
http_fetcher_unittest.cc:146: HttpFetcherTestTypes;
This should be indented, no?

http://codereview.chromium.org/3010009/diff/1/2#newcode394
http_fetcher_unittest.cc:394: EXPECT_EQ(false, successful);
EXPECT_FALSE

http://codereview.chromium.org/3010009/diff/1/2#newcode406
http_fetcher_unittest.cc:406: GMainLoop *loop =
g_main_loop_new(g_main_context_default(), FALSE);
star to the left

http://codereview.chromium.org/3010009/diff/1/2#newcode429
http_fetcher_unittest.cc:429: GMainLoop *loop =
g_main_loop_new(g_main_context_default(), FALSE);
star to the left

http://codereview.chromium.org/3010009/diff/1/3
File libcurl_http_fetcher.cc (right):

http://codereview.chromium.org/3010009/diff/1/3#newcode78
libcurl_http_fetcher.cc:78: } while (CurlPerformOnce());
It seems that CurlPerformOnce always returns false? So no need for a loop any
more? Or you've left it just in case?

http://codereview.chromium.org/3010009/diff/1/3#newcode98
libcurl_http_fetcher.cc:98: {
Why is this in a subscope?

http://codereview.chromium.org/3010009/diff/1/3#newcode99
libcurl_http_fetcher.cc:99: CHECK_EQ(curl_easy_getinfo(curl_handle_,
CURLINFO_RESPONSE_CODE, &http_response_code), CURLE_OK);
80 chars

http://codereview.chromium.org/3010009/diff/1/3#newcode100
libcurl_http_fetcher.cc:100: LOG(INFO)<< "HTTP response code: " <<
http_response_code;
space after LOG(INFO)

[adlr, 2010-07-16 21:39:50]

PTAL

Thanks!

http://codereview.chromium.org/3010009/diff/1/2
File http_fetcher_unittest.cc (right):

http://codereview.chromium.org/3010009/diff/1/2#newcode146
http_fetcher_unittest.cc:146: HttpFetcherTestTypes;
On 2010/07/16 21:20:01, petkov wrote:
> This should be indented, no?
> 

Hm, I use google's emacs style settings to auto-indent, and sometimes it makes
mistakes. fixed.

http://codereview.chromium.org/3010009/diff/1/2#newcode394
http_fetcher_unittest.cc:394: EXPECT_EQ(false, successful);
On 2010/07/16 21:20:01, petkov wrote:
> EXPECT_FALSE
> 

Done.

http://codereview.chromium.org/3010009/diff/1/2#newcode406
http_fetcher_unittest.cc:406: GMainLoop *loop =
g_main_loop_new(g_main_context_default(), FALSE);
On 2010/07/16 21:20:01, petkov wrote:
> star to the left
> 

Done.

http://codereview.chromium.org/3010009/diff/1/2#newcode429
http_fetcher_unittest.cc:429: GMainLoop *loop =
g_main_loop_new(g_main_context_default(), FALSE);
On 2010/07/16 21:20:01, petkov wrote:
> star to the left
> 

Done.

http://codereview.chromium.org/3010009/diff/1/3
File libcurl_http_fetcher.cc (right):

http://codereview.chromium.org/3010009/diff/1/3#newcode78
libcurl_http_fetcher.cc:78: } while (CurlPerformOnce());
On 2010/07/16 21:20:01, petkov wrote:
> It seems that CurlPerformOnce always returns false? So no need for a loop any
> more? Or you've left it just in case?
> 

Done.

http://codereview.chromium.org/3010009/diff/1/3#newcode98
libcurl_http_fetcher.cc:98: {
On 2010/07/16 21:20:01, petkov wrote:
> Why is this in a subscope?
> 

ah, earlier in my coding it made sense. fixed.

http://codereview.chromium.org/3010009/diff/1/3#newcode99
libcurl_http_fetcher.cc:99: CHECK_EQ(curl_easy_getinfo(curl_handle_,
CURLINFO_RESPONSE_CODE, &http_response_code), CURLE_OK);
On 2010/07/16 21:20:01, petkov wrote:
> 80 chars
> 

Done.

http://codereview.chromium.org/3010009/diff/1/3#newcode100
libcurl_http_fetcher.cc:100: LOG(INFO)<< "HTTP response code: " <<
http_response_code;
On 2010/07/16 21:20:01, petkov wrote:
> space after LOG(INFO)

Done.

[petkov, 2010-07-16 21:47:45]

Consider the comments below. Still LGTM.

http://codereview.chromium.org/3010009/diff/6001/7002
File libcurl_http_fetcher.cc (right):

http://codereview.chromium.org/3010009/diff/6001/7002#newcode82
libcurl_http_fetcher.cc:82: bool LibcurlHttpFetcher::CurlPerformOnce() {
You could make this method void.

http://codereview.chromium.org/3010009/diff/6001/7002#newcode94
libcurl_http_fetcher.cc:94: if (curl_easy_getinfo(curl_handle_,
This is definitely better than the CHECK_EQ...

http://codereview.chromium.org/3010009/diff/6001/7002#newcode263
libcurl_http_fetcher.cc:263: // Since we will return false from this function,
which tells glib to
Actually, it seems we're returning TRUE from this function? Is this a bug? I
guess this is not part of this CL...