If an idle socket has received data unexpectedly, we can't

reuse it.

Add the IsConnectedAndIdle method, which returns true if the
connection is still alive and idle (hasn't received any data
unexpectedly).

R=eroman
BUG=4606
Review URL: http://codereview.chromium.org/21501

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

16 files changed, 118 insertions, 14 deletions

diff --git a/net/base/client_socket.h b/net/base/client_socket.h
index b96f07b..f61c7f5 100644
--- a/net/base/client_socket.h
+++ b/net/base/client_socket.h
@@ -47,6 +47,11 @@ class ClientSocket : public Socket {
   // connection wasn't established or the connection is dead.
   virtual bool IsConnected() const = 0;
 
+  // Called to test if the connection is still alive and idle.  Returns false
+  // if a connection wasn't established, the connection is dead, or some data
+  // have been received.
+  virtual bool IsConnectedAndIdle() const = 0;
+
 #if defined(OS_LINUX)
   // Identical to posix system call getpeername().
   // Needed by ssl_client_socket_nss.
diff --git a/net/base/client_socket_pool.cc b/net/base/client_socket_pool.cc
index b8434dd..41c1b7f 100644
--- a/net/base/client_socket_pool.cc
+++ b/net/base/client_socket_pool.cc
@@ -13,11 +13,12 @@ using base::TimeDelta;
 
 namespace {
 
-// The timeout value, in seconds, used to clean up disconnected idle sockets.
+// The timeout value, in seconds, used to clean up idle sockets that can't be
+// reused.
 const int kCleanupInterval = 10;
 
 // The maximum duration, in seconds, to keep idle persistent sockets alive.
-const int kIdleTimeout = 300; // 5 minutes.
+const int kIdleTimeout = 300;  // 5 minutes.
 
 }  // namespace
 
@@ -54,12 +55,12 @@ int ClientSocketPool::RequestSocket(ClientSocketHandle* handle,
   group.active_socket_count++;
 
   // Use idle sockets in LIFO order because they're more likely to be
-  // still connected.
+  // still reusable.
   while (!group.idle_sockets.empty()) {
     IdleSocket idle_socket = group.idle_sockets.back();
     group.idle_sockets.pop_back();
     DecrementIdleCount();
-    if ((*idle_socket.ptr)->IsConnected()) {
+    if ((*idle_socket.ptr)->IsConnectedAndIdle()) {
       // We found one we can reuse!
       handle->socket_ = idle_socket.ptr;
       return OK;
@@ -103,9 +104,9 @@ void ClientSocketPool::CloseIdleSockets() {
 }
 
 bool ClientSocketPool::IdleSocket::ShouldCleanup(base::TimeTicks now) const {
-  bool timed_out = (now - start_time) >= 
+  bool timed_out = (now - start_time) >=
       base::TimeDelta::FromSeconds(kIdleTimeout);
-  return timed_out || !(*ptr)->IsConnected();
+  return timed_out || !(*ptr)->IsConnectedAndIdle();
 }
 
 void ClientSocketPool::CleanupIdleSockets(bool force) {
@@ -162,7 +163,7 @@ void ClientSocketPool::DoReleaseSocket(const std::string& group_name,
   DCHECK(group.active_socket_count > 0);
   group.active_socket_count--;
 
-  bool can_reuse = ptr->get() && (*ptr)->IsConnected();
+  bool can_reuse = ptr->get() && (*ptr)->IsConnectedAndIdle();
   if (can_reuse) {
     IdleSocket idle_socket;
     idle_socket.ptr = ptr;
diff --git a/net/base/client_socket_pool.h b/net/base/client_socket_pool.h
index 764f859..cb43637 100644
--- a/net/base/client_socket_pool.h
+++ b/net/base/client_socket_pool.h
@@ -77,7 +77,7 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
   ~ClientSocketPool();
 
   // Closes all idle sockets if |force| is true.  Else, only closes idle
-  // sockets that are disconnected or timed out.
+  // sockets that timed out or can't be reused.
   void CleanupIdleSockets(bool force);
 
   // Called when the number of idle sockets changes.
@@ -88,7 +88,7 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
   void DoReleaseSocket(const std::string& group_name, ClientSocketPtr* ptr);
 
   // Called when timer_ fires.  This method scans the idle sockets removing
-  // sockets that are disconnected or timed out.
+  // sockets that timed out or can't be reused.
   void OnCleanupTimerFired() {
     CleanupIdleSockets(false);
   }
@@ -105,8 +105,13 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
     ClientSocketPtr* ptr;
     base::TimeTicks start_time;
 
-    // An idle socket should be removed if it is disconnected, or has been idle
+    // An idle socket should be removed if it can't be reused, or has been idle
     // for too long. |now| is the current time value (TimeTicks::Now()).
+    //
+    // An idle socket can't be reused if it is disconnected or has received
+    // data unexpectedly (hence no longer idle).  The unread data would be
+    // mistaken for the beginning of the next response if we were to reuse the
+    // socket for a new request.
     bool ShouldCleanup(base::TimeTicks now) const;
   };
 
@@ -122,8 +127,8 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
   typedef std::map<std::string, Group> GroupMap;
   GroupMap group_map_;
 
-  // Timer used to periodically prune idle sockets that are disconnected or
-  // timed out.
+  // Timer used to periodically prune idle sockets that timed out or can't be
+  // reused.
   base::RepeatingTimer<ClientSocketPool> timer_;
 
   // The total number of idle sockets in the system.
@@ -131,7 +136,7 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
 
   // The maximum number of sockets kept per group.
   int max_sockets_per_group_;
-  
+
   DISALLOW_COPY_AND_ASSIGN(ClientSocketPool);
 };
 
diff --git a/net/base/client_socket_pool_unittest.cc b/net/base/client_socket_pool_unittest.cc
index affae15..5b0836b 100644
--- a/net/base/client_socket_pool_unittest.cc
+++ b/net/base/client_socket_pool_unittest.cc
@@ -35,6 +35,9 @@ class MockClientSocket : public net::ClientSocket {
   virtual bool IsConnected() const {
     return connected_;
   }
+  virtual bool IsConnectedAndIdle() const {
+    return connected_;
+  }
 
   // Socket methods:
   virtual int Read(char* buf, int buf_len,
@@ -56,7 +59,7 @@ int MockClientSocket::allocation_count = 0;
 
 class TestSocketRequest : public CallbackRunner< Tuple1<int> > {
  public:
-  TestSocketRequest(net::ClientSocketPool* pool) : handle(pool) {}
+  explicit TestSocketRequest(net::ClientSocketPool* pool) : handle(pool) {}
 
   net::ClientSocketHandle handle;
 
diff --git a/net/base/ssl_client_socket_mac.cc b/net/base/ssl_client_socket_mac.cc
index a84929b..d351db5 100644
--- a/net/base/ssl_client_socket_mac.cc
+++ b/net/base/ssl_client_socket_mac.cc
@@ -307,6 +307,17 @@ bool SSLClientSocketMac::IsConnected() const {
   return completed_handshake_ && transport_->IsConnected();
 }
 
+bool SSLClientSocketMac::IsConnectedAndIdle() const {
+  // Unlike IsConnected, this method doesn't return a false positive.
+  //
+  // Strictly speaking, we should check if we have received the close_notify
+  // alert message from the server, and return false in that case.  Although
+  // the close_notify alert message means EOF in the SSL layer, it is just
+  // bytes to the transport layer below, so transport_->IsConnectedAndIdle()
+  // returns the desired false when we receive close_notify.
+  return completed_handshake_ && transport_->IsConnectedAndIdle();
+}
+
 int SSLClientSocketMac::Read(char* buf, int buf_len,
                              CompletionCallback* callback) {
   DCHECK(completed_handshake_);
diff --git a/net/base/ssl_client_socket_mac.h b/net/base/ssl_client_socket_mac.h
index 1c74b68..c8a43ce 100644
--- a/net/base/ssl_client_socket_mac.h
+++ b/net/base/ssl_client_socket_mac.h
@@ -37,6 +37,7 @@ class SSLClientSocketMac : public SSLClientSocket {
   virtual int ReconnectIgnoringLastError(CompletionCallback* callback);
   virtual void Disconnect();
   virtual bool IsConnected() const;
+  virtual bool IsConnectedAndIdle() const;
 
   // Socket methods:
   virtual int Read(char* buf, int buf_len, CompletionCallback* callback);
diff --git a/net/base/ssl_client_socket_nss.cc b/net/base/ssl_client_socket_nss.cc
index f67c246..648f807 100644
--- a/net/base/ssl_client_socket_nss.cc
+++ b/net/base/ssl_client_socket_nss.cc
@@ -165,12 +165,32 @@ void SSLClientSocketNSS::Disconnect() {
 }
 
 bool SSLClientSocketNSS::IsConnected() const {
+  // Ideally, we should also check if we have received the close_notify alert
+  // message from the server, and return false in that case.  We're not doing
+  // that, so this function may return a false positive.  Since the upper
+  // layer (HttpNetworkTransaction) needs to handle a persistent connection
+  // closed by the server when we send a request anyway, a false positive in
+  // exchange for simpler code is a good trade-off.
   EnterFunction("");
   bool ret = completed_handshake_ && transport_->IsConnected();
   LeaveFunction("");
   return ret;
 }
 
+bool SSLClientSocketNSS::IsConnectedAndIdle() const {
+  // Unlike IsConnected, this method doesn't return a false positive.
+  //
+  // Strictly speaking, we should check if we have received the close_notify
+  // alert message from the server, and return false in that case.  Although
+  // the close_notify alert message means EOF in the SSL layer, it is just
+  // bytes to the transport layer below, so transport_->IsConnectedAndIdle()
+  // returns the desired false when we receive close_notify.
+  EnterFunction("");
+  bool ret = completed_handshake_ && transport_->IsConnectedAndIdle();
+  LeaveFunction("");
+  return ret;
+}
+
 int SSLClientSocketNSS::Read(char* buf, int buf_len,
                              CompletionCallback* callback) {
   EnterFunction(buf_len);
diff --git a/net/base/ssl_client_socket_nss.h b/net/base/ssl_client_socket_nss.h
index b16557c..6729598 100644
--- a/net/base/ssl_client_socket_nss.h
+++ b/net/base/ssl_client_socket_nss.h
@@ -37,6 +37,7 @@ class SSLClientSocketNSS : public SSLClientSocket {
   virtual int ReconnectIgnoringLastError(CompletionCallback* callback);
   virtual void Disconnect();
   virtual bool IsConnected() const;
+  virtual bool IsConnectedAndIdle() const;
 
   // Socket methods:
   virtual int Read(char* buf, int buf_len, CompletionCallback* callback);
diff --git a/net/base/ssl_client_socket_unittest.cc b/net/base/ssl_client_socket_unittest.cc
index 4710151..3264c36 100644
--- a/net/base/ssl_client_socket_unittest.cc
+++ b/net/base/ssl_client_socket_unittest.cc
@@ -166,6 +166,11 @@ TEST_F(SSLClientSocketTest, MAYBE_ConnectMismatched) {
   EXPECT_TRUE(sock->IsConnected());
 }
 
+// TODO(wtc): Add unit tests for IsConnectedAndIdle:
+//   - Server closes an SSL connection (with a close_notify alert message).
+//   - Server closes the underlying TCP connection directly.
+//   - Server sends data unexpectedly.
+
 TEST_F(SSLClientSocketTest, MAYBE_Read) {
   StartOKServer();
 
diff --git a/net/base/ssl_client_socket_win.cc b/net/base/ssl_client_socket_win.cc
index 5f73746..6aba9c2 100644
--- a/net/base/ssl_client_socket_win.cc
+++ b/net/base/ssl_client_socket_win.cc
@@ -314,6 +314,17 @@ bool SSLClientSocketWin::IsConnected() const {
   return completed_handshake_ && transport_->IsConnected();
 }
 
+bool SSLClientSocketWin::IsConnectedAndIdle() const {
+  // Unlike IsConnected, this method doesn't return a false positive.
+  //
+  // Strictly speaking, we should check if we have received the close_notify
+  // alert message from the server, and return false in that case.  Although
+  // the close_notify alert message means EOF in the SSL layer, it is just
+  // bytes to the transport layer below, so transport_->IsConnectedAndIdle()
+  // returns the desired false when we receive close_notify.
+  return completed_handshake_ && transport_->IsConnectedAndIdle();
+}
+
 int SSLClientSocketWin::Read(char* buf, int buf_len,
                              CompletionCallback* callback) {
   DCHECK(completed_handshake_);
diff --git a/net/base/ssl_client_socket_win.h b/net/base/ssl_client_socket_win.h
index 6fd19a0..feba718 100644
--- a/net/base/ssl_client_socket_win.h
+++ b/net/base/ssl_client_socket_win.h
@@ -42,6 +42,7 @@ class SSLClientSocketWin : public SSLClientSocket {
   virtual int ReconnectIgnoringLastError(CompletionCallback* callback);
   virtual void Disconnect();
   virtual bool IsConnected() const;
+  virtual bool IsConnectedAndIdle() const;
 
   // Socket methods:
   virtual int Read(char* buf, int buf_len, CompletionCallback* callback);
diff --git a/net/base/tcp_client_socket.h b/net/base/tcp_client_socket.h
index e4a0848..abb3b8e 100644
--- a/net/base/tcp_client_socket.h
+++ b/net/base/tcp_client_socket.h
@@ -50,6 +50,7 @@ class TCPClientSocket : public ClientSocket,
   virtual int ReconnectIgnoringLastError(CompletionCallback* callback);
   virtual void Disconnect();
   virtual bool IsConnected() const;
+  virtual bool IsConnectedAndIdle() const;
 
   // Socket methods:
   // Multiple outstanding requests are not supported.
diff --git a/net/base/tcp_client_socket_libevent.cc b/net/base/tcp_client_socket_libevent.cc
index 7a5776a..4722094 100644
--- a/net/base/tcp_client_socket_libevent.cc
+++ b/net/base/tcp_client_socket_libevent.cc
@@ -161,6 +161,22 @@ bool TCPClientSocket::IsConnected() const {
   return true;
 }
 
+bool TCPClientSocket::IsConnectedAndIdle() const {
+  if (socket_ == kInvalidSocket || waiting_connect_)
+    return false;
+
+  // Check if connection is alive and we haven't received any data
+  // unexpectedly.
+  char c;
+  int rv = recv(socket_, &c, 1, MSG_PEEK);
+  if (rv >= 0)
+    return false;
+  if (errno != EAGAIN && errno != EWOULDBLOCK)
+    return false;
+
+  return true;
+}
+
 int TCPClientSocket::Read(char* buf,
                           int buf_len,
                           CompletionCallback* callback) {
diff --git a/net/base/tcp_client_socket_unittest.cc b/net/base/tcp_client_socket_unittest.cc
index 976ff1e..536528b 100644
--- a/net/base/tcp_client_socket_unittest.cc
+++ b/net/base/tcp_client_socket_unittest.cc
@@ -89,6 +89,10 @@ TEST_F(TCPClientSocketTest, Connect) {
   EXPECT_FALSE(sock.IsConnected());
 }
 
+// TODO(wtc): Add unit tests for IsConnectedAndIdle:
+//   - Server closes a connection.
+//   - Server sends data unexpectedly.
+
 TEST_F(TCPClientSocketTest, Read) {
   net::AddressList addr;
   net::HostResolver resolver;
diff --git a/net/base/tcp_client_socket_win.cc b/net/base/tcp_client_socket_win.cc
index d2a1452..18afe03c 100644
--- a/net/base/tcp_client_socket_win.cc
+++ b/net/base/tcp_client_socket_win.cc
@@ -157,6 +157,22 @@ bool TCPClientSocket::IsConnected() const {
   return true;
 }
 
+bool TCPClientSocket::IsConnectedAndIdle() const {
+  if (socket_ == INVALID_SOCKET || wait_state_ == WAITING_CONNECT)
+    return false;
+
+  // Check if connection is alive and we haven't received any data
+  // unexpectedly.
+  char c;
+  int rv = recv(socket_, &c, 1, MSG_PEEK);
+  if (rv >= 0)
+    return false;
+  if (WSAGetLastError() != WSAEWOULDBLOCK)
+    return false;
+
+  return true;
+}
+
 int TCPClientSocket::Read(char* buf,
                           int buf_len,
                           CompletionCallback* callback) {
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 52e299f..5ceeb39 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -109,6 +109,9 @@ class MockTCPClientSocket : public net::ClientSocket {
   virtual bool IsConnected() const {
     return connected_;
   }
+  virtual bool IsConnectedAndIdle() const {
+    return connected_;
+  }
   // Socket methods:
   virtual int Read(char* buf, int buf_len, net::CompletionCallback* callback) {
     DCHECK(!callback_);