Fix terminating a transfer while resolving proxies. am: 71f6762c7e
am: a7d55efc9b
Change-Id: Ie2fcccc49d6ad900ff2c5039aab8034272b2b14c
diff --git a/common/http_fetcher.cc b/common/http_fetcher.cc
index e592cc5..4fd1082 100644
--- a/common/http_fetcher.cc
+++ b/common/http_fetcher.cc
@@ -26,10 +26,7 @@
namespace chromeos_update_engine {
HttpFetcher::~HttpFetcher() {
- if (no_resolver_idle_id_ != MessageLoop::kTaskIdNull) {
- MessageLoop::current()->CancelTask(no_resolver_idle_id_);
- no_resolver_idle_id_ = MessageLoop::kTaskIdNull;
- }
+ CancelProxyResolution();
}
void HttpFetcher::SetPostData(const void* data, size_t size,
@@ -59,23 +56,42 @@
base::Unretained(this)));
return true;
}
- return proxy_resolver_->GetProxiesForUrl(
+ proxy_request_ = proxy_resolver_->GetProxiesForUrl(
url, base::Bind(&HttpFetcher::ProxiesResolved, base::Unretained(this)));
+ if (proxy_request_ == kProxyRequestIdNull) {
+ callback_.reset();
+ return false;
+ }
+ return true;
}
void HttpFetcher::NoProxyResolverCallback() {
+ no_resolver_idle_id_ = MessageLoop::kTaskIdNull;
ProxiesResolved(deque<string>());
}
void HttpFetcher::ProxiesResolved(const deque<string>& proxies) {
- no_resolver_idle_id_ = MessageLoop::kTaskIdNull;
+ proxy_request_ = kProxyRequestIdNull;
if (!proxies.empty())
SetProxies(proxies);
- CHECK_NE(static_cast<Closure*>(nullptr), callback_.get());
+ CHECK(callback_.get()) << "ProxiesResolved but none pending.";
Closure* callback = callback_.release();
// This may indirectly call back into ResolveProxiesForUrl():
callback->Run();
delete callback;
}
+bool HttpFetcher::CancelProxyResolution() {
+ bool ret = false;
+ if (no_resolver_idle_id_ != MessageLoop::kTaskIdNull) {
+ ret = MessageLoop::current()->CancelTask(no_resolver_idle_id_);
+ no_resolver_idle_id_ = MessageLoop::kTaskIdNull;
+ }
+ if (proxy_request_ && proxy_resolver_) {
+ ret = proxy_resolver_->CancelProxyRequest(proxy_request_) || ret;
+ proxy_request_ = kProxyRequestIdNull;
+ }
+ return ret;
+}
+
} // namespace chromeos_update_engine
diff --git a/common/http_fetcher.h b/common/http_fetcher.h
index 9f81879..fb15689 100644
--- a/common/http_fetcher.h
+++ b/common/http_fetcher.h
@@ -134,6 +134,11 @@
ProxyResolver* proxy_resolver() const { return proxy_resolver_; }
protected:
+ // Cancels a proxy resolution in progress. The callback passed to
+ // ResolveProxiesForUrl() will not be called. Returns whether there was a
+ // pending proxy resolution to be canceled.
+ bool CancelProxyResolution();
+
// The URL we're actively fetching from
std::string url_;
@@ -170,6 +175,10 @@
// |proxy_resolver_|.
void NoProxyResolverCallback();
+ // Stores the ongoing proxy request id if there is one, otherwise
+ // kProxyRequestIdNull.
+ ProxyRequestId proxy_request_{kProxyRequestIdNull};
+
DISALLOW_COPY_AND_ASSIGN(HttpFetcher);
};
diff --git a/common/http_fetcher_unittest.cc b/common/http_fetcher_unittest.cc
index 11a20e9..e3f3c6d 100644
--- a/common/http_fetcher_unittest.cc
+++ b/common/http_fetcher_unittest.cc
@@ -432,7 +432,6 @@
}
void TransferTerminated(HttpFetcher* fetcher) override {
- ADD_FAILURE();
times_transfer_terminated_called_++;
MessageLoop::current()->BreakLoop();
}
@@ -468,6 +467,7 @@
fetcher.get(),
this->test_.SmallUrl(server->GetPort())));
this->loop_.Run();
+ EXPECT_EQ(0, delegate.times_transfer_terminated_called_);
}
TYPED_TEST(HttpFetcherTest, SimpleBigTest) {
@@ -483,6 +483,7 @@
fetcher.get(),
this->test_.BigUrl(server->GetPort())));
this->loop_.Run();
+ EXPECT_EQ(0, delegate.times_transfer_terminated_called_);
}
// Issue #9648: when server returns an error HTTP response, the fetcher needs to
@@ -508,13 +509,13 @@
this->loop_.Run();
// Make sure that no bytes were received.
- CHECK_EQ(delegate.times_received_bytes_called_, 0);
- CHECK_EQ(fetcher->GetBytesDownloaded(), static_cast<size_t>(0));
+ EXPECT_EQ(0, delegate.times_received_bytes_called_);
+ EXPECT_EQ(0U, fetcher->GetBytesDownloaded());
// 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);
+ EXPECT_EQ(1, delegate.times_transfer_complete_called_);
+ EXPECT_EQ(0, delegate.times_transfer_terminated_called_);
}
TYPED_TEST(HttpFetcherTest, ExtraHeadersInRequestTest) {
@@ -706,6 +707,32 @@
this->loop_.CancelTask(task_id);
}
+TYPED_TEST(HttpFetcherTest, TerminateTransferWhileResolvingProxyTest) {
+ if (this->test_.IsMock() || !this->test_.IsHttpSupported())
+ return;
+ MockProxyResolver mock_resolver;
+ unique_ptr<HttpFetcher> fetcher(this->test_.NewLargeFetcher(&mock_resolver));
+
+ HttpFetcherTestDelegate delegate;
+ fetcher->set_delegate(&delegate);
+
+ EXPECT_CALL(mock_resolver, GetProxiesForUrl(_, _)).WillOnce(Return(123));
+ fetcher->BeginTransfer("http://fake_url");
+ // Run the message loop until idle. This must call the MockProxyResolver with
+ // the request.
+ while (this->loop_.RunOnce(false)) {
+ }
+ testing::Mock::VerifyAndClearExpectations(&mock_resolver);
+
+ EXPECT_CALL(mock_resolver, CancelProxyRequest(123)).WillOnce(Return(true));
+
+ // Terminate the transfer right before the proxy resolution response.
+ fetcher->TerminateTransfer();
+ EXPECT_EQ(0, delegate.times_received_bytes_called_);
+ EXPECT_EQ(0, delegate.times_transfer_complete_called_);
+ EXPECT_EQ(1, delegate.times_transfer_terminated_called_);
+}
+
namespace {
class FlakyHttpFetcherTestDelegate : public HttpFetcherDelegate {
public:
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index 30ee1f3..ee31c8d 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -58,6 +58,7 @@
LibcurlHttpFetcher::~LibcurlHttpFetcher() {
LOG_IF(ERROR, transfer_in_progress_)
<< "Destroying the fetcher while a transfer is in progress.";
+ CancelProxyResolution();
CleanUp();
}
@@ -313,6 +314,7 @@
}
void LibcurlHttpFetcher::ForceTransferTermination() {
+ CancelProxyResolution();
CleanUp();
if (delegate_) {
// Note that after the callback returns this object may be destroyed.