Timeout persistent idle connections after 5 minutes, and increase the socket cleanup timer from 5 seconds to 10 seconds.

http://code.google.com/p/chromium/issues/detail?id=3574
Review URL: http://codereview.chromium.org/8124

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@4112 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 53 insertions, 30 deletions

diff --git a/net/base/client_socket_pool.cc b/net/base/client_socket_pool.cc
index 46afef6..b8434dd 100644
--- a/net/base/client_socket_pool.cc
+++ b/net/base/client_socket_pool.cc
@@ -14,7 +14,10 @@ using base::TimeDelta;
 namespace {
 
 // The timeout value, in seconds, used to clean up disconnected idle sockets.
-const int kCleanupInterval = 5;
+const int kCleanupInterval = 10;
+
+// The maximum duration, in seconds, to keep idle persistent sockets alive.
+const int kIdleTimeout = 300; // 5 minutes.
 
 }  // namespace
 
@@ -26,9 +29,9 @@ ClientSocketPool::ClientSocketPool(int max_sockets_per_group)
 }
 
 ClientSocketPool::~ClientSocketPool() {
-  // Clean up any idle sockets.  Assert that we have no remaining active sockets
-  // or pending requests.  They should have all been cleaned up prior to the
-  // manager being destroyed.
+  // Clean up any idle sockets.  Assert that we have no remaining active
+  // sockets or pending requests.  They should have all been cleaned up prior
+  // to the manager being destroyed.
   CloseIdleSockets();
   DCHECK(group_map_.empty());
 }
@@ -53,15 +56,15 @@ int ClientSocketPool::RequestSocket(ClientSocketHandle* handle,
   // Use idle sockets in LIFO order because they're more likely to be
   // still connected.
   while (!group.idle_sockets.empty()) {
-    ClientSocketPtr* ptr = group.idle_sockets.back();
+    IdleSocket idle_socket = group.idle_sockets.back();
     group.idle_sockets.pop_back();
     DecrementIdleCount();
-    if ((*ptr)->IsConnected()) {
+    if ((*idle_socket.ptr)->IsConnected()) {
       // We found one we can reuse!
-      handle->socket_ = ptr;
+      handle->socket_ = idle_socket.ptr;
       return OK;
     }
-    delete ptr;
+    delete idle_socket.ptr;
   }
 
   handle->socket_ = new ClientSocketPtr();
@@ -96,22 +99,31 @@ void ClientSocketPool::ReleaseSocket(ClientSocketHandle* handle) {
 }
 
 void ClientSocketPool::CloseIdleSockets() {
-  MaybeCloseIdleSockets(false);
+  CleanupIdleSockets(true);
+}
+
+bool ClientSocketPool::IdleSocket::ShouldCleanup(base::TimeTicks now) const {
+  bool timed_out = (now - start_time) >= 
+      base::TimeDelta::FromSeconds(kIdleTimeout);
+  return timed_out || !(*ptr)->IsConnected();
 }
 
-void ClientSocketPool::MaybeCloseIdleSockets(
-    bool only_if_disconnected) {
+void ClientSocketPool::CleanupIdleSockets(bool force) {
   if (idle_socket_count_ == 0)
     return;
 
+  // Current time value. Retrieving it once at the function start rather than
+  // inside the inner loop, since it shouldn't change by any meaningful amount.
+  base::TimeTicks now = base::TimeTicks::Now();
+
   GroupMap::iterator i = group_map_.begin();
   while (i != group_map_.end()) {
     Group& group = i->second;
 
-    std::deque<ClientSocketPtr*>::iterator j = group.idle_sockets.begin();
+    std::deque<IdleSocket>::iterator j = group.idle_sockets.begin();
     while (j != group.idle_sockets.end()) {
-      if (!only_if_disconnected || !(*j)->get()->IsConnected()) {
-        delete *j;
+      if (force || j->ShouldCleanup(now)) {
+        delete j->ptr;
         j = group.idle_sockets.erase(j);
         DecrementIdleCount();
       } else {
@@ -132,7 +144,7 @@ void ClientSocketPool::MaybeCloseIdleSockets(
 void ClientSocketPool::IncrementIdleCount() {
   if (++idle_socket_count_ == 1)
     timer_.Start(TimeDelta::FromSeconds(kCleanupInterval), this,
-                 &ClientSocketPool::DoTimeout);
+                 &ClientSocketPool::OnCleanupTimerFired);
 }
 
 void ClientSocketPool::DecrementIdleCount() {
@@ -152,7 +164,11 @@ void ClientSocketPool::DoReleaseSocket(const std::string& group_name,
 
   bool can_reuse = ptr->get() && (*ptr)->IsConnected();
   if (can_reuse) {
-    group.idle_sockets.push_back(ptr);
+    IdleSocket idle_socket;
+    idle_socket.ptr = ptr;
+    idle_socket.start_time = base::TimeTicks::Now();
+
+    group.idle_sockets.push_back(idle_socket);
     IncrementIdleCount();
   } else {
     delete ptr;
@@ -175,9 +191,5 @@ void ClientSocketPool::DoReleaseSocket(const std::string& group_name,
   }
 }
 
-void ClientSocketPool::DoTimeout() {
-  MaybeCloseIdleSockets(true);
-}
-
 }  // namespace net
 
diff --git a/net/base/client_socket_pool.h b/net/base/client_socket_pool.h
index 4b89705..76a3dff 100644
--- a/net/base/client_socket_pool.h
+++ b/net/base/client_socket_pool.h
@@ -71,11 +71,9 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
 
   ~ClientSocketPool();
 
-  // Closes all idle sockets if |only_if_disconnected| is false.  Else, only
-  // idle sockets that are disconnected get closed.  "Maybe" refers to the
-  // fact that it doesn't close all idle sockets if |only_if_disconnected| is
-  // true.
-  void MaybeCloseIdleSockets(bool only_if_disconnected);
+  // Closes all idle sockets if |force| is true.  Else, only closes idle
+  // sockets that are disconnected or timed out.
+  void CleanupIdleSockets(bool force);
 
   // Called when the number of idle sockets changes.
   void IncrementIdleCount();
@@ -84,9 +82,11 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
   // Called via PostTask by ReleaseSocket.
   void DoReleaseSocket(const std::string& group_name, ClientSocketPtr* ptr);
 
-  // Called when timer_ fires.  This method scans the idle sockets checking to
-  // see if any have been disconnected.
-  void DoTimeout();
+  // Called when timer_ fires.  This method scans the idle sockets removing
+  // sockets that are disconnected or timed out.
+  void OnCleanupTimerFired() {
+    CleanupIdleSockets(false);
+  }
 
   // A Request is allocated per call to RequestSocket that results in
   // ERR_IO_PENDING.
@@ -95,11 +95,21 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
     CompletionCallback* callback;
   };
 
+  // Entry for a persistent socket which became idle at time |start_time|.
+  struct IdleSocket {
+    ClientSocketPtr* ptr;
+    base::TimeTicks start_time;
+
+    // An idle socket should be removed if it is disconnected, or has been idle
+    // for too long. |now| is the current time value (TimeTicks::Now()).
+    bool ShouldCleanup(base::TimeTicks now) const;
+  };
+
   // A Group is allocated per group_name when there are idle sockets or pending
   // requests.  Otherwise, the Group object is removed from the map.
   struct Group {
     Group() : active_socket_count(0) {}
-    std::deque<ClientSocketPtr*> idle_sockets;
+    std::deque<IdleSocket> idle_sockets;
     std::deque<Request> pending_requests;
     int active_socket_count;
   };
@@ -107,7 +117,8 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
   typedef std::map<std::string, Group> GroupMap;
   GroupMap group_map_;
 
-  // Timer used to periodically prune sockets that have been disconnected.
+  // Timer used to periodically prune idle sockets that are disconnected or
+  // timed out.
   base::RepeatingTimer<ClientSocketPool> timer_;
 
   // The total number of idle sockets in the system.