From 02cf5a4cb28d86669bcc4c3934f51437795b05ac Mon Sep 17 00:00:00 2001 From: "eroman@chromium.org" Date: Tue, 12 Jan 2010 22:10:25 +0000 Subject: Retry proxies which were cached as bad before giving up. This morphs ProxyList::RemoveBadProxies() into ProxyList::DeprioritizeBadProxies(), such that "bad proxies" are moved to the end of the fallback list rather than removed alltogether. BUG=31983 TEST=ProxyListTest.DeprioritizeBadProxies Review URL: http://codereview.chromium.org/542029 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@36054 0039d316-1c4b-4281-b951-d872f2087c98 --- net/base/net_error_list.h | 2 +- net/http/http_network_transaction.cc | 11 ++---- net/proxy/proxy_info.h | 7 ++-- net/proxy/proxy_list.cc | 17 ++++++--- net/proxy/proxy_list.h | 5 +-- net/proxy/proxy_list_unittest.cc | 69 +++++++++++++++++++++++++++++++++--- net/proxy/proxy_service.cc | 2 +- net/proxy/proxy_service_unittest.cc | 14 ++++++-- 8 files changed, 101 insertions(+), 26 deletions(-) diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h index 9502018..3bc2c3e 100644 --- a/net/base/net_error_list.h +++ b/net/base/net_error_list.h @@ -289,7 +289,7 @@ NET_ERROR(UNRECOGNIZED_FTP_DIRECTORY_LISTING_FORMAT, -334) NET_ERROR(INVALID_FLIP_STREAM, -335) // There are no supported proxies in the provided list. -NET_ERROR(EMPTY_PROXY_LIST, -336) +NET_ERROR(NO_SUPPORTED_PROXIES, -336) // The cache does not have the requested entry. NET_ERROR(CACHE_MISS, -400) diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc index ea61c79..d3fed93 100644 --- a/net/http/http_network_transaction.cc +++ b/net/http/http_network_transaction.cc @@ -611,14 +611,9 @@ int HttpNetworkTransaction::DoResolveProxyComplete(int result) { // http://www.chromium.org/developers/design-documents/proxy-settings-fallback if (proxy_info_.is_empty()) { - // No proxies/direct to choose from. This can happen when: - // a. We don't support any of the proxies in the returned list. - // b. The proxy service returned us an empty list. - // 1. this can happen if all the proxies were marked as bad already. - // - // TODO(eroman): in case (b.1) it would be better to just try the bad - // proxies again rather than failing without having tried anything! - return ERR_EMPTY_PROXY_LIST; + // No proxies/direct to choose from. This happens when we don't support any + // of the proxies in the returned list. + return ERR_NO_SUPPORTED_PROXIES; } if (result != OK) { diff --git a/net/proxy/proxy_info.h b/net/proxy/proxy_info.h index d65348e..b18feb8 100644 --- a/net/proxy/proxy_info.h +++ b/net/proxy/proxy_info.h @@ -68,9 +68,10 @@ class ProxyInfo { return proxy_list_.Fallback(proxy_retry_info); } - // Remove all proxies known to be bad from the proxy list. - void RemoveBadProxies(const ProxyRetryInfoMap& proxy_retry_info) { - proxy_list_.RemoveBadProxies(proxy_retry_info); + // De-prioritizes the proxies that we have cached as not working, by moving + // them to the end of the proxy list. + void DeprioritizeBadProxies(const ProxyRetryInfoMap& proxy_retry_info) { + proxy_list_.DeprioritizeBadProxies(proxy_retry_info); } // Delete any entry which doesn't have one of the specified proxy schemes. diff --git a/net/proxy/proxy_list.cc b/net/proxy/proxy_list.cc index 924fd15..b119b13 100644 --- a/net/proxy/proxy_list.cc +++ b/net/proxy/proxy_list.cc @@ -31,8 +31,14 @@ void ProxyList::SetSingleProxyServer(const ProxyServer& proxy_server) { proxies_.push_back(proxy_server); } -void ProxyList::RemoveBadProxies(const ProxyRetryInfoMap& proxy_retry_info) { - std::vector new_proxy_list; +void ProxyList::DeprioritizeBadProxies( + const ProxyRetryInfoMap& proxy_retry_info) { + // Partition the proxy list in two: + // (1) the known bad proxies + // (2) everything else + std::vector good_proxies; + std::vector bad_proxies; + std::vector::const_iterator iter = proxies_.begin(); for (; iter != proxies_.end(); ++iter) { ProxyRetryInfoMap::const_iterator bad_proxy = @@ -41,13 +47,16 @@ void ProxyList::RemoveBadProxies(const ProxyRetryInfoMap& proxy_retry_info) { // This proxy is bad. Check if it's time to retry. if (bad_proxy->second.bad_until >= TimeTicks::Now()) { // still invalid. + bad_proxies.push_back(*iter); continue; } } - new_proxy_list.push_back(*iter); + good_proxies.push_back(*iter); } - proxies_ = new_proxy_list; + // "proxies_ = good_proxies + bad_proxies" + proxies_.swap(good_proxies); + proxies_.insert(proxies_.end(), bad_proxies.begin(), bad_proxies.end()); } void ProxyList::RemoveProxiesWithoutScheme(int scheme_bit_field) { diff --git a/net/proxy/proxy_list.h b/net/proxy/proxy_list.h index f05d4cf..1afd1d9 100644 --- a/net/proxy/proxy_list.h +++ b/net/proxy/proxy_list.h @@ -25,8 +25,9 @@ class ProxyList { // Set the proxy list to a single entry, |proxy_server|. void SetSingleProxyServer(const ProxyServer& proxy_server); - // Remove all proxies known to be bad from the proxy list. - void RemoveBadProxies(const ProxyRetryInfoMap& proxy_retry_info); + // De-prioritizes the proxies that we have cached as not working, by moving + // them to the end of the fallback list. + void DeprioritizeBadProxies(const ProxyRetryInfoMap& proxy_retry_info); // Delete any entry which doesn't have one of the specified proxy schemes. // |scheme_bit_field| is a bunch of ProxyServer::Scheme bitwise ORed together. diff --git a/net/proxy/proxy_list_unittest.cc b/net/proxy/proxy_list_unittest.cc index 401461f..397bb7c 100644 --- a/net/proxy/proxy_list_unittest.cc +++ b/net/proxy/proxy_list_unittest.cc @@ -5,6 +5,10 @@ #include "net/proxy/proxy_list.h" #include "testing/gtest/include/gtest/gtest.h" +namespace net { + +namespace { + // Test parsing from a PAC string. TEST(ProxyListTest, SetFromPacString) { const struct { @@ -50,7 +54,7 @@ TEST(ProxyListTest, SetFromPacString) { }; for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) { - net::ProxyList list; + ProxyList list; list.SetFromPacString(tests[i].pac_input); EXPECT_EQ(tests[i].pac_output, list.ToPacString()); EXPECT_FALSE(list.IsEmpty()); @@ -65,20 +69,77 @@ TEST(ProxyListTest, RemoveProxiesWithoutScheme) { } tests[] = { { "PROXY foopy:10 ; SOCKS5 foopy2 ; SOCKS foopy11 ; PROXY foopy3 ; DIRECT", // Remove anything that isn't HTTP or DIRECT. - net::ProxyServer::SCHEME_DIRECT | net::ProxyServer::SCHEME_HTTP, + ProxyServer::SCHEME_DIRECT | ProxyServer::SCHEME_HTTP, "PROXY foopy:10;PROXY foopy3:80;DIRECT", }, { "PROXY foopy:10 ; SOCKS5 foopy2", // Remove anything that isn't HTTP or SOCKS5. - net::ProxyServer::SCHEME_DIRECT | net::ProxyServer::SCHEME_SOCKS4, + ProxyServer::SCHEME_DIRECT | ProxyServer::SCHEME_SOCKS4, "", }, }; for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) { - net::ProxyList list; + ProxyList list; list.SetFromPacString(tests[i].pac_input); list.RemoveProxiesWithoutScheme(tests[i].filter); EXPECT_EQ(tests[i].filtered_pac_output, list.ToPacString()); } } + +TEST(ProxyListTest, DeprioritizeBadProxies) { + // Retry info that marks a proxy as being bad for a *very* long time (to avoid + // the test depending on the current time.) + ProxyRetryInfo proxy_retry_info; + proxy_retry_info.bad_until = + base::TimeTicks::Now() + base::TimeDelta::FromDays(1); + + // Call DeprioritizeBadProxies with an empty map -- should have no effect. + { + ProxyList list; + list.SetFromPacString("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80"); + + ProxyRetryInfoMap retry_info_map; + list.DeprioritizeBadProxies(retry_info_map); + EXPECT_EQ("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80", + list.ToPacString()); + } + + // Call DeprioritizeBadProxies with 2 of the three proxies marked as bad. + // These proxies should be retried last. + { + ProxyList list; + list.SetFromPacString("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80"); + + ProxyRetryInfoMap retry_info_map; + retry_info_map["foopy1:80"] = proxy_retry_info; + retry_info_map["foopy3:80"] = proxy_retry_info; + retry_info_map["socks5://localhost:1080"] = proxy_retry_info; + + list.DeprioritizeBadProxies(retry_info_map); + + EXPECT_EQ("PROXY foopy2:80;PROXY foopy1:80;PROXY foopy3:80", + list.ToPacString()); + } + + // Call DeprioritizeBadProxies where ALL of the proxies are marked as bad. + // This should have no effect on the order. + { + ProxyList list; + list.SetFromPacString("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80"); + + ProxyRetryInfoMap retry_info_map; + retry_info_map["foopy1:80"] = proxy_retry_info; + retry_info_map["foopy2:80"] = proxy_retry_info; + retry_info_map["foopy3:80"] = proxy_retry_info; + + list.DeprioritizeBadProxies(retry_info_map); + + EXPECT_EQ("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80", + list.ToPacString()); + } +} + +} // namesapce + +} // namespace net diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc index 8160e64..b90d781 100644 --- a/net/proxy/proxy_service.cc +++ b/net/proxy/proxy_service.cc @@ -498,7 +498,7 @@ int ProxyService::DidFinishResolvingProxy(ProxyInfo* result, // Clean up the results list. if (result_code == OK) - result->RemoveBadProxies(proxy_retry_info_); + result->DeprioritizeBadProxies(proxy_retry_info_); LoadLog::EndEvent(load_log, LoadLog::TYPE_PROXY_SERVICE); return result_code; diff --git a/net/proxy/proxy_service_unittest.cc b/net/proxy/proxy_service_unittest.cc index f220ce1..0178dbe 100644 --- a/net/proxy/proxy_service_unittest.cc +++ b/net/proxy/proxy_service_unittest.cc @@ -404,7 +404,7 @@ TEST(ProxyServiceTest, ProxyFallback) { EXPECT_EQ(url, resolver->pending_requests()[0]->url()); // Set the result in proxy resolver -- the second result is already known - // to be bad. + // to be bad, so we will not try to use it initially. resolver->pending_requests()[0]->results()->UseNamedProxy( "foopy3:7070;foopy1:8080;foopy2:9090"); resolver->pending_requests()[0]->CompleteNow(OK); @@ -419,10 +419,18 @@ TEST(ProxyServiceTest, ProxyFallback) { EXPECT_EQ(OK, rv); EXPECT_EQ("foopy2:9090", info.proxy_server().ToURI()); - // Fake another error, the last proxy is gone, the list should now be empty, - // so there is nothing left to try. + // We fake another error. At this point we have tried all of the + // proxy servers we thought were valid; next we try the proxy server + // that was in our bad proxies map (foopy1:8080). TestCompletionCallback callback5; rv = service->ReconsiderProxyAfterError(url, &info, &callback5, NULL, NULL); + EXPECT_EQ(OK, rv); + EXPECT_EQ("foopy1:8080", info.proxy_server().ToURI()); + + // Fake another error, the last proxy is gone, the list should now be empty, + // so there is nothing left to try. + TestCompletionCallback callback6; + rv = service->ReconsiderProxyAfterError(url, &info, &callback6, NULL, NULL); EXPECT_EQ(ERR_FAILED, rv); EXPECT_FALSE(info.is_direct()); EXPECT_TRUE(info.is_empty()); -- cgit v1.1