Allow to cancel a proxy resolution request.
am: 3582194c10
Change-Id: I89393fcb150d86cd024e9038500b006be436d490
diff --git a/Android.mk b/Android.mk
index a18f754..eb192f0 100644
--- a/Android.mk
+++ b/Android.mk
@@ -1077,6 +1077,7 @@
payload_generator/tarjan_unittest.cc \
payload_generator/topological_sort_unittest.cc \
payload_generator/zip_unittest.cc \
+ proxy_resolver_unittest.cc \
testrunner.cc
ifeq ($(local_use_omaha),1)
LOCAL_C_INCLUDES += \
diff --git a/chrome_browser_proxy_resolver.cc b/chrome_browser_proxy_resolver.cc
index da57e1d..09365c1 100644
--- a/chrome_browser_proxy_resolver.cc
+++ b/chrome_browser_proxy_resolver.cc
@@ -17,9 +17,7 @@
#include "update_engine/chrome_browser_proxy_resolver.h"
#include <deque>
-#include <map>
#include <string>
-#include <utility>
#include <base/bind.h>
#include <base/strings/string_tokenizer.h>
@@ -33,8 +31,6 @@
using base::TimeDelta;
using brillo::MessageLoop;
using std::deque;
-using std::make_pair;
-using std::pair;
using std::string;
const char kLibCrosServiceName[] = "org.chromium.LibCrosService";
@@ -64,15 +60,13 @@
ChromeBrowserProxyResolver::~ChromeBrowserProxyResolver() {
// Kill outstanding timers.
- for (auto& timer : timers_) {
- MessageLoop::current()->CancelTask(timer.second);
- timer.second = MessageLoop::kTaskIdNull;
+ for (const auto& it : callbacks_) {
+ MessageLoop::current()->CancelTask(it.second->timeout_id);
}
}
-bool ChromeBrowserProxyResolver::GetProxiesForUrl(const string& url,
- ProxiesResolvedFn callback,
- void* data) {
+ProxyRequestId ChromeBrowserProxyResolver::GetProxiesForUrl(
+ const string& url, const ProxiesResolvedFn& callback) {
int timeout = timeout_;
brillo::ErrorPtr error;
if (!libcros_proxy_->service_interface_proxy()->ResolveNetworkProxy(
@@ -84,38 +78,47 @@
timeout = 0;
}
- callbacks_.insert(make_pair(url, make_pair(callback, data)));
- MessageLoop::TaskId timer = MessageLoop::current()->PostDelayedTask(
+ std::unique_ptr<ProxyRequestData> request(new ProxyRequestData());
+ request->callback = callback;
+ ProxyRequestId timeout_id = MessageLoop::current()->PostDelayedTask(
FROM_HERE,
base::Bind(&ChromeBrowserProxyResolver::HandleTimeout,
base::Unretained(this),
- url),
+ url,
+ request.get()),
TimeDelta::FromSeconds(timeout));
- timers_.insert(make_pair(url, timer));
- return true;
+ request->timeout_id = timeout_id;
+ callbacks_.emplace(url, std::move(request));
+
+ // We re-use the timeout_id from the MessageLoop as the request id.
+ return timeout_id;
}
-bool ChromeBrowserProxyResolver::DeleteUrlState(
- const string& source_url,
- bool delete_timer,
- pair<ProxiesResolvedFn, void*>* callback) {
- {
- CallbacksMap::iterator it = callbacks_.lower_bound(source_url);
- TEST_AND_RETURN_FALSE(it != callbacks_.end());
- TEST_AND_RETURN_FALSE(it->first == source_url);
- if (callback)
- *callback = it->second;
- callbacks_.erase(it);
+bool ChromeBrowserProxyResolver::CancelProxyRequest(ProxyRequestId request) {
+ // Finding the timeout_id in the callbacks_ structure requires a linear search
+ // but we expect this operation to not be so frequent and to have just a few
+ // proxy requests, so this should be fast enough.
+ for (auto it = callbacks_.begin(); it != callbacks_.end(); ++it) {
+ if (it->second->timeout_id == request) {
+ MessageLoop::current()->CancelTask(request);
+ callbacks_.erase(it);
+ return true;
+ }
}
- {
- TimeoutsMap::iterator it = timers_.lower_bound(source_url);
- TEST_AND_RETURN_FALSE(it != timers_.end());
- TEST_AND_RETURN_FALSE(it->first == source_url);
- if (delete_timer)
- MessageLoop::current()->CancelTask(it->second);
- timers_.erase(it);
+ return false;
+}
+
+void ChromeBrowserProxyResolver::ProcessUrlResponse(
+ const string& source_url, const deque<string>& proxies) {
+ // Call all the occurrences of the |source_url| and erase them.
+ auto lower_end = callbacks_.lower_bound(source_url);
+ auto upper_end = callbacks_.upper_bound(source_url);
+ for (auto it = lower_end; it != upper_end; ++it) {
+ ProxyRequestData* request = it->second.get();
+ MessageLoop::current()->CancelTask(request->timeout_id);
+ request->callback.Run(proxies);
}
- return true;
+ callbacks_.erase(lower_end, upper_end);
}
void ChromeBrowserProxyResolver::OnSignalConnected(const string& interface_name,
@@ -131,21 +134,21 @@
const string& source_url,
const string& proxy_info,
const string& error_message) {
- pair<ProxiesResolvedFn, void*> callback;
- TEST_AND_RETURN(DeleteUrlState(source_url, true, &callback));
if (!error_message.empty()) {
LOG(WARNING) << "ProxyResolved error: " << error_message;
}
- (*callback.first)(ParseProxyString(proxy_info), callback.second);
+ ProcessUrlResponse(source_url, ParseProxyString(proxy_info));
}
-void ChromeBrowserProxyResolver::HandleTimeout(string source_url) {
+void ChromeBrowserProxyResolver::HandleTimeout(string source_url,
+ ProxyRequestData* request) {
LOG(INFO) << "Timeout handler called. Seems Chrome isn't responding.";
- pair<ProxiesResolvedFn, void*> callback;
- TEST_AND_RETURN(DeleteUrlState(source_url, false, &callback));
- deque<string> proxies;
- proxies.push_back(kNoProxy);
- (*callback.first)(proxies, callback.second);
+ // Mark the timer_id that produced this callback as invalid to prevent
+ // canceling the timeout callback that already fired.
+ request->timeout_id = MessageLoop::kTaskIdNull;
+
+ deque<string> proxies = {kNoProxy};
+ ProcessUrlResponse(source_url, proxies);
}
deque<string> ChromeBrowserProxyResolver::ParseProxyString(
diff --git a/chrome_browser_proxy_resolver.h b/chrome_browser_proxy_resolver.h
index 84b0c28..eb92bac 100644
--- a/chrome_browser_proxy_resolver.h
+++ b/chrome_browser_proxy_resolver.h
@@ -18,9 +18,7 @@
#define UPDATE_ENGINE_CHROME_BROWSER_PROXY_RESOLVER_H_
#include <deque>
-#include <map>
#include <string>
-#include <utility>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
@@ -43,16 +41,19 @@
// Initialize the ProxyResolver using the provided DBus proxies.
bool Init();
- bool GetProxiesForUrl(const std::string& url,
- ProxiesResolvedFn callback,
- void* data) override;
+ ProxyRequestId GetProxiesForUrl(const std::string& url,
+ const ProxiesResolvedFn& callback) override;
+ bool CancelProxyRequest(ProxyRequestId request) override;
private:
FRIEND_TEST(ChromeBrowserProxyResolverTest, ParseTest);
FRIEND_TEST(ChromeBrowserProxyResolverTest, SuccessTest);
- typedef std::multimap<std::string, std::pair<ProxiesResolvedFn, void*>>
+ struct ProxyRequestData {
+ brillo::MessageLoop::TaskId timeout_id;
+ ProxiesResolvedFn callback;
+ };
+ typedef std::multimap<std::string, std::unique_ptr<ProxyRequestData>>
CallbacksMap;
- typedef std::multimap<std::string, brillo::MessageLoop::TaskId> TimeoutsMap;
// Called when the signal in UpdateEngineLibcrosProxyResolvedInterface is
// connected.
@@ -65,19 +66,19 @@
const std::string& proxy_info,
const std::string& error_message);
- // Handle no reply:
- void HandleTimeout(std::string source_url);
+ // Handle no reply. The |request| pointer points to the ProxyRequestData in
+ // the |callbacks_| map that triggered this timeout.
+ void HandleTimeout(std::string source_url, ProxyRequestData* request);
// Parses a string-encoded list of proxies and returns a deque
// of individual proxies. The last one will always be kNoProxy.
static std::deque<std::string> ParseProxyString(const std::string& input);
- // Deletes internal state for the first instance of url in the state.
- // If delete_timer is set, calls CancelTask on the timer id.
- // Returns the callback in an out parameter. Returns true on success.
- bool DeleteUrlState(const std::string& url,
- bool delete_timer,
- std::pair<ProxiesResolvedFn, void*>* callback);
+ // Process a proxy response by calling all the callbacks associated with the
+ // passed |source_url|. All the timeouts associated with these callbacks will
+ // be removed.
+ void ProcessUrlResponse(const std::string& source_url,
+ const std::deque<std::string>& proxies);
// Shutdown the dbus proxy object.
void Shutdown();
@@ -88,7 +89,6 @@
LibCrosProxy* libcros_proxy_;
int timeout_;
- TimeoutsMap timers_;
CallbacksMap callbacks_;
DISALLOW_COPY_AND_ASSIGN(ChromeBrowserProxyResolver);
};
diff --git a/chrome_browser_proxy_resolver_unittest.cc b/chrome_browser_proxy_resolver_unittest.cc
index bb5193e..24f8a2e 100644
--- a/chrome_browser_proxy_resolver_unittest.cc
+++ b/chrome_browser_proxy_resolver_unittest.cc
@@ -77,7 +77,6 @@
void RunTest(bool chrome_replies, bool chrome_alive);
- private:
brillo::FakeMessageLoop loop_{nullptr};
// Local pointers to the mocks. The instances are owned by the
@@ -107,15 +106,14 @@
}
namespace {
-void CheckResponseResolved(const deque<string>& proxies,
- void* /* pirv_data */) {
+void CheckResponseResolved(const deque<string>& proxies) {
EXPECT_EQ(2U, proxies.size());
EXPECT_EQ("socks5://192.168.52.83:5555", proxies[0]);
EXPECT_EQ(kNoProxy, proxies[1]);
MessageLoop::current()->BreakLoop();
}
-void CheckResponseNoReply(const deque<string>& proxies, void* /* pirv_data */) {
+void CheckResponseNoReply(const deque<string>& proxies) {
EXPECT_EQ(1U, proxies.size());
EXPECT_EQ(kNoProxy, proxies[0]);
MessageLoop::current()->BreakLoop();
@@ -138,9 +136,9 @@
_))
.WillOnce(Return(chrome_alive));
- ProxiesResolvedFn get_proxies_response = &CheckResponseNoReply;
+ ProxiesResolvedFn get_proxies_response = base::Bind(&CheckResponseNoReply);
if (chrome_replies) {
- get_proxies_response = &CheckResponseResolved;
+ get_proxies_response = base::Bind(&CheckResponseResolved);
MessageLoop::current()->PostDelayedTask(
FROM_HERE,
base::Bind(&ChromeBrowserProxyResolverTest::SendReplySignal,
@@ -151,7 +149,8 @@
base::TimeDelta::FromSeconds(1));
}
- EXPECT_TRUE(resolver_.GetProxiesForUrl(kUrl, get_proxies_response, nullptr));
+ EXPECT_NE(kProxyRequestIdNull,
+ resolver_.GetProxiesForUrl(kUrl, get_proxies_response));
MessageLoop::current()->Run();
}
@@ -208,4 +207,32 @@
RunTest(false, false);
}
+TEST_F(ChromeBrowserProxyResolverTest, CancelCallbackTest) {
+ int called = 0;
+ auto callback = base::Bind(
+ [](int* called, const deque<string>& proxies) { (*called)++; }, &called);
+
+ EXPECT_CALL(*service_interface_mock_, ResolveNetworkProxy(_, _, _, _, _))
+ .Times(4)
+ .WillRepeatedly(Return(true));
+
+ EXPECT_NE(kProxyRequestIdNull,
+ resolver_.GetProxiesForUrl("http://urlA", callback));
+ ProxyRequestId req_b = resolver_.GetProxiesForUrl("http://urlB", callback);
+ // Note that we add twice the same url.
+ ProxyRequestId req_c = resolver_.GetProxiesForUrl("http://urlC", callback);
+ EXPECT_NE(kProxyRequestIdNull,
+ resolver_.GetProxiesForUrl("http://urlC", callback));
+
+ EXPECT_EQ(0, called);
+ EXPECT_TRUE(resolver_.CancelProxyRequest(req_b));
+ EXPECT_TRUE(resolver_.CancelProxyRequest(req_c));
+ // Canceling the same request twice should fail even if there's another
+ // request for the same URL.
+ EXPECT_FALSE(resolver_.CancelProxyRequest(req_c));
+
+ loop_.Run();
+ EXPECT_EQ(2, called);
+}
+
} // namespace chromeos_update_engine
diff --git a/common/http_fetcher.cc b/common/http_fetcher.cc
index 400b43c..e592cc5 100644
--- a/common/http_fetcher.cc
+++ b/common/http_fetcher.cc
@@ -59,9 +59,8 @@
base::Unretained(this)));
return true;
}
- return proxy_resolver_->GetProxiesForUrl(url,
- &HttpFetcher::StaticProxiesResolved,
- this);
+ return proxy_resolver_->GetProxiesForUrl(
+ url, base::Bind(&HttpFetcher::ProxiesResolved, base::Unretained(this)));
}
void HttpFetcher::NoProxyResolverCallback() {
diff --git a/common/http_fetcher.h b/common/http_fetcher.h
index d2499eb..9f81879 100644
--- a/common/http_fetcher.h
+++ b/common/http_fetcher.h
@@ -165,10 +165,6 @@
private:
// Callback from the proxy resolver
void ProxiesResolved(const std::deque<std::string>& proxies);
- static void StaticProxiesResolved(const std::deque<std::string>& proxies,
- void* data) {
- reinterpret_cast<HttpFetcher*>(data)->ProxiesResolved(proxies);
- }
// Callback used to run the proxy resolver callback when there is no
// |proxy_resolver_|.
diff --git a/common/http_fetcher_unittest.cc b/common/http_fetcher_unittest.cc
index 0f34475..11a20e9 100644
--- a/common/http_fetcher_unittest.cc
+++ b/common/http_fetcher_unittest.cc
@@ -622,12 +622,9 @@
unique_ptr<HttpFetcher> fetcher(this->test_.NewLargeFetcher(&mock_resolver));
// Saved arguments from the proxy call.
- ProxiesResolvedFn proxy_callback = nullptr;
- void* proxy_data = nullptr;
-
- EXPECT_CALL(mock_resolver, GetProxiesForUrl("http://fake_url", _, _))
- .WillOnce(DoAll(
- SaveArg<1>(&proxy_callback), SaveArg<2>(&proxy_data), Return(true)));
+ ProxiesResolvedFn proxy_callback;
+ EXPECT_CALL(mock_resolver, GetProxiesForUrl("http://fake_url", _))
+ .WillOnce(DoAll(SaveArg<1>(&proxy_callback), Return(true)));
fetcher->BeginTransfer("http://fake_url");
testing::Mock::VerifyAndClearExpectations(&mock_resolver);
@@ -637,7 +634,7 @@
fetcher->Pause();
// Proxy resolver comes back after we paused the fetcher.
ASSERT_TRUE(proxy_callback);
- (*proxy_callback)({1, kNoProxy}, proxy_data);
+ proxy_callback.Run({1, kNoProxy});
}
namespace {
diff --git a/mock_proxy_resolver.h b/mock_proxy_resolver.h
index 0595f5a..bd6d04f 100644
--- a/mock_proxy_resolver.h
+++ b/mock_proxy_resolver.h
@@ -27,10 +27,10 @@
class MockProxyResolver : public ProxyResolver {
public:
- MOCK_METHOD3(GetProxiesForUrl,
- bool(const std::string& url,
- ProxiesResolvedFn callback,
- void* data));
+ MOCK_METHOD2(GetProxiesForUrl,
+ ProxyRequestId(const std::string& url,
+ const ProxiesResolvedFn& callback));
+ MOCK_METHOD1(CancelProxyRequest, bool(ProxyRequestId request));
};
} // namespace chromeos_update_engine
diff --git a/proxy_resolver.cc b/proxy_resolver.cc
index abd6f76..2ec59db 100644
--- a/proxy_resolver.cc
+++ b/proxy_resolver.cc
@@ -26,6 +26,7 @@
namespace chromeos_update_engine {
const char kNoProxy[] = "direct://";
+const ProxyRequestId kProxyRequestIdNull = brillo::MessageLoop::kTaskIdNull;
DirectProxyResolver::~DirectProxyResolver() {
if (idle_callback_id_ != MessageLoop::kTaskIdNull) {
@@ -39,27 +40,27 @@
}
}
-bool DirectProxyResolver::GetProxiesForUrl(const string& url,
- ProxiesResolvedFn callback,
- void* data) {
+ProxyRequestId DirectProxyResolver::GetProxiesForUrl(
+ const string& url, const ProxiesResolvedFn& callback) {
idle_callback_id_ = MessageLoop::current()->PostTask(
FROM_HERE,
- base::Bind(
- &DirectProxyResolver::ReturnCallback,
- base::Unretained(this),
- callback,
- data));
- return true;
+ base::Bind(&DirectProxyResolver::ReturnCallback,
+ base::Unretained(this),
+ callback));
+ return idle_callback_id_;
}
-void DirectProxyResolver::ReturnCallback(ProxiesResolvedFn callback,
- void* data) {
+bool DirectProxyResolver::CancelProxyRequest(ProxyRequestId request) {
+ return MessageLoop::current()->CancelTask(request);
+}
+
+void DirectProxyResolver::ReturnCallback(const ProxiesResolvedFn& callback) {
idle_callback_id_ = MessageLoop::kTaskIdNull;
// Initialize proxy pool with as many proxies as indicated (all identical).
deque<string> proxies(num_proxies_, kNoProxy);
- (*callback)(proxies, data);
+ callback.Run(proxies);
}
diff --git a/proxy_resolver.h b/proxy_resolver.h
index 2c8824f..19a400f 100644
--- a/proxy_resolver.h
+++ b/proxy_resolver.h
@@ -35,8 +35,15 @@
// http://<host>[:<port>] - HTTP proxy
// socks{4,5}://<host>[:<port>] - SOCKS4/5 proxy
// kNoProxy - no proxy
-typedef void (*ProxiesResolvedFn)(const std::deque<std::string>& proxies,
- void* data);
+typedef base::Callback<void(const std::deque<std::string>& proxies)>
+ ProxiesResolvedFn;
+
+// An id that identifies a proxy request. Used to cancel an ongoing request
+// before the callback is called.
+typedef brillo::MessageLoop::TaskId ProxyRequestId;
+
+// A constant identifying an invalid ProxyRequestId.
+extern const ProxyRequestId kProxyRequestIdNull;
class ProxyResolver {
public:
@@ -44,11 +51,14 @@
virtual ~ProxyResolver() {}
// Finds proxies for the given URL and returns them via the callback.
- // |data| will be passed to the callback.
- // Returns true on success.
- virtual bool GetProxiesForUrl(const std::string& url,
- ProxiesResolvedFn callback,
- void* data) = 0;
+ // Returns the id of the pending request on success or kProxyRequestIdNull
+ // otherwise.
+ virtual ProxyRequestId GetProxiesForUrl(
+ const std::string& url, const ProxiesResolvedFn& callback) = 0;
+
+ // Cancel the proxy resolution request initiated by GetProxiesForUrl(). The
+ // |request| value must be the one provided by GetProxiesForUrl().
+ virtual bool CancelProxyRequest(ProxyRequestId request) = 0;
private:
DISALLOW_COPY_AND_ASSIGN(ProxyResolver);
@@ -59,9 +69,9 @@
public:
DirectProxyResolver() = default;
~DirectProxyResolver() override;
- bool GetProxiesForUrl(const std::string& url,
- ProxiesResolvedFn callback,
- void* data) override;
+ ProxyRequestId GetProxiesForUrl(const std::string& url,
+ const ProxiesResolvedFn& callback) override;
+ bool CancelProxyRequest(ProxyRequestId request) override;
// Set the number of direct (non-) proxies to be returned by resolver.
// The default value is 1; higher numbers are currently used in testing.
@@ -79,7 +89,7 @@
size_t num_proxies_{1};
// The MainLoop callback, from here we return to the client.
- void ReturnCallback(ProxiesResolvedFn callback, void* data);
+ void ReturnCallback(const ProxiesResolvedFn& callback);
DISALLOW_COPY_AND_ASSIGN(DirectProxyResolver);
};
diff --git a/proxy_resolver_unittest.cc b/proxy_resolver_unittest.cc
new file mode 100644
index 0000000..070b361
--- /dev/null
+++ b/proxy_resolver_unittest.cc
@@ -0,0 +1,92 @@
+//
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+#include "update_engine/proxy_resolver.h"
+
+#include <deque>
+#include <string>
+
+#include <gtest/gtest.h>
+
+#include <base/bind.h>
+#include <brillo/bind_lambda.h>
+#include <brillo/message_loops/fake_message_loop.h>
+
+using std::deque;
+using std::string;
+
+namespace chromeos_update_engine {
+
+class ProxyResolverTest : public ::testing::Test {
+ protected:
+ virtual ~ProxyResolverTest() = default;
+
+ void SetUp() override { loop_.SetAsCurrent(); }
+
+ void TearDown() override { EXPECT_FALSE(loop_.PendingTasks()); }
+
+ brillo::FakeMessageLoop loop_{nullptr};
+ DirectProxyResolver resolver_;
+};
+
+TEST_F(ProxyResolverTest, DirectProxyResolverCallbackTest) {
+ bool called = false;
+ deque<string> callback_proxies;
+ auto callback = base::Bind(
+ [](bool* called,
+ deque<string>* callback_proxies,
+ const deque<string>& proxies) {
+ *called = true;
+ *callback_proxies = proxies;
+ },
+ &called,
+ &callback_proxies);
+
+ EXPECT_NE(kProxyRequestIdNull,
+ resolver_.GetProxiesForUrl("http://foo", callback));
+ // Check the callback is not called until the message loop runs.
+ EXPECT_FALSE(called);
+ loop_.Run();
+ EXPECT_TRUE(called);
+ EXPECT_EQ(kNoProxy, callback_proxies.front());
+}
+
+TEST_F(ProxyResolverTest, DirectProxyResolverCancelCallbackTest) {
+ bool called = false;
+ auto callback = base::Bind(
+ [](bool* called, const deque<string>& proxies) { *called = true; },
+ &called);
+
+ ProxyRequestId request = resolver_.GetProxiesForUrl("http://foo", callback);
+ EXPECT_FALSE(called);
+ EXPECT_TRUE(resolver_.CancelProxyRequest(request));
+ loop_.Run();
+ EXPECT_FALSE(called);
+}
+
+TEST_F(ProxyResolverTest, DirectProxyResolverSimultaneousCallbacksTest) {
+ int called = 0;
+ auto callback = base::Bind(
+ [](int* called, const deque<string>& proxies) { (*called)++; }, &called);
+
+ resolver_.GetProxiesForUrl("http://foo", callback);
+ resolver_.GetProxiesForUrl("http://bar", callback);
+ EXPECT_EQ(0, called);
+ loop_.Run();
+ EXPECT_EQ(2, called);
+}
+
+} // namespace chromeos_update_engine
diff --git a/update_engine.gyp b/update_engine.gyp
index e6ff776..31296a5 100644
--- a/update_engine.gyp
+++ b/update_engine.gyp
@@ -551,6 +551,7 @@
'payload_generator/topological_sort_unittest.cc',
'payload_generator/zip_unittest.cc',
'payload_state_unittest.cc',
+ 'proxy_resolver_unittest.cc',
'update_attempter_unittest.cc',
'update_manager/boxed_value_unittest.cc',
'update_manager/chromeos_policy_unittest.cc',