Updater avoids download in case of an error HTTP response.
(a) LibcurlHttpFetcher avoids download if the HTTP reponse indicates an
error; corresponding change to unit test code and test HTTP server. (b)
Added a method for returning the total bytes downloaded to HttpFetcher
and all subclasses, needed for unit testing. (c) Generalized check for
successful HTTP response code in LibcurlHttpFetcher.
BUG=chromium-os:9648
TEST=unit tests
Change-Id: I46d72fbde0ecfb53823b0705ce17f9547515ee61
Reviewed-on: https://gerrit.chromium.org/gerrit/11773
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Andrew de los Reyes <adlr@chromium.org>
Commit-Ready: Gilad Arnold <garnold@chromium.org>
diff --git a/http_fetcher.h b/http_fetcher.h
index f4ee9b7..a14e854 100644
--- a/http_fetcher.h
+++ b/http_fetcher.h
@@ -91,6 +91,9 @@
virtual void set_idle_seconds(int seconds) {}
virtual void set_retry_seconds(int seconds) {}
+ // Get the total number of bytes downloaded by fetcher.
+ virtual size_t GetBytesDownloaded() = 0;
+
ProxyResolver* proxy_resolver() const { return proxy_resolver_; }
// These are used for testing:
diff --git a/http_fetcher_unittest.cc b/http_fetcher_unittest.cc
index 66552ff..2aabfd9 100644
--- a/http_fetcher_unittest.cc
+++ b/http_fetcher_unittest.cc
@@ -42,6 +42,7 @@
HttpFetcher* NewSmallFetcher() = 0;
string BigUrl() const = 0;
string SmallUrl() const = 0;
+ string ErrorUrl() const = 0;
bool IsMock() const = 0;
bool IsMulti() const = 0;
};
@@ -76,6 +77,9 @@
string SmallUrl() const {
return "unused://unused";
}
+ string ErrorUrl() const {
+ return "unused://unused";
+ }
bool IsMock() const { return true; }
bool IsMulti() const { return false; }
typedef NullHttpServer HttpServer;
@@ -166,6 +170,9 @@
string SmallUrl() const {
return LocalServerUrlForPath("/foo");
}
+ string ErrorUrl() const {
+ return LocalServerUrlForPath("/error");
+ }
bool IsMock() const { return false; }
bool IsMulti() const { return false; }
typedef PythonHttpServer HttpServer;
@@ -207,20 +214,45 @@
namespace {
class HttpFetcherTestDelegate : public HttpFetcherDelegate {
public:
+ HttpFetcherTestDelegate(void) :
+ is_expect_error_(false), times_transfer_complete_called_(0),
+ times_transfer_terminated_called_(0), times_received_bytes_called_(0) {}
+
virtual void ReceivedBytes(HttpFetcher* fetcher,
const char* bytes, int length) {
char str[length + 1];
memset(str, 0, length + 1);
memcpy(str, bytes, length);
+
+ // Update counters
+ times_received_bytes_called_++;
}
+
virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
- EXPECT_EQ(200, fetcher->http_response_code());
+ if (is_expect_error_)
+ EXPECT_EQ(404, fetcher->http_response_code());
+ else
+ EXPECT_EQ(200, fetcher->http_response_code());
g_main_loop_quit(loop_);
+
+ // Update counter
+ times_transfer_complete_called_++;
}
+
virtual void TransferTerminated(HttpFetcher* fetcher) {
ADD_FAILURE();
+ times_transfer_terminated_called_++;
}
+
GMainLoop* loop_;
+
+ // Are we expecting an error response? (default: no)
+ bool is_expect_error_;
+
+ // Counters for callback invocations.
+ int times_transfer_complete_called_;
+ int times_transfer_terminated_called_;
+ int times_received_bytes_called_;
};
struct StartTransferArgs {
@@ -273,6 +305,43 @@
g_main_loop_unref(loop);
}
+// Issue #9648: when server returns an error HTTP response, the fetcher needs to
+// terminate transfer prematurely, rather than try to process the error payload.
+TYPED_TEST(HttpFetcherTest, ErrorTest) {
+ if (this->IsMock() || this->IsMulti())
+ return;
+ GMainLoop* loop = g_main_loop_new(g_main_context_default(), FALSE);
+ {
+ HttpFetcherTestDelegate delegate;
+ delegate.loop_ = loop;
+
+ // Delegate should expect an error response.
+ delegate.is_expect_error_ = true;
+
+ scoped_ptr<HttpFetcher> fetcher(this->NewSmallFetcher());
+ fetcher->set_delegate(&delegate);
+
+ typename TestFixture::HttpServer server;
+ ASSERT_TRUE(server.started_);
+
+ StartTransferArgs start_xfer_args = {fetcher.get(), this->ErrorUrl()};
+
+ g_timeout_add(0, StartTransfer, &start_xfer_args);
+ g_main_loop_run(loop);
+
+ // Make sure that no bytes were received.
+ CHECK_EQ(delegate.times_received_bytes_called_, 0);
+ CHECK_EQ(fetcher->GetBytesDownloaded(), 0);
+
+ // Make sure that transfer completion was signaled once, and no termination
+ // was signaled.
+ CHECK_EQ(delegate.times_transfer_complete_called_, 1);
+ CHECK_EQ(delegate.times_transfer_terminated_called_, 0);
+ }
+ g_main_loop_unref(loop);
+}
+
+
namespace {
class PausingHttpFetcherTestDelegate : public HttpFetcherDelegate {
public:
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index 7f5f859..499087f 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -246,8 +246,7 @@
return;
}
- if (!sent_byte_ &&
- (http_response_code_ < 200 || http_response_code_ >= 300)) {
+ if (!sent_byte_ && ! IsHttpResponseSuccess()) {
// The transfer completed w/ error and we didn't get any bytes.
// If we have another proxy to try, try that.
@@ -282,8 +281,7 @@
} else {
if (delegate_) {
// success is when http_response_code is 2xx
- bool success = (http_response_code_ >= 200) &&
- (http_response_code_ < 300);
+ bool success = IsHttpResponseSuccess();
delegate_->TransferComplete(this, success);
}
}
@@ -294,10 +292,18 @@
}
size_t LibcurlHttpFetcher::LibcurlWrite(void *ptr, size_t size, size_t nmemb) {
- if (size == 0)
- return 0;
- sent_byte_ = true;
+ // Update HTTP response first.
GetHttpResponseCode();
+ const size_t payload_size = size * nmemb;
+
+ // Do nothing if no payload or HTTP response is an error.
+ if (payload_size == 0 || ! IsHttpResponseSuccess()) {
+ LOG(INFO) << "HTTP response unsuccessful (" << http_response_code_
+ << ") or no payload (" << payload_size << "), nothing to do";
+ return 0;
+ }
+
+ sent_byte_ = true;
{
double transfer_size_double;
CHECK_EQ(curl_easy_getinfo(curl_handle_,
@@ -308,12 +314,12 @@
transfer_size_ = resume_offset_ + new_transfer_size;
}
}
- bytes_downloaded_ += size * nmemb;
+ bytes_downloaded_ += payload_size;
in_write_callback_ = true;
if (delegate_)
- delegate_->ReceivedBytes(this, reinterpret_cast<char*>(ptr), size * nmemb);
+ delegate_->ReceivedBytes(this, reinterpret_cast<char*>(ptr), payload_size);
in_write_callback_ = false;
- return size * nmemb;
+ return payload_size;
}
void LibcurlHttpFetcher::Pause() {
diff --git a/libcurl_http_fetcher.h b/libcurl_http_fetcher.h
index b436de6..d727f19 100644
--- a/libcurl_http_fetcher.h
+++ b/libcurl_http_fetcher.h
@@ -99,6 +99,10 @@
check_certificate_ = check_certificate;
}
+ virtual size_t GetBytesDownloaded() {
+ return static_cast<size_t>(bytes_downloaded_);
+ }
+
private:
// Callback for when proxy resolution has completed. This begins the
// transfer.
@@ -107,6 +111,11 @@
// Asks libcurl for the http response code and stores it in the object.
void GetHttpResponseCode();
+ // Checks whether stored HTTP response is successful.
+ inline bool IsHttpResponseSuccess() {
+ return (http_response_code_ >= 200 && http_response_code_ < 300);
+ }
+
// Resumes a transfer where it left off. This will use the
// HTTP Range: header to make a new connection from where the last
// left off.
diff --git a/mock_http_fetcher.h b/mock_http_fetcher.h
index 6d72b92..0fd9537 100644
--- a/mock_http_fetcher.h
+++ b/mock_http_fetcher.h
@@ -51,6 +51,11 @@
delegate_->SeekToOffset(offset);
}
+ // Dummy: no bytes were downloaded.
+ virtual size_t GetBytesDownloaded() {
+ return sent_size_;
+ }
+
// Begins the transfer if it hasn't already begun.
virtual void BeginTransfer(const std::string& url);
diff --git a/multi_range_http_fetcher.h b/multi_range_http_fetcher.h
index 1127568..306ac2a 100644
--- a/multi_range_http_fetcher.h
+++ b/multi_range_http_fetcher.h
@@ -80,6 +80,10 @@
base_fetcher_->SetProxies(proxies);
}
+ inline virtual size_t GetBytesDownloaded() {
+ return base_fetcher_->GetBytesDownloaded();
+ }
+
private:
// pair<offset, length>:
typedef std::vector<std::pair<off_t, off_t> > RangesVect;
diff --git a/test_http_server.cc b/test_http_server.cc
index 6a919d5..11b66ca 100644
--- a/test_http_server.cc
+++ b/test_http_server.cc
@@ -125,10 +125,52 @@
return buf;
}
+// Return a string description for common HTTP response codes.
+const char *GetHttpStatusLine(int code_id) {
+ struct HttpStatusCode {
+ int id;
+ const char* description;
+ } http_status_codes[] = {
+ { 200, "OK" },
+ { 201, "Created" },
+ { 202, "Accepted" },
+ { 203, "Non-Authoritative Information" },
+ { 204, "No Content" },
+ { 205, "Reset Content" },
+ { 206, "Partial Content" },
+ { 300, "Multiple Choices" },
+ { 301, "Moved Permanently" },
+ { 302, "Found" },
+ { 303, "See Other" },
+ { 304, "Not Modified" },
+ { 305, "Use Proxy" },
+ { 307, "Temporary Redirect" },
+ { 400, "Bad Request" },
+ { 401, "Unauthorized" },
+ { 403, "Forbidden" },
+ { 404, "Not Found" },
+ { 408, "Request Timeout" },
+ { 500, "Internal Server Error" },
+ { 501, "Not Implemented" },
+ { 503, "Service Unavailable" },
+ { 505, "HTTP Version Not Supported" },
+ { 0, "Undefined" },
+ };
+
+ int i;
+ for (i = 0; http_status_codes[i].id; ++i)
+ if (http_status_codes[i].id == code_id)
+ break;
+
+ return http_status_codes[i].description;
+}
+
+
void WriteHeaders(int fd, bool support_range, off_t full_size,
off_t start_offset, int return_code) {
LOG(INFO) << "writing headers";
- WriteString(fd, string("HTTP/1.1 ") + Itoa(return_code) + " OK\r\n");
+ WriteString(fd, string("HTTP/1.1 ") + Itoa(return_code) + " " +
+ GetHttpStatusLine(return_code) + "\r\n");
WriteString(fd, "Content-Type: application/octet-stream\r\n");
if (support_range) {
WriteString(fd, "Accept-Ranges: bytes\r\n");
@@ -235,6 +277,16 @@
WriteString(fd, data_to_write);
}
+// Handle an error response from server.
+void HandleError(int fd, const HttpRequest& request) {
+ LOG(INFO) << "Generating error HTTP response";
+
+ // Generate a Not Found" error page with some text.
+ const string data("This is an error page.");
+ WriteHeaders(fd, false, data.size(), 0, 404);
+ WriteString(fd, data);
+}
+
void HandleConnection(int fd) {
HttpRequest request;
ParseRequest(fd, &request);
@@ -249,6 +301,8 @@
HandleFlaky(fd, request);
else if (request.url.find("/redirect/") == 0)
HandleRedirect(fd, request);
+ else if (request.url.find("/error") == 0)
+ HandleError(fd, request);
else
HandleDefault(fd, request);