diff options
author | joaodasilva@chromium.org <joaodasilva@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-12-14 22:40:42 +0000 |
---|---|---|
committer | joaodasilva@chromium.org <joaodasilva@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-12-14 22:40:42 +0000 |
commit | 7af985a498165e9ba74d7e428be4ffe9a054f053 (patch) | |
tree | 88a20282a57a9b0bffc1d99f34e0ba7ae65f4063 /net | |
parent | ab108f2658ea9da9572085ea8bd501505f3f38c5 (diff) | |
download | chromium_src-7af985a498165e9ba74d7e428be4ffe9a054f053.zip chromium_src-7af985a498165e9ba74d7e428be4ffe9a054f053.tar.gz chromium_src-7af985a498165e9ba74d7e428be4ffe9a054f053.tar.bz2 |
Introduce ERR_NETWORK_CHANGED and allow URLFetcher to automatically retry on that error.
BUG=164363
Review URL: https://codereview.chromium.org/11464028
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@173227 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'net')
43 files changed, 537 insertions, 132 deletions
diff --git a/net/base/dnsrr_resolver.cc b/net/base/dnsrr_resolver.cc index cfe63e2..3fecfe3 100644 --- a/net/base/dnsrr_resolver.cc +++ b/net/base/dnsrr_resolver.cc @@ -509,11 +509,7 @@ class RRResolverJob { } ~RRResolverJob() { - if (worker_) { - worker_->Cancel(); - worker_ = NULL; - PostAll(ERR_ABORTED, NULL); - } + Cancel(ERR_ABORTED); } void AddHandle(RRResolverHandle* handle) { @@ -525,6 +521,14 @@ class RRResolverJob { PostAll(result, &response); } + void Cancel(int result) { + if (worker_) { + worker_->Cancel(); + worker_ = NULL; + PostAll(result, NULL); + } + } + private: void PostAll(int result, const RRResponse* response) { std::vector<RRResolverHandle*> handles; @@ -643,6 +647,9 @@ void DnsRRResolver::OnIPAddressChanged() { inflight.swap(inflight_); cache_.clear(); + std::map<std::pair<std::string, uint16>, RRResolverJob*>::iterator it; + for (it = inflight.begin(); it != inflight.end(); ++it) + it->second->Cancel(ERR_NETWORK_CHANGED); STLDeleteValues(&inflight); } diff --git a/net/base/dnsrr_resolver_unittest.cc b/net/base/dnsrr_resolver_unittest.cc index cc6e80b..0b67b65 100644 --- a/net/base/dnsrr_resolver_unittest.cc +++ b/net/base/dnsrr_resolver_unittest.cc @@ -97,7 +97,7 @@ TEST(DnsRRResolverTest, Resolve) { ASSERT_TRUE(handle != DnsRRResolver::kInvalidHandle); resolver.OnIPAddressChanged(); ASSERT_TRUE(callback.have_result()); - ASSERT_EQ(ERR_ABORTED, callback.WaitForResult()); + ASSERT_EQ(ERR_NETWORK_CHANGED, callback.WaitForResult()); ASSERT_EQ(4u, resolver.requests()); ASSERT_EQ(2u, resolver.cache_hits()); ASSERT_EQ(0u, resolver.inflight_joins()); diff --git a/net/base/host_resolver_impl.cc b/net/base/host_resolver_impl.cc index 64c9d2a..4b6bb3ad 100644 --- a/net/base/host_resolver_impl.cc +++ b/net/base/host_resolver_impl.cc @@ -1267,11 +1267,11 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job { } } - // Called from AbortAllInProgressJobs. Completes all requests as aborted - // and destroys the job. + // Called from AbortAllInProgressJobs. Completes all requests and destroys + // the job. This currently assumes the abort is due to a network change. void Abort() { DCHECK(is_running()); - CompleteRequestsWithError(ERR_ABORTED); + CompleteRequestsWithError(ERR_NETWORK_CHANGED); } // If DnsTask present, abort it and fall back to ProcTask. @@ -1531,7 +1531,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job { resolver_->received_dns_config_); } - bool did_complete = (entry.error != ERR_ABORTED) && + bool did_complete = (entry.error != ERR_NETWORK_CHANGED) && (entry.error != ERR_HOST_RESOLVER_QUEUE_TOO_LARGE); if (did_complete) resolver_->CacheResult(key_, entry, ttl); diff --git a/net/base/host_resolver_impl.h b/net/base/host_resolver_impl.h index 61096641..d68d6ab 100644 --- a/net/base/host_resolver_impl.h +++ b/net/base/host_resolver_impl.h @@ -207,8 +207,8 @@ class NET_EXPORT HostResolverImpl // Removes |job| from |jobs_|, only if it exists. void RemoveJob(Job* job); - // Aborts all in progress jobs and notifies their requests. - // Might start new jobs. + // Aborts all in progress jobs with ERR_NETWORK_CHANGED and notifies their + // requests. Might start new jobs. void AbortAllInProgressJobs(); // Attempts to serve each Job in |jobs_| from the HOSTS file if we have diff --git a/net/base/host_resolver_impl_unittest.cc b/net/base/host_resolver_impl_unittest.cc index 08262a5..bc6b870 100644 --- a/net/base/host_resolver_impl_unittest.cc +++ b/net/base/host_resolver_impl_unittest.cc @@ -735,7 +735,7 @@ TEST_F(HostResolverImplTest, DeleteWithinAbortedCallback) { // |MyHandler| will send quit message once all the requests have finished. MessageLoop::current()->Run(); - EXPECT_EQ(ERR_ABORTED, requests_[0]->result()); + EXPECT_EQ(ERR_NETWORK_CHANGED, requests_[0]->result()); EXPECT_EQ(ERR_IO_PENDING, requests_[1]->result()); EXPECT_EQ(ERR_IO_PENDING, requests_[2]->result()); EXPECT_EQ(ERR_IO_PENDING, requests_[3]->result()); @@ -830,7 +830,7 @@ TEST_F(HostResolverImplTest, FlushCacheOnIPAddressChange) { EXPECT_EQ(OK, req->WaitForResult()); } -// Test that IP address changes send ERR_ABORTED to pending requests. +// Test that IP address changes send ERR_NETWORK_CHANGED to pending requests. TEST_F(HostResolverImplTest, AbortOnIPAddressChanged) { Request* req = CreateRequest("host1", 70); EXPECT_EQ(ERR_IO_PENDING, req->Resolve()); @@ -841,7 +841,7 @@ TEST_F(HostResolverImplTest, AbortOnIPAddressChanged) { MessageLoop::current()->RunUntilIdle(); // Notification happens async. proc_->SignalAll(); - EXPECT_EQ(ERR_ABORTED, req->WaitForResult()); + EXPECT_EQ(ERR_NETWORK_CHANGED, req->WaitForResult()); EXPECT_EQ(0u, resolver_->GetHostCache()->size()); } @@ -859,7 +859,7 @@ TEST_F(HostResolverImplTest, ObeyPoolConstraintsAfterIPAddressChange) { MessageLoop::current()->RunUntilIdle(); // Notification happens async. proc_->SignalMultiple(3u); // Let the false-start go so that we can catch it. - EXPECT_EQ(ERR_ABORTED, requests_[0]->WaitForResult()); + EXPECT_EQ(ERR_NETWORK_CHANGED, requests_[0]->WaitForResult()); EXPECT_EQ(1u, num_running_jobs()); @@ -901,9 +901,9 @@ TEST_F(HostResolverImplTest, AbortOnlyExistingRequestsOnIPAddressChange) { NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests(); // This should abort all running jobs. MessageLoop::current()->RunUntilIdle(); - EXPECT_EQ(ERR_ABORTED, requests_[0]->result()); - EXPECT_EQ(ERR_ABORTED, requests_[1]->result()); - EXPECT_EQ(ERR_ABORTED, requests_[2]->result()); + EXPECT_EQ(ERR_NETWORK_CHANGED, requests_[0]->result()); + EXPECT_EQ(ERR_NETWORK_CHANGED, requests_[1]->result()); + EXPECT_EQ(ERR_NETWORK_CHANGED, requests_[2]->result()); ASSERT_EQ(6u, requests_.size()); // Unblock all calls to proc. proc_->SignalMultiple(requests_.size()); diff --git a/net/base/mock_host_resolver.cc b/net/base/mock_host_resolver.cc index d86e209..a0835b8 100644 --- a/net/base/mock_host_resolver.cc +++ b/net/base/mock_host_resolver.cc @@ -84,10 +84,13 @@ int MockHostResolverBase::Resolve(const RequestInfo& info, requests_[id] = req; if (handle) *handle = reinterpret_cast<RequestHandle>(id); - MessageLoop::current()->PostTask(FROM_HERE, - base::Bind(&MockHostResolverBase::ResolveNow, - AsWeakPtr(), - id)); + + if (!ondemand_mode_) { + MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&MockHostResolverBase::ResolveNow, AsWeakPtr(), id)); + } + return ERR_IO_PENDING; } @@ -117,9 +120,21 @@ HostCache* MockHostResolverBase::GetHostCache() { return cache_.get(); } +void MockHostResolverBase::ResolveAllPending() { + DCHECK(CalledOnValidThread()); + DCHECK(ondemand_mode_); + for (RequestMap::iterator i = requests_.begin(); i != requests_.end(); ++i) { + MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&MockHostResolverBase::ResolveNow, AsWeakPtr(), i->first)); + } +} + // start id from 1 to distinguish from NULL RequestHandle MockHostResolverBase::MockHostResolverBase(bool use_caching) - : synchronous_mode_(false), next_request_id_(1) { + : synchronous_mode_(false), + ondemand_mode_(false), + next_request_id_(1) { rules_ = CreateCatchAllHostResolverProc(); if (use_caching) { diff --git a/net/base/mock_host_resolver.h b/net/base/mock_host_resolver.h index 033cf14..5542a64 100644 --- a/net/base/mock_host_resolver.h +++ b/net/base/mock_host_resolver.h @@ -65,6 +65,14 @@ class MockHostResolverBase : public HostResolver, synchronous_mode_ = is_synchronous; } + // Asynchronous requests are automatically resolved by default. + // If set_ondemand_mode() is set then Resolve() returns IO_PENDING and + // ResolveAllPending() must be explicitly invoked to resolve all requests + // that are pending. + void set_ondemand_mode(bool is_ondemand) { + ondemand_mode_ = is_ondemand; + } + // HostResolver methods: virtual int Resolve(const RequestInfo& info, AddressList* addresses, @@ -77,6 +85,15 @@ class MockHostResolverBase : public HostResolver, virtual void CancelRequest(RequestHandle req) OVERRIDE; virtual HostCache* GetHostCache() OVERRIDE; + // Resolves all pending requests. It is only valid to invoke this if + // set_ondemand_mode was set before. The requests are resolved asynchronously, + // after this call returns. + void ResolveAllPending(); + + // Returns true if there are pending requests that can be resolved by invoking + // ResolveAllPending(). + bool has_pending_requests() const { return !requests_.empty(); } + protected: explicit MockHostResolverBase(bool use_caching); @@ -94,6 +111,7 @@ class MockHostResolverBase : public HostResolver, void ResolveNow(size_t id); bool synchronous_mode_; + bool ondemand_mode_; scoped_refptr<RuleBasedHostResolverProc> rules_; scoped_ptr<HostCache> cache_; RequestMap requests_; diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h index fe98e42..44f0c8d 100644 --- a/net/base/net_error_list.h +++ b/net/base/net_error_list.h @@ -84,6 +84,9 @@ NET_ERROR(FILE_VIRUS_INFECTED, -19) // The client chose to block the request. NET_ERROR(BLOCKED_BY_CLIENT, -20) +// The network changed. +NET_ERROR(NETWORK_CHANGED, -21) + // A connection was closed (corresponding to a TCP FIN). NET_ERROR(CONNECTION_CLOSED, -100) diff --git a/net/base/network_change_notifier.h b/net/base/network_change_notifier.h index fe8d091..124e084 100644 --- a/net/base/network_change_notifier.h +++ b/net/base/network_change_notifier.h @@ -274,6 +274,7 @@ class NET_EXPORT NetworkChangeNotifier { friend class NetworkChangeNotifierAndroidTest; friend class NetworkChangeNotifierLinuxTest; friend class NetworkChangeNotifierWinTest; + friend class URLFetcherMockDnsTest; class NetworkState; class NetworkChangeCalculator; diff --git a/net/http/http_network_session.cc b/net/http/http_network_session.cc index 50a18c2..daace43 100644 --- a/net/http/http_network_session.cc +++ b/net/http/http_network_session.cc @@ -172,9 +172,9 @@ Value* HttpNetworkSession::SpdySessionPoolInfoToValue() const { } void HttpNetworkSession::CloseAllConnections() { - normal_socket_pool_manager_->FlushSocketPools(); - websocket_socket_pool_manager_->FlushSocketPools(); - spdy_session_pool_.CloseCurrentSessions(); + normal_socket_pool_manager_->FlushSocketPoolsWithError(ERR_ABORTED); + websocket_socket_pool_manager_->FlushSocketPoolsWithError(ERR_ABORTED); + spdy_session_pool_.CloseCurrentSessions(ERR_ABORTED); } void HttpNetworkSession::CloseIdleConnections() { diff --git a/net/http/http_pipelined_connection_impl.cc b/net/http/http_pipelined_connection_impl.cc index f604a26..c8bc02a 100644 --- a/net/http/http_pipelined_connection_impl.cc +++ b/net/http/http_pipelined_connection_impl.cc @@ -702,6 +702,7 @@ void HttpPipelinedConnectionImpl::CheckHeadersForPipelineCompatibility( // Collect metrics to see if this code is useful. case ERR_ABORTED: case ERR_INTERNET_DISCONNECTED: + case ERR_NETWORK_CHANGED: // These errors are no fault of the server. break; diff --git a/net/http/http_proxy_client_socket_pool.cc b/net/http/http_proxy_client_socket_pool.cc index 8991735..e612cd9 100644 --- a/net/http/http_proxy_client_socket_pool.cc +++ b/net/http/http_proxy_client_socket_pool.cc @@ -451,8 +451,8 @@ void HttpProxyClientSocketPool::ReleaseSocket(const std::string& group_name, base_.ReleaseSocket(group_name, socket, id); } -void HttpProxyClientSocketPool::Flush() { - base_.Flush(); +void HttpProxyClientSocketPool::FlushWithError(int error) { + base_.FlushWithError(error); } bool HttpProxyClientSocketPool::IsStalled() const { diff --git a/net/http/http_proxy_client_socket_pool.h b/net/http/http_proxy_client_socket_pool.h index 867131c..be380af 100644 --- a/net/http/http_proxy_client_socket_pool.h +++ b/net/http/http_proxy_client_socket_pool.h @@ -204,7 +204,7 @@ class NET_EXPORT_PRIVATE HttpProxyClientSocketPool StreamSocket* socket, int id) OVERRIDE; - virtual void Flush() OVERRIDE; + virtual void FlushWithError(int error) OVERRIDE; virtual bool IsStalled() const OVERRIDE; diff --git a/net/socket/client_socket_pool.h b/net/socket/client_socket_pool.h index 1d2246c..7a274b7 100644 --- a/net/socket/client_socket_pool.h +++ b/net/socket/client_socket_pool.h @@ -115,10 +115,11 @@ class NET_EXPORT ClientSocketPool { int id) = 0; // This flushes all state from the ClientSocketPool. This means that all - // idle and connecting sockets are discarded. Active sockets being - // held by ClientSocketPool clients will be discarded when released back to - // the pool. Does not flush any pools wrapped by |this|. - virtual void Flush() = 0; + // idle and connecting sockets are discarded with the given |error|. + // Active sockets being held by ClientSocketPool clients will be discarded + // when released back to the pool. + // Does not flush any pools wrapped by |this|. + virtual void FlushWithError(int error) = 0; // Returns true if a there is currently a request blocked on the // per-pool (not per-host) max socket limit. diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc index 0880724..3e9cd82 100644 --- a/net/socket/client_socket_pool_base.cc +++ b/net/socket/client_socket_pool_base.cc @@ -189,7 +189,7 @@ ClientSocketPoolBaseHelper::~ClientSocketPoolBaseHelper() { // Clean up any idle sockets and pending connect jobs. Assert that we have no // remaining active sockets or pending requests. They should have all been // cleaned up prior to |this| being destroyed. - Flush(); + FlushWithError(ERR_ABORTED); DCHECK(group_map_.empty()); DCHECK(pending_callback_map_.empty()); DCHECK_EQ(0, connecting_socket_count_); @@ -903,14 +903,14 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete( } void ClientSocketPoolBaseHelper::OnIPAddressChanged() { - Flush(); + FlushWithError(ERR_NETWORK_CHANGED); } -void ClientSocketPoolBaseHelper::Flush() { +void ClientSocketPoolBaseHelper::FlushWithError(int error) { pool_generation_number_++; CancelAllConnectJobs(); CloseIdleSockets(); - AbortAllRequests(); + CancelAllRequestsWithError(error); } bool ClientSocketPoolBaseHelper::IsStalled() const { @@ -1031,7 +1031,7 @@ void ClientSocketPoolBaseHelper::CancelAllConnectJobs() { DCHECK_EQ(0, connecting_socket_count_); } -void ClientSocketPoolBaseHelper::AbortAllRequests() { +void ClientSocketPoolBaseHelper::CancelAllRequestsWithError(int error) { for (GroupMap::iterator i = group_map_.begin(); i != group_map_.end();) { Group* group = i->second; @@ -1041,7 +1041,7 @@ void ClientSocketPoolBaseHelper::AbortAllRequests() { it2 != pending_requests.end(); ++it2) { scoped_ptr<const Request> request(*it2); InvokeUserCallbackLater( - request->handle(), request->callback(), ERR_ABORTED); + request->handle(), request->callback(), error); } // Delete group if no longer needed. diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h index e35ab29..39dbeb0 100644 --- a/net/socket/client_socket_pool_base.h +++ b/net/socket/client_socket_pool_base.h @@ -242,8 +242,8 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper StreamSocket* socket, int id); - // See ClientSocketPool::Flush for documentation on this function. - void Flush(); + // See ClientSocketPool::FlushWithError for documentation on this function. + void FlushWithError(int error); // See ClientSocketPool::IsStalled for documentation on this function. bool IsStalled() const; @@ -505,9 +505,9 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper // groups if they are no longer needed. void CancelAllConnectJobs(); - // Iterates through |group_map_|, posting ERR_ABORTED callbacks for all + // Iterates through |group_map_|, posting |error| callbacks for all // requests, and then deleting groups if they are no longer needed. - void AbortAllRequests(); + void CancelAllRequestsWithError(int error); // Returns true if we can't create any more sockets due to the total limit. bool ReachedMaxSocketsLimit() const; @@ -584,9 +584,9 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper // TODO(vandebo) Remove when backup jobs move to TransportClientSocketPool bool connect_backup_jobs_enabled_; - // A unique id for the pool. It gets incremented every time we Flush() the - // pool. This is so that when sockets get released back to the pool, we can - // make sure that they are discarded rather than reused. + // A unique id for the pool. It gets incremented every time we + // FlushWithError() the pool. This is so that when sockets get released back + // to the pool, we can make sure that they are discarded rather than reused. int pool_generation_number_; std::set<LayeredPool*> higher_layer_pools_; @@ -708,7 +708,7 @@ class ClientSocketPoolBase { return helper_.ReleaseSocket(group_name, socket, id); } - void Flush() { helper_.Flush(); } + void FlushWithError(int error) { helper_.FlushWithError(error); } bool IsStalled() const { return helper_.IsStalled(); } diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc index dbbeb5d..f55dadb 100644 --- a/net/socket/client_socket_pool_base_unittest.cc +++ b/net/socket/client_socket_pool_base_unittest.cc @@ -476,8 +476,8 @@ class TestClientSocketPool : public ClientSocketPool { base_.ReleaseSocket(group_name, socket, id); } - virtual void Flush() OVERRIDE { - base_.Flush(); + virtual void FlushWithError(int error) OVERRIDE { + base_.FlushWithError(error); } virtual bool IsStalled() const OVERRIDE { @@ -2486,7 +2486,7 @@ TEST_F(ClientSocketPoolBaseTest, CallbackThatReleasesPool) { pool_.get(), BoundNetLog())); - pool_->Flush(); + pool_->FlushWithError(ERR_NETWORK_CHANGED); // We'll call back into this now. callback.WaitForResult(); @@ -2507,7 +2507,7 @@ TEST_F(ClientSocketPoolBaseTest, DoNotReuseSocketAfterFlush) { EXPECT_EQ(OK, callback.WaitForResult()); EXPECT_EQ(ClientSocketHandle::UNUSED, handle.reuse_type()); - pool_->Flush(); + pool_->FlushWithError(ERR_NETWORK_CHANGED); handle.Reset(); MessageLoop::current()->RunUntilIdle(); @@ -2584,8 +2584,8 @@ TEST_F(ClientSocketPoolBaseTest, AbortAllRequestsOnFlush) { // Second job will be started during the first callback, and will // asynchronously complete with OK. connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob); - pool_->Flush(); - EXPECT_EQ(ERR_ABORTED, callback.WaitForResult()); + pool_->FlushWithError(ERR_NETWORK_CHANGED); + EXPECT_EQ(ERR_NETWORK_CHANGED, callback.WaitForResult()); EXPECT_EQ(OK, callback.WaitForNestedResult()); } diff --git a/net/socket/client_socket_pool_manager.h b/net/socket/client_socket_pool_manager.h index 69d77e4..a92d218 100644 --- a/net/socket/client_socket_pool_manager.h +++ b/net/socket/client_socket_pool_manager.h @@ -67,7 +67,7 @@ class NET_EXPORT_PRIVATE ClientSocketPoolManager { HttpNetworkSession::SocketPoolType pool_type, int socket_count); - virtual void FlushSocketPools() = 0; + virtual void FlushSocketPoolsWithError(int error) = 0; virtual void CloseIdleSockets() = 0; virtual TransportClientSocketPool* GetTransportSocketPool() = 0; virtual SSLClientSocketPool* GetSSLSocketPool() = 0; diff --git a/net/socket/client_socket_pool_manager_impl.cc b/net/socket/client_socket_pool_manager_impl.cc index a91bca9..bd46652 100644 --- a/net/socket/client_socket_pool_manager_impl.cc +++ b/net/socket/client_socket_pool_manager_impl.cc @@ -90,7 +90,7 @@ ClientSocketPoolManagerImpl::~ClientSocketPoolManagerImpl() { CertDatabase::GetInstance()->RemoveObserver(this); } -void ClientSocketPoolManagerImpl::FlushSocketPools() { +void ClientSocketPoolManagerImpl::FlushSocketPoolsWithError(int error) { // Flush the highest level pools first, since higher level pools may release // stuff to the lower level pools. @@ -98,46 +98,46 @@ void ClientSocketPoolManagerImpl::FlushSocketPools() { ssl_socket_pools_for_proxies_.begin(); it != ssl_socket_pools_for_proxies_.end(); ++it) - it->second->Flush(); + it->second->FlushWithError(error); for (HTTPProxySocketPoolMap::const_iterator it = http_proxy_socket_pools_.begin(); it != http_proxy_socket_pools_.end(); ++it) - it->second->Flush(); + it->second->FlushWithError(error); for (SSLSocketPoolMap::const_iterator it = ssl_socket_pools_for_https_proxies_.begin(); it != ssl_socket_pools_for_https_proxies_.end(); ++it) - it->second->Flush(); + it->second->FlushWithError(error); for (TransportSocketPoolMap::const_iterator it = transport_socket_pools_for_https_proxies_.begin(); it != transport_socket_pools_for_https_proxies_.end(); ++it) - it->second->Flush(); + it->second->FlushWithError(error); for (TransportSocketPoolMap::const_iterator it = transport_socket_pools_for_http_proxies_.begin(); it != transport_socket_pools_for_http_proxies_.end(); ++it) - it->second->Flush(); + it->second->FlushWithError(error); for (SOCKSSocketPoolMap::const_iterator it = socks_socket_pools_.begin(); it != socks_socket_pools_.end(); ++it) - it->second->Flush(); + it->second->FlushWithError(error); for (TransportSocketPoolMap::const_iterator it = transport_socket_pools_for_socks_proxies_.begin(); it != transport_socket_pools_for_socks_proxies_.end(); ++it) - it->second->Flush(); + it->second->FlushWithError(error); - ssl_socket_pool_->Flush(); - transport_socket_pool_->Flush(); + ssl_socket_pool_->FlushWithError(error); + transport_socket_pool_->FlushWithError(error); } void ClientSocketPoolManagerImpl::CloseIdleSockets() { @@ -372,7 +372,7 @@ Value* ClientSocketPoolManagerImpl::SocketPoolInfoToValue() const { } void ClientSocketPoolManagerImpl::OnCertAdded(const X509Certificate* cert) { - FlushSocketPools(); + FlushSocketPoolsWithError(ERR_NETWORK_CHANGED); } void ClientSocketPoolManagerImpl::OnCertTrustChanged( @@ -387,7 +387,7 @@ void ClientSocketPoolManagerImpl::OnCertTrustChanged( // Since the OnCertTrustChanged method doesn't tell us what // kind of trust change it is, we have to flush the socket // pools to be safe. - FlushSocketPools(); + FlushSocketPoolsWithError(ERR_NETWORK_CHANGED); } } // namespace net diff --git a/net/socket/client_socket_pool_manager_impl.h b/net/socket/client_socket_pool_manager_impl.h index 1b0de1d..ff5e5a2 100644 --- a/net/socket/client_socket_pool_manager_impl.h +++ b/net/socket/client_socket_pool_manager_impl.h @@ -68,7 +68,7 @@ class ClientSocketPoolManagerImpl : public base::NonThreadSafe, HttpNetworkSession::SocketPoolType pool_type); virtual ~ClientSocketPoolManagerImpl(); - virtual void FlushSocketPools() OVERRIDE; + virtual void FlushSocketPoolsWithError(int error) OVERRIDE; virtual void CloseIdleSockets() OVERRIDE; virtual TransportClientSocketPool* GetTransportSocketPool() OVERRIDE; diff --git a/net/socket/mock_client_socket_pool_manager.cc b/net/socket/mock_client_socket_pool_manager.cc index 7cb9b5b..0496adb 100644 --- a/net/socket/mock_client_socket_pool_manager.cc +++ b/net/socket/mock_client_socket_pool_manager.cc @@ -42,7 +42,7 @@ void MockClientSocketPoolManager::SetSocketPoolForSSLWithProxy( ssl_socket_pools_for_proxies_[proxy_server] = pool; } -void MockClientSocketPoolManager::FlushSocketPools() { +void MockClientSocketPoolManager::FlushSocketPoolsWithError(int error) { NOTIMPLEMENTED(); } diff --git a/net/socket/mock_client_socket_pool_manager.h b/net/socket/mock_client_socket_pool_manager.h index 5db6506..c2c3792 100644 --- a/net/socket/mock_client_socket_pool_manager.h +++ b/net/socket/mock_client_socket_pool_manager.h @@ -27,7 +27,7 @@ class MockClientSocketPoolManager : public ClientSocketPoolManager { SSLClientSocketPool* pool); // ClientSocketPoolManager methods: - virtual void FlushSocketPools() OVERRIDE; + virtual void FlushSocketPoolsWithError(int error) OVERRIDE; virtual void CloseIdleSockets() OVERRIDE; virtual TransportClientSocketPool* GetTransportSocketPool() OVERRIDE; virtual SSLClientSocketPool* GetSSLSocketPool() OVERRIDE; diff --git a/net/socket/socks_client_socket_pool.cc b/net/socket/socks_client_socket_pool.cc index a7c7ecb..dc181e4 100644 --- a/net/socket/socks_client_socket_pool.cc +++ b/net/socket/socks_client_socket_pool.cc @@ -242,8 +242,8 @@ void SOCKSClientSocketPool::ReleaseSocket(const std::string& group_name, base_.ReleaseSocket(group_name, socket, id); } -void SOCKSClientSocketPool::Flush() { - base_.Flush(); +void SOCKSClientSocketPool::FlushWithError(int error) { + base_.FlushWithError(error); } bool SOCKSClientSocketPool::IsStalled() const { diff --git a/net/socket/socks_client_socket_pool.h b/net/socket/socks_client_socket_pool.h index 090c29f..96c48426 100644 --- a/net/socket/socks_client_socket_pool.h +++ b/net/socket/socks_client_socket_pool.h @@ -137,7 +137,7 @@ class NET_EXPORT_PRIVATE SOCKSClientSocketPool StreamSocket* socket, int id) OVERRIDE; - virtual void Flush() OVERRIDE; + virtual void FlushWithError(int error) OVERRIDE; virtual bool IsStalled() const OVERRIDE; diff --git a/net/socket/ssl_client_socket_pool.cc b/net/socket/ssl_client_socket_pool.cc index 21ebf97..30ad46e 100644 --- a/net/socket/ssl_client_socket_pool.cc +++ b/net/socket/ssl_client_socket_pool.cc @@ -530,8 +530,8 @@ void SSLClientSocketPool::ReleaseSocket(const std::string& group_name, base_.ReleaseSocket(group_name, socket, id); } -void SSLClientSocketPool::Flush() { - base_.Flush(); +void SSLClientSocketPool::FlushWithError(int error) { + base_.FlushWithError(error); } bool SSLClientSocketPool::IsStalled() const { @@ -603,7 +603,7 @@ ClientSocketPoolHistograms* SSLClientSocketPool::histograms() const { } void SSLClientSocketPool::OnSSLConfigChanged() { - Flush(); + FlushWithError(ERR_NETWORK_CHANGED); } bool SSLClientSocketPool::CloseOneIdleConnection() { diff --git a/net/socket/ssl_client_socket_pool.h b/net/socket/ssl_client_socket_pool.h index 8802afc..6ee0155 100644 --- a/net/socket/ssl_client_socket_pool.h +++ b/net/socket/ssl_client_socket_pool.h @@ -206,7 +206,7 @@ class NET_EXPORT_PRIVATE SSLClientSocketPool StreamSocket* socket, int id) OVERRIDE; - virtual void Flush() OVERRIDE; + virtual void FlushWithError(int error) OVERRIDE; virtual bool IsStalled() const OVERRIDE; diff --git a/net/socket/transport_client_socket_pool.cc b/net/socket/transport_client_socket_pool.cc index bc49d98..84e89c8 100644 --- a/net/socket/transport_client_socket_pool.cc +++ b/net/socket/transport_client_socket_pool.cc @@ -419,8 +419,8 @@ void TransportClientSocketPool::ReleaseSocket( base_.ReleaseSocket(group_name, socket, id); } -void TransportClientSocketPool::Flush() { - base_.Flush(); +void TransportClientSocketPool::FlushWithError(int error) { + base_.FlushWithError(error); } bool TransportClientSocketPool::IsStalled() const { diff --git a/net/socket/transport_client_socket_pool.h b/net/socket/transport_client_socket_pool.h index 7764b88..af14152 100644 --- a/net/socket/transport_client_socket_pool.h +++ b/net/socket/transport_client_socket_pool.h @@ -164,7 +164,7 @@ class NET_EXPORT_PRIVATE TransportClientSocketPool : public ClientSocketPool { virtual void ReleaseSocket(const std::string& group_name, StreamSocket* socket, int id) OVERRIDE; - virtual void Flush() OVERRIDE; + virtual void FlushWithError(int error) OVERRIDE; virtual bool IsStalled() const OVERRIDE; virtual void CloseIdleSockets() OVERRIDE; virtual int IdleSocketCount() const OVERRIDE; diff --git a/net/socket/transport_client_socket_pool_unittest.cc b/net/socket/transport_client_socket_pool_unittest.cc index efeaa6e..aefe79c 100644 --- a/net/socket/transport_client_socket_pool_unittest.cc +++ b/net/socket/transport_client_socket_pool_unittest.cc @@ -983,7 +983,7 @@ TEST_F(TransportClientSocketPoolTest, BackupSocketConnect) { handle.Reset(); // Close all pending connect jobs and existing sockets. - pool_.Flush(); + pool_.FlushWithError(ERR_NETWORK_CHANGED); } } diff --git a/net/spdy/spdy_network_transaction_spdy2_unittest.cc b/net/spdy/spdy_network_transaction_spdy2_unittest.cc index 9df36f0..07708dc 100644 --- a/net/spdy/spdy_network_transaction_spdy2_unittest.cc +++ b/net/spdy/spdy_network_transaction_spdy2_unittest.cc @@ -170,7 +170,7 @@ class SpdyNetworkTransactionSpdy2Test output_.rv = callback.WaitForResult(); if (output_.rv != OK) { - session_->spdy_session_pool()->CloseCurrentSessions(); + session_->spdy_session_pool()->CloseCurrentSessions(net::ERR_ABORTED); return; } diff --git a/net/spdy/spdy_network_transaction_spdy3_unittest.cc b/net/spdy/spdy_network_transaction_spdy3_unittest.cc index 0b6a794..197ba7d 100644 --- a/net/spdy/spdy_network_transaction_spdy3_unittest.cc +++ b/net/spdy/spdy_network_transaction_spdy3_unittest.cc @@ -171,7 +171,7 @@ class SpdyNetworkTransactionSpdy3Test output_.rv = callback.WaitForResult(); if (output_.rv != OK) { - session_->spdy_session_pool()->CloseCurrentSessions(); + session_->spdy_session_pool()->CloseCurrentSessions(net::ERR_ABORTED); return; } diff --git a/net/spdy/spdy_session_pool.cc b/net/spdy/spdy_session_pool.cc index a9279d2..b967a46 100644 --- a/net/spdy/spdy_session_pool.cc +++ b/net/spdy/spdy_session_pool.cc @@ -275,12 +275,12 @@ Value* SpdySessionPool::SpdySessionPoolInfoToValue() const { } void SpdySessionPool::OnIPAddressChanged() { - CloseCurrentSessions(); + CloseCurrentSessions(ERR_NETWORK_CHANGED); http_server_properties_->ClearSpdySettings(); } void SpdySessionPool::OnSSLConfigChanged() { - CloseCurrentSessions(); + CloseCurrentSessions(ERR_NETWORK_CHANGED); } scoped_refptr<SpdySession> SpdySessionPool::GetExistingSession( @@ -347,7 +347,7 @@ scoped_refptr<SpdySession> SpdySessionPool::GetFromAlias( } void SpdySessionPool::OnCertAdded(const X509Certificate* cert) { - CloseCurrentSessions(); + CloseCurrentSessions(ERR_NETWORK_CHANGED); } void SpdySessionPool::OnCertTrustChanged(const X509Certificate* cert) { @@ -355,7 +355,7 @@ void SpdySessionPool::OnCertTrustChanged(const X509Certificate* cert) { // reduced. CloseCurrentSessions now because OnCertTrustChanged does not // tell us this. // See comments in ClientSocketPoolManager::OnCertTrustChanged. - CloseCurrentSessions(); + CloseCurrentSessions(ERR_NETWORK_CHANGED); } const HostPortProxyPair& SpdySessionPool::NormalizeListPair( @@ -447,7 +447,7 @@ void SpdySessionPool::CloseAllSessions() { } } -void SpdySessionPool::CloseCurrentSessions() { +void SpdySessionPool::CloseCurrentSessions(net::Error error) { SpdySessionsMap old_map; old_map.swap(sessions_); for (SpdySessionsMap::const_iterator it = old_map.begin(); @@ -464,8 +464,7 @@ void SpdySessionPool::CloseCurrentSessions() { CHECK(list); const scoped_refptr<SpdySession>& session = list->front(); CHECK(session); - session->CloseSessionOnError( - net::ERR_ABORTED, false, "Closing current sessions."); + session->CloseSessionOnError(error, false, "Closing current sessions."); list->pop_front(); if (list->empty()) { delete list; diff --git a/net/spdy/spdy_session_pool.h b/net/spdy/spdy_session_pool.h index b77f28b..0467583 100644 --- a/net/spdy/spdy_session_pool.h +++ b/net/spdy/spdy_session_pool.h @@ -104,9 +104,9 @@ class NET_EXPORT SpdySessionPool // Close all SpdySessions, including any new ones created in the process of // closing the current ones. void CloseAllSessions(); - // Close only the currently existing SpdySessions. Let any new ones created - // continue to live. - void CloseCurrentSessions(); + // Close only the currently existing SpdySessions with |error|. + // Let any new ones created continue to live. + void CloseCurrentSessions(net::Error error); // Close only the idle SpdySessions. void CloseIdleSessions(); diff --git a/net/spdy/spdy_session_spdy2_unittest.cc b/net/spdy/spdy_session_spdy2_unittest.cc index 17c9c53..f75a1f6 100644 --- a/net/spdy/spdy_session_spdy2_unittest.cc +++ b/net/spdy/spdy_session_spdy2_unittest.cc @@ -1090,7 +1090,7 @@ void IPPoolingTest(bool clean_via_close_current_sessions) { spdy_session_pool->Remove(session2); session2 = NULL; } else { - spdy_session_pool->CloseCurrentSessions(); + spdy_session_pool->CloseCurrentSessions(net::ERR_ABORTED); } // Verify that the map is all cleaned up. diff --git a/net/spdy/spdy_session_spdy3_unittest.cc b/net/spdy/spdy_session_spdy3_unittest.cc index 21fd700..82b4287 100644 --- a/net/spdy/spdy_session_spdy3_unittest.cc +++ b/net/spdy/spdy_session_spdy3_unittest.cc @@ -1102,7 +1102,7 @@ void IPPoolingTest(bool clean_via_close_current_sessions) { spdy_session_pool->Remove(session2); session2 = NULL; } else { - spdy_session_pool->CloseCurrentSessions(); + spdy_session_pool->CloseCurrentSessions(net::ERR_ABORTED); } // Verify that the map is all cleaned up. diff --git a/net/url_request/test_url_fetcher_factory.cc b/net/url_request/test_url_fetcher_factory.cc index 967b810..08ba483 100644 --- a/net/url_request/test_url_fetcher_factory.cc +++ b/net/url_request/test_url_fetcher_factory.cc @@ -114,11 +114,11 @@ void TestURLFetcher::SetStopOnRedirect(bool stop_on_redirect) { void TestURLFetcher::SetAutomaticallyRetryOn5xx(bool retry) { } -void TestURLFetcher::SetMaxRetries(int max_retries) { +void TestURLFetcher::SetMaxRetriesOn5xx(int max_retries) { fake_max_retries_ = max_retries; } -int TestURLFetcher::GetMaxRetries() const { +int TestURLFetcher::GetMaxRetriesOn5xx() const { return fake_max_retries_; } @@ -126,6 +126,9 @@ base::TimeDelta TestURLFetcher::GetBackoffDelay() const { return fake_backoff_delay_; } +void TestURLFetcher::SetAutomaticallyRetryOnNetworkChanges(int max_retries) { +} + void TestURLFetcher::SaveResponseToFileAtPath( const FilePath& file_path, scoped_refptr<base::TaskRunner> file_task_runner) { diff --git a/net/url_request/test_url_fetcher_factory.h b/net/url_request/test_url_fetcher_factory.h index b1e9e78..27f2f7a 100644 --- a/net/url_request/test_url_fetcher_factory.h +++ b/net/url_request/test_url_fetcher_factory.h @@ -106,9 +106,10 @@ class TestURLFetcher : public URLFetcher { const CreateDataCallback& create_data_callback) OVERRIDE; virtual void SetStopOnRedirect(bool stop_on_redirect) OVERRIDE; virtual void SetAutomaticallyRetryOn5xx(bool retry) OVERRIDE; - virtual void SetMaxRetries(int max_retries) OVERRIDE; - virtual int GetMaxRetries() const OVERRIDE; + virtual void SetMaxRetriesOn5xx(int max_retries) OVERRIDE; + virtual int GetMaxRetriesOn5xx() const OVERRIDE; virtual base::TimeDelta GetBackoffDelay() const OVERRIDE; + virtual void SetAutomaticallyRetryOnNetworkChanges(int max_retries) OVERRIDE; virtual void SaveResponseToFileAtPath( const FilePath& file_path, scoped_refptr<base::TaskRunner> file_task_runner) OVERRIDE; diff --git a/net/url_request/url_fetcher.h b/net/url_request/url_fetcher.h index 190e539..37aba02 100644 --- a/net/url_request/url_fetcher.h +++ b/net/url_request/url_fetcher.h @@ -186,13 +186,21 @@ class NET_EXPORT URLFetcher { // after backoff_delay() elapses. URLFetcher has it set to true by default. virtual void SetAutomaticallyRetryOn5xx(bool retry) = 0; - virtual void SetMaxRetries(int max_retries) = 0; - virtual int GetMaxRetries() const = 0; + virtual void SetMaxRetriesOn5xx(int max_retries) = 0; + virtual int GetMaxRetriesOn5xx() const = 0; // Returns the back-off delay before the request will be retried, // when a 5xx response was received. virtual base::TimeDelta GetBackoffDelay() const = 0; + // Retries up to |max_retries| times when requests fail with + // ERR_NETWORK_CHANGED. If ERR_NETWORK_CHANGED is received after having + // retried |max_retries| times then it is propagated to the observer. + // Each retry can be delayed if the network if currently offline, and will be + // attempted once the network connection is back. The first fetch is also + // delayed if the network is offline when Start() is invoked. + virtual void SetAutomaticallyRetryOnNetworkChanges(int max_retries) = 0; + // By default, the response is saved in a string. Call this method to save the // response to a file instead. Must be called before Start(). // |file_task_runner| will be used for all file operations. diff --git a/net/url_request/url_fetcher_core.cc b/net/url_request/url_fetcher_core.cc index 00aa56f..b0a6c65 100644 --- a/net/url_request/url_fetcher_core.cc +++ b/net/url_request/url_fetcher_core.cc @@ -280,13 +280,15 @@ URLFetcherCore::URLFetcherCore(URLFetcher* fetcher, url_request_data_key_(NULL), was_fetched_via_proxy_(false), is_chunked_upload_(false), - num_retries_(0), was_cancelled_(false), response_destination_(STRING), stop_on_redirect_(false), stopped_on_redirect_(false), automatically_retry_on_5xx_(true), - max_retries_(0), + num_retries_on_5xx_(0), + max_retries_on_5xx_(0), + num_retries_on_network_changes_(0), + max_retries_on_network_changes_(0), current_upload_bytes_(-1), current_response_bytes_(0), total_response_bytes_(-1) { @@ -304,8 +306,17 @@ void URLFetcherCore::Start() { } DCHECK(network_task_runner_.get()) << "We need an IO task runner"; - network_task_runner_->PostTask( - FROM_HERE, base::Bind(&URLFetcherCore::StartOnIOThread, this)); + if (num_retries_on_network_changes_ < max_retries_on_network_changes_ && + NetworkChangeNotifier::IsOffline()) { + // We're currently offline and this request will immediately fail. Try to + // start later if |max_retries_on_network_changes_| is set, indicating that + // our owner wants the fetcher to automatically retry on network changes. + ++num_retries_on_network_changes_; + NetworkChangeNotifier::AddConnectionTypeObserver(this); + } else { + network_task_runner_->PostTask( + FROM_HERE, base::Bind(&URLFetcherCore::StartOnIOThread, this)); + } } void URLFetcherCore::Stop() { @@ -407,18 +418,22 @@ void URLFetcherCore::SetAutomaticallyRetryOn5xx(bool retry) { automatically_retry_on_5xx_ = retry; } -void URLFetcherCore::SetMaxRetries(int max_retries) { - max_retries_ = max_retries; +void URLFetcherCore::SetMaxRetriesOn5xx(int max_retries) { + max_retries_on_5xx_ = max_retries; } -int URLFetcherCore::GetMaxRetries() const { - return max_retries_; +int URLFetcherCore::GetMaxRetriesOn5xx() const { + return max_retries_on_5xx_; } base::TimeDelta URLFetcherCore::GetBackoffDelay() const { return backoff_delay_; } +void URLFetcherCore::SetAutomaticallyRetryOnNetworkChanges(int max_retries) { + max_retries_on_network_changes_ = max_retries; +} + void URLFetcherCore::SaveResponseToFileAtPath( const FilePath& file_path, scoped_refptr<base::TaskRunner> file_task_runner) { @@ -606,6 +621,20 @@ void URLFetcherCore::OnReadCompleted(URLRequest* request, } } +void URLFetcherCore::OnConnectionTypeChanged( + NetworkChangeNotifier::ConnectionType type) { + DCHECK_GT(num_retries_on_network_changes_, 0); + if (type == NetworkChangeNotifier::CONNECTION_NONE) { + // Keep waiting. + return; + } + + // Stop observing and try again now. + NetworkChangeNotifier::RemoveConnectionTypeObserver(this); + network_task_runner_->PostTask( + FROM_HERE, base::Bind(&URLFetcherCore::StartOnIOThread, this)); +} + void URLFetcherCore::CancelAll() { g_registry.Get().CancelAll(); } @@ -838,7 +867,7 @@ void URLFetcherCore::RetryOrCompleteUrlFetch() { status_.error() == ERR_TEMPORARILY_THROTTLED) { // When encountering a server error, we will send the request again // after backoff time. - ++num_retries_; + ++num_retries_on_5xx_; // Note that backoff_delay may be 0 because (a) the // URLRequestThrottlerManager and related code does not @@ -850,13 +879,32 @@ void URLFetcherCore::RetryOrCompleteUrlFetch() { if (backoff_delay < base::TimeDelta()) backoff_delay = base::TimeDelta(); - if (automatically_retry_on_5xx_ && num_retries_ <= max_retries_) { + if (automatically_retry_on_5xx_ && + num_retries_on_5xx_ <= max_retries_on_5xx_) { StartOnIOThread(); return; } } else { backoff_delay = base::TimeDelta(); } + + // Retry if the request failed due to network changes. + if (status_.error() == ERR_NETWORK_CHANGED && + num_retries_on_network_changes_ < max_retries_on_network_changes_) { + ++num_retries_on_network_changes_; + + if (NetworkChangeNotifier::IsOffline()) { + // Retry once we're back online. + NetworkChangeNotifier::AddConnectionTypeObserver(this); + } else { + // Retry soon, after flushing all the current tasks which may include + // further network change observers. + network_task_runner_->PostTask( + FROM_HERE, base::Bind(&URLFetcherCore::StartOnIOThread, this)); + } + return; + } + request_context_getter_ = NULL; first_party_for_cookies_ = GURL(); url_request_data_key_ = NULL; @@ -871,6 +919,8 @@ void URLFetcherCore::RetryOrCompleteUrlFetch() { } void URLFetcherCore::ReleaseRequest() { + if (num_retries_on_network_changes_ > 0) + NetworkChangeNotifier::RemoveConnectionTypeObserver(this); upload_progress_checker_timer_.reset(); request_.reset(); g_registry.Get().RemoveURLFetcherCore(this); diff --git a/net/url_request/url_fetcher_core.h b/net/url_request/url_fetcher_core.h index 4b258e5..67589c4 100644 --- a/net/url_request/url_fetcher_core.h +++ b/net/url_request/url_fetcher_core.h @@ -20,6 +20,7 @@ #include "base/timer.h" #include "googleurl/src/gurl.h" #include "net/base/host_port_pair.h" +#include "net/base/network_change_notifier.h" #include "net/http/http_request_headers.h" #include "net/url_request/url_fetcher.h" #include "net/url_request/url_request.h" @@ -38,7 +39,8 @@ class URLRequestThrottlerEntryInterface; class URLFetcherCore : public base::RefCountedThreadSafe<URLFetcherCore>, - public URLRequest::Delegate { + public URLRequest::Delegate, + public NetworkChangeNotifier::ConnectionTypeObserver { public: URLFetcherCore(URLFetcher* fetcher, const GURL& original_url, @@ -86,9 +88,10 @@ class URLFetcherCore const URLFetcher::CreateDataCallback& create_data_callback); void SetStopOnRedirect(bool stop_on_redirect); void SetAutomaticallyRetryOn5xx(bool retry); - void SetMaxRetries(int max_retries); - int GetMaxRetries() const; + void SetMaxRetriesOn5xx(int max_retries); + int GetMaxRetriesOn5xx() const; base::TimeDelta GetBackoffDelay() const; + void SetAutomaticallyRetryOnNetworkChanges(int max_retries); void SaveResponseToFileAtPath( const FilePath& file_path, scoped_refptr<base::TaskRunner> file_task_runner); @@ -122,6 +125,10 @@ class URLFetcherCore virtual void OnReadCompleted(URLRequest* request, int bytes_read) OVERRIDE; + // Overridden from NetworkChangeNotifier::ConnectionTypeObserver: + virtual void OnConnectionTypeChanged( + NetworkChangeNotifier::ConnectionType type) OVERRIDE; + URLFetcherDelegate* delegate() const { return delegate_; } static void CancelAll(); static int GetNumFetcherCores(); @@ -352,11 +359,6 @@ class URLFetcherCore original_url_throttler_entry_; scoped_refptr<URLRequestThrottlerEntryInterface> url_throttler_entry_; - // |num_retries_| indicates how many times we've failed to successfully - // fetch this URL. Once this value exceeds the maximum number of retries - // specified by the owner URLFetcher instance, we'll give up. - int num_retries_; - // True if the URLFetcher has been cancelled. bool was_cancelled_; @@ -384,11 +386,22 @@ class URLFetcherCore // re-execute the request, after the back-off delay has expired. // true by default. bool automatically_retry_on_5xx_; - // Maximum retries allowed. - int max_retries_; + // |num_retries_on_5xx_| indicates how many times we've failed to successfully + // fetch this URL due to 5xx responses. Once this value exceeds the maximum + // number of retries specified by the owner URLFetcher instance, + // we'll give up. + int num_retries_on_5xx_; + // Maximum retries allowed when 5xx responses are received. + int max_retries_on_5xx_; // Back-off time delay. 0 by default. base::TimeDelta backoff_delay_; + // The number of retries that have been attempted due to ERR_NETWORK_CHANGED. + int num_retries_on_network_changes_; + // Maximum retries allowed when the request fails with ERR_NETWORK_CHANGED. + // 0 by default. + int max_retries_on_network_changes_; + // Timer to poll the progress of uploading for POST and PUT requests. // When crbug.com/119629 is fixed, scoped_ptr is not necessary here. scoped_ptr<base::RepeatingTimer<URLFetcherCore> > diff --git a/net/url_request/url_fetcher_impl.cc b/net/url_request/url_fetcher_impl.cc index 0fe1bfa..cdbd104 100644 --- a/net/url_request/url_fetcher_impl.cc +++ b/net/url_request/url_fetcher_impl.cc @@ -89,12 +89,12 @@ void URLFetcherImpl::SetAutomaticallyRetryOn5xx(bool retry) { core_->SetAutomaticallyRetryOn5xx(retry); } -void URLFetcherImpl::SetMaxRetries(int max_retries) { - core_->SetMaxRetries(max_retries); +void URLFetcherImpl::SetMaxRetriesOn5xx(int max_retries) { + core_->SetMaxRetriesOn5xx(max_retries); } -int URLFetcherImpl::GetMaxRetries() const { - return core_->GetMaxRetries(); +int URLFetcherImpl::GetMaxRetriesOn5xx() const { + return core_->GetMaxRetriesOn5xx(); } @@ -102,6 +102,10 @@ base::TimeDelta URLFetcherImpl::GetBackoffDelay() const { return core_->GetBackoffDelay(); } +void URLFetcherImpl::SetAutomaticallyRetryOnNetworkChanges(int max_retries) { + core_->SetAutomaticallyRetryOnNetworkChanges(max_retries); +} + void URLFetcherImpl::SaveResponseToFileAtPath( const FilePath& file_path, scoped_refptr<base::TaskRunner> file_task_runner) { diff --git a/net/url_request/url_fetcher_impl.h b/net/url_request/url_fetcher_impl.h index eb7e315..9b0c1de 100644 --- a/net/url_request/url_fetcher_impl.h +++ b/net/url_request/url_fetcher_impl.h @@ -58,9 +58,10 @@ class NET_EXPORT_PRIVATE URLFetcherImpl : public URLFetcher { const CreateDataCallback& create_data_callback) OVERRIDE; virtual void SetStopOnRedirect(bool stop_on_redirect) OVERRIDE; virtual void SetAutomaticallyRetryOn5xx(bool retry) OVERRIDE; - virtual void SetMaxRetries(int max_retries) OVERRIDE; - virtual int GetMaxRetries() const OVERRIDE; + virtual void SetMaxRetriesOn5xx(int max_retries) OVERRIDE; + virtual int GetMaxRetriesOn5xx() const OVERRIDE; virtual base::TimeDelta GetBackoffDelay() const OVERRIDE; + virtual void SetAutomaticallyRetryOnNetworkChanges(int max_retries) OVERRIDE; virtual void SaveResponseToFileAtPath( const FilePath& file_path, scoped_refptr<base::TaskRunner> file_task_runner) OVERRIDE; diff --git a/net/url_request/url_fetcher_impl_unittest.cc b/net/url_request/url_fetcher_impl_unittest.cc index 1c6b77b..af6b2ad 100644 --- a/net/url_request/url_fetcher_impl_unittest.cc +++ b/net/url_request/url_fetcher_impl_unittest.cc @@ -10,10 +10,13 @@ #include "base/file_util.h" #include "base/files/scoped_temp_dir.h" #include "base/message_loop_proxy.h" +#include "base/stringprintf.h" #include "base/synchronization/waitable_event.h" #include "base/threading/thread.h" #include "build/build_config.h" #include "crypto/nss_util.h" +#include "net/base/mock_host_resolver.h" +#include "net/base/network_change_notifier.h" #include "net/http/http_response_headers.h" #include "net/test/test_server.h" #include "net/url_request/url_fetcher_delegate.h" @@ -72,6 +75,24 @@ class ThrottlingTestURLRequestContextGetter TestURLRequestContext* const context_; }; +class TestNetworkChangeNotifier : public NetworkChangeNotifier { + public: + TestNetworkChangeNotifier() : connection_type_(CONNECTION_UNKNOWN) {} + + // Implementation of NetworkChangeNotifier: + virtual ConnectionType GetCurrentConnectionType() const OVERRIDE { + return connection_type_; + } + + void SetCurrentConnectionType(ConnectionType type) { + connection_type_ = type; + NotifyObserversOfConnectionTypeChange(); + } + + private: + ConnectionType connection_type_; +}; + } // namespace class URLFetcherTest : public testing::Test, @@ -79,7 +100,7 @@ class URLFetcherTest : public testing::Test, public: URLFetcherTest() : fetcher_(NULL), - context_(new ThrottlingTestURLRequestContext()) { + context_(NULL) { } static int GetNumFetcherCores() { @@ -111,6 +132,7 @@ class URLFetcherTest : public testing::Test, virtual void SetUp() OVERRIDE { testing::Test::SetUp(); + context_.reset(new ThrottlingTestURLRequestContext()); io_message_loop_proxy_ = base::MessageLoopProxy::current(); #if defined(USE_NSS) || defined(OS_IOS) @@ -132,7 +154,29 @@ class URLFetcherTest : public testing::Test, scoped_refptr<base::MessageLoopProxy> io_message_loop_proxy_; URLFetcherImpl* fetcher_; - const scoped_ptr<TestURLRequestContext> context_; + scoped_ptr<TestURLRequestContext> context_; +}; + +// A test fixture that uses a MockHostResolver, so that name resolutions can +// be manipulated by the tests to keep connections in the resolving state. +class URLFetcherMockDnsTest : public URLFetcherTest { + public: + // testing::Test: + virtual void SetUp() OVERRIDE; + + // URLFetcherTest: + virtual void CreateFetcher(const GURL& url) OVERRIDE; + + // URLFetcherDelegate: + virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE; + + protected: + GURL test_url_; + scoped_ptr<TestServer> test_server_; + MockHostResolver resolver_; + scoped_ptr<URLFetcher> completed_fetcher_; + NetworkChangeNotifier::DisableForTest disable_default_notifier_; + TestNetworkChangeNotifier network_change_notifier_; }; void URLFetcherTest::CreateFetcher(const GURL& url) { @@ -163,6 +207,43 @@ void URLFetcherTest::CleanupAfterFetchComplete() { // the main loop returns and this thread subsequently goes out of scope. } +void URLFetcherMockDnsTest::SetUp() { + URLFetcherTest::SetUp(); + + resolver_.set_ondemand_mode(true); + resolver_.rules()->AddRule("example.com", "127.0.0.1"); + + context_.reset(new TestURLRequestContext(true)); + context_->set_host_resolver(&resolver_); + context_->Init(); + + test_server_.reset(new TestServer(TestServer::TYPE_HTTP, + TestServer::kLocalhost, + FilePath(kDocRoot))); + ASSERT_TRUE(test_server_->Start()); + + // test_server_.GetURL() returns a URL with 127.0.0.1 (kLocalhost), that is + // immediately resolved by the MockHostResolver. Use a hostname instead to + // trigger an async resolve. + test_url_ = GURL( + base::StringPrintf("http://example.com:%d/defaultresponse", + test_server_->host_port_pair().port())); + ASSERT_TRUE(test_url_.is_valid()); +} + +void URLFetcherMockDnsTest::CreateFetcher(const GURL& url) { + fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this); + fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter( + io_message_loop_proxy(), request_context())); +} + +void URLFetcherMockDnsTest::OnURLFetchComplete(const URLFetcher* source) { + io_message_loop_proxy()->PostTask(FROM_HERE, MessageLoop::QuitClosure()); + ASSERT_EQ(fetcher_, source); + EXPECT_EQ(test_url_, source->GetOriginalURL()); + completed_fetcher_.reset(fetcher_); +} + namespace { // Version of URLFetcherTest that does a POST instead @@ -584,7 +665,7 @@ void URLFetcherProtectTest::CreateFetcher(const GURL& url) { fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter( io_message_loop_proxy(), request_context())); start_time_ = Time::Now(); - fetcher_->SetMaxRetries(11); + fetcher_->SetMaxRetriesOn5xx(11); fetcher_->Start(); } @@ -623,7 +704,7 @@ void URLFetcherProtectTestPassedThrough::CreateFetcher(const GURL& url) { io_message_loop_proxy(), request_context())); fetcher_->SetAutomaticallyRetryOn5xx(false); start_time_ = Time::Now(); - fetcher_->SetMaxRetries(11); + fetcher_->SetMaxRetriesOn5xx(11); fetcher_->Start(); } @@ -682,7 +763,7 @@ void URLFetcherCancelTest::CreateFetcher(const GURL& url) { new CancelTestURLRequestContextGetter(io_message_loop_proxy(), url); fetcher_->SetRequestContext(context_getter); - fetcher_->SetMaxRetries(2); + fetcher_->SetMaxRetriesOn5xx(2); fetcher_->Start(); // We need to wait for the creation of the URLRequestContext, since we // rely on it being destroyed as a signal to end the test. @@ -828,6 +909,205 @@ TEST_F(URLFetcherTest, CancelAll) { delete fetcher_; } +TEST_F(URLFetcherMockDnsTest, DontRetryOnNetworkChangedByDefault) { + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + + // This posts a task to start the fetcher. + CreateFetcher(test_url_); + fetcher_->Start(); + EXPECT_EQ(0, GetNumFetcherCores()); + MessageLoop::current()->RunUntilIdle(); + + // The fetcher is now running, but is pending the host resolve. + EXPECT_EQ(1, GetNumFetcherCores()); + EXPECT_TRUE(resolver_.has_pending_requests()); + ASSERT_FALSE(completed_fetcher_); + + // A network change notification aborts the connect job. + NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests(); + MessageLoop::current()->RunUntilIdle(); + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + ASSERT_TRUE(completed_fetcher_); + + // And the owner of the fetcher gets the ERR_NETWORK_CHANGED error. + EXPECT_EQ(ERR_NETWORK_CHANGED, completed_fetcher_->GetStatus().error()); +} + +TEST_F(URLFetcherMockDnsTest, RetryOnNetworkChangedAndFail) { + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + + // This posts a task to start the fetcher. + CreateFetcher(test_url_); + fetcher_->SetAutomaticallyRetryOnNetworkChanges(3); + fetcher_->Start(); + EXPECT_EQ(0, GetNumFetcherCores()); + MessageLoop::current()->RunUntilIdle(); + + // The fetcher is now running, but is pending the host resolve. + EXPECT_EQ(1, GetNumFetcherCores()); + EXPECT_TRUE(resolver_.has_pending_requests()); + ASSERT_FALSE(completed_fetcher_); + + // Make it fail 3 times. + for (int i = 0; i < 3; ++i) { + // A network change notification aborts the connect job. + NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests(); + MessageLoop::current()->RunUntilIdle(); + + // But the fetcher retries automatically. + EXPECT_EQ(1, GetNumFetcherCores()); + EXPECT_TRUE(resolver_.has_pending_requests()); + ASSERT_FALSE(completed_fetcher_); + } + + // A 4th failure doesn't trigger another retry, and propagates the error + // to the owner of the fetcher. + NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests(); + MessageLoop::current()->RunUntilIdle(); + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + ASSERT_TRUE(completed_fetcher_); + + // And the owner of the fetcher gets the ERR_NETWORK_CHANGED error. + EXPECT_EQ(ERR_NETWORK_CHANGED, completed_fetcher_->GetStatus().error()); +} + +TEST_F(URLFetcherMockDnsTest, RetryOnNetworkChangedAndSucceed) { + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + + // This posts a task to start the fetcher. + CreateFetcher(test_url_); + fetcher_->SetAutomaticallyRetryOnNetworkChanges(3); + fetcher_->Start(); + EXPECT_EQ(0, GetNumFetcherCores()); + MessageLoop::current()->RunUntilIdle(); + + // The fetcher is now running, but is pending the host resolve. + EXPECT_EQ(1, GetNumFetcherCores()); + EXPECT_TRUE(resolver_.has_pending_requests()); + ASSERT_FALSE(completed_fetcher_); + + // Make it fail 3 times. + for (int i = 0; i < 3; ++i) { + // A network change notification aborts the connect job. + NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests(); + MessageLoop::current()->RunUntilIdle(); + + // But the fetcher retries automatically. + EXPECT_EQ(1, GetNumFetcherCores()); + EXPECT_TRUE(resolver_.has_pending_requests()); + ASSERT_FALSE(completed_fetcher_); + } + + // Now let it succeed by resolving the pending request. + resolver_.ResolveAllPending(); + MessageLoop::current()->Run(); + + // URLFetcherMockDnsTest::OnURLFetchComplete() will quit the loop. + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + ASSERT_TRUE(completed_fetcher_); + + // This time the request succeeded. + EXPECT_EQ(OK, completed_fetcher_->GetStatus().error()); + EXPECT_EQ(200, completed_fetcher_->GetResponseCode()); +} + +TEST_F(URLFetcherMockDnsTest, RetryAfterComingBackOnline) { + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + + // This posts a task to start the fetcher. + CreateFetcher(test_url_); + fetcher_->SetAutomaticallyRetryOnNetworkChanges(1); + fetcher_->Start(); + EXPECT_EQ(0, GetNumFetcherCores()); + MessageLoop::current()->RunUntilIdle(); + + // The fetcher is now running, but is pending the host resolve. + EXPECT_EQ(1, GetNumFetcherCores()); + EXPECT_TRUE(resolver_.has_pending_requests()); + ASSERT_FALSE(completed_fetcher_); + + // Make it fail by changing the connection type to offline. + EXPECT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN, + NetworkChangeNotifier::GetConnectionType()); + EXPECT_FALSE(NetworkChangeNotifier::IsOffline()); + network_change_notifier_.SetCurrentConnectionType( + NetworkChangeNotifier::CONNECTION_NONE); + // This makes the connect job fail: + NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests(); + resolver_.ResolveAllPending(); + MessageLoop::current()->RunUntilIdle(); + + // The fetcher is now waiting for the connection to become online again. + EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE, + NetworkChangeNotifier::GetConnectionType()); + EXPECT_TRUE(NetworkChangeNotifier::IsOffline()); + EXPECT_FALSE(resolver_.has_pending_requests()); + ASSERT_FALSE(completed_fetcher_); + // The core is still alive, but it dropped its request. + EXPECT_EQ(0, GetNumFetcherCores()); + + // It should retry once the connection is back. + network_change_notifier_.SetCurrentConnectionType( + NetworkChangeNotifier::CONNECTION_WIFI); + MessageLoop::current()->RunUntilIdle(); + EXPECT_EQ(1, GetNumFetcherCores()); + ASSERT_FALSE(completed_fetcher_); + EXPECT_TRUE(resolver_.has_pending_requests()); + + // Resolve the pending request; the fetcher should complete now. + resolver_.ResolveAllPending(); + MessageLoop::current()->Run(); + + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + ASSERT_TRUE(completed_fetcher_); + EXPECT_EQ(OK, completed_fetcher_->GetStatus().error()); + EXPECT_EQ(200, completed_fetcher_->GetResponseCode()); +} + +TEST_F(URLFetcherMockDnsTest, StartOnlyWhenOnline) { + // Start offline. + network_change_notifier_.SetCurrentConnectionType( + NetworkChangeNotifier::CONNECTION_NONE); + EXPECT_TRUE(NetworkChangeNotifier::IsOffline()); + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + + // Create a fetcher that retries on network changes. It will try to connect + // only once the network is back online. + CreateFetcher(test_url_); + fetcher_->SetAutomaticallyRetryOnNetworkChanges(1); + fetcher_->Start(); + MessageLoop::current()->RunUntilIdle(); + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + + // It should retry once the connection is back. + network_change_notifier_.SetCurrentConnectionType( + NetworkChangeNotifier::CONNECTION_WIFI); + MessageLoop::current()->RunUntilIdle(); + EXPECT_EQ(1, GetNumFetcherCores()); + ASSERT_FALSE(completed_fetcher_); + EXPECT_TRUE(resolver_.has_pending_requests()); + + // Resolve the pending request; the fetcher should complete now. + resolver_.ResolveAllPending(); + MessageLoop::current()->Run(); + + EXPECT_EQ(0, GetNumFetcherCores()); + EXPECT_FALSE(resolver_.has_pending_requests()); + ASSERT_TRUE(completed_fetcher_); + EXPECT_EQ(OK, completed_fetcher_->GetStatus().error()); + EXPECT_EQ(200, completed_fetcher_->GetResponseCode()); +} + #if defined(OS_MACOSX) // SIGSEGV on Mac: http://crbug.com/60426 TEST_F(URLFetcherPostTest, DISABLED_Basic) { |