Fetcher tries all proxies when a secondary chunk download error occurs.
This is a fix to issue 18143:
* New test cases for asserting the desired behavior: if a transfer of
a secondary chunk within a multi-chunk fetch fails, then the fetcher
needs to retry with other available proxies; it will only fail when no
additional proxies are available. The tests ensure both success (one
of the proxies eventually succeeds) and failure (all proxies fail)
cases.
* Small fix to LibcurlHttpFetcher to retry with other proxies upon
failure (error value) of a secondary chunk.
Other changes applied in the course of this fix:
* Massive refactoring of http_fetcher_unittest: substituted template
specialization in typed test setup with proper subclassing, resulting
in a safer and more maintainable infrastructure; extended URLs to
include all (most) parameters pertaining to test workload, such as
download size, flakiness, etc.
* Respective changes to test_http_server: it is now much more
independent of particular kind of tests, and more easily
parametrizable. Also, generalized several internal methods for better
readability and extensibility, such as writing of arbitrary payloads,
parsing headers,
* Migrated common definitions into http_common.{h,cc} (universal
HTTP-related stuff) and http_fetcher_unittest.h (shared definitions
pertaining to unit tests).
* Extended direct proxy resolver to generate a list of (non-) proxies,
so we can unit test proxy failure. Also, better logging to improve
testability.
* Some renaming of classes for better consistency.
BUG=chromium-os:18143
TEST=unit tests
Change-Id: Ib90b53394d7e47184d9953df8fc80348921e8af0
Reviewed-on: https://gerrit.chromium.org/gerrit/12092
Commit-Ready: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index 499087f..05d03fa 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -246,24 +246,33 @@
return;
}
- if (!sent_byte_ && ! IsHttpResponseSuccess()) {
+ if ((!sent_byte_ && !IsHttpResponseSuccess()) || IsHttpResponseError()) {
// The transfer completed w/ error and we didn't get any bytes.
// If we have another proxy to try, try that.
+ //
+ // TODO(garnold) in fact there are two separate cases here: one case is an
+ // other-than-success return code (including no return code) and no
+ // received bytes, which is necessary due to the way callbacks are
+ // currently processing error conditions; the second is an explicit HTTP
+ // error code, where some data may have been received (as in the case of a
+ // semi-successful multi-chunk fetch). This is a confusing behavior and
+ // should be unified into a complete, coherent interface.
+ LOG(INFO) << "Transfer resulted in an error (" << http_response_code_
+ << "), " << bytes_downloaded_ << " bytes downloaded";
PopProxy(); // Delete the proxy we just gave up on.
if (HasProxy()) {
// We have another proxy. Retry immediately.
+ LOG(INFO) << "Trying next proxy: " << GetCurrentProxy();
g_idle_add(&LibcurlHttpFetcher::StaticRetryTimeoutCallback, this);
} else {
// Out of proxies. Give up.
+ LOG(INFO) << "No further proxies, indicating transfer complete";
if (delegate_)
- delegate_->TransferComplete(this, false); // success
+ delegate_->TransferComplete(this, false); // signal fail
}
- return;
- }
-
- if ((transfer_size_ >= 0) && (bytes_downloaded_ < transfer_size_)) {
+ } else if ((transfer_size_ >= 0) && (bytes_downloaded_ < transfer_size_)) {
// Need to restart transfer
retry_count_++;
LOG(INFO) << "Restarting transfer b/c we finished, had downloaded "
@@ -271,16 +280,16 @@
<< transfer_size_ << ". retry_count: " << retry_count_;
if (retry_count_ > kMaxRetriesCount) {
if (delegate_)
- delegate_->TransferComplete(this, false); // success
+ delegate_->TransferComplete(this, false); // signal fail
} else {
g_timeout_add_seconds(retry_seconds_,
&LibcurlHttpFetcher::StaticRetryTimeoutCallback,
this);
}
- return;
} else {
+ LOG(INFO) << "Transfer completed (" << http_response_code_
+ << "), " << bytes_downloaded_ << " bytes downloaded";
if (delegate_) {
- // success is when http_response_code is 2xx
bool success = IsHttpResponseSuccess();
delegate_->TransferComplete(this, success);
}
@@ -297,7 +306,7 @@
const size_t payload_size = size * nmemb;
// Do nothing if no payload or HTTP response is an error.
- if (payload_size == 0 || ! IsHttpResponseSuccess()) {
+ if (payload_size == 0 || !IsHttpResponseSuccess()) {
LOG(INFO) << "HTTP response unsuccessful (" << http_response_code_
<< ") or no payload (" << payload_size << "), nothing to do";
return 0;