Make some changes from my code review. Two significant

changes are:
1. Call WSAEventSelect before connect to fix a race
   condition.
2. Call WSAResetEvent before WSARecv and WSASend because
   the event is manual-reset and WSAGetOverlappedResult
   doesn't reset the event for us.

R=darin

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

6 files changed, 64 insertions, 33 deletions

diff --git a/net/base/client_socket.h b/net/base/client_socket.h
index 8661adc..1f61c0f 100644
--- a/net/base/client_socket.h
+++ b/net/base/client_socket.h
@@ -38,9 +38,9 @@ class ClientSocket : public Socket {
  public:
   // Called to establish a connection.  Returns OK if the connection could be
   // established synchronously.  Otherwise, ERR_IO_PENDING is returned and the
-  // given callback will be notified asynchronously when the connection is
-  // established or when an error occurs.  The result is some other error code
-  // if the connection could not be established.
+  // given callback will run asynchronously when the connection is established
+  // or when an error occurs.  The result is some other error code if the
+  // connection could not be established.
   //
   // The socket's Read and Write methods may not be called until Connect
   // succeeds.
@@ -48,7 +48,7 @@ class ClientSocket : public Socket {
   // It is valid to call Connect on an already connected socket, in which case
   // OK is simply returned.
   //
-  // Connect may also be called again after a call to the Close method.
+  // Connect may also be called again after a call to the Disconnect method.
   //
   virtual int Connect(CompletionCallback* callback) = 0;
 
@@ -62,7 +62,8 @@ class ClientSocket : public Socket {
   // Connect again to establish a new connection.
   virtual void Disconnect() = 0;
 
-  // Called to test if the socket is connected.
+  // Called to test if the connection is still alive.  Returns false if a
+  // connection wasn't established or the connection is dead.
   virtual bool IsConnected() const = 0;
 };
 
diff --git a/net/base/client_socket_factory.h b/net/base/client_socket_factory.h
index 9a1b412..4115d74 100644
--- a/net/base/client_socket_factory.h
+++ b/net/base/client_socket_factory.h
@@ -41,6 +41,8 @@ class ClientSocket;
 // testing code with mock socket implementations.
 class ClientSocketFactory {
  public:
+  virtual ~ClientSocketFactory() {}
+
   virtual ClientSocket* CreateTCPClientSocket(
       const AddressList& addresses) = 0;
 
diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
index 1b12af4..9a6c4f0 100644
--- a/net/base/net_error_list.h
+++ b/net/base/net_error_list.h
@@ -56,6 +56,10 @@ NET_ERROR(TIMED_OUT, -7)
 // The file is too large.
 NET_ERROR(FILE_TOO_BIG, -8)
 
+// An unexpected error.  This may be caused by a programming mistake or an
+// invalid assumption.
+NET_ERROR(UNEXPECTED, -9)
+
 // A connection was closed (corresponding to a TCP FIN).
 NET_ERROR(CONNECTION_CLOSED, -100)
 
diff --git a/net/base/socket.h b/net/base/socket.h
index 11f0906e..8891339 100644
--- a/net/base/socket.h
+++ b/net/base/socket.h
@@ -39,11 +39,11 @@ class Socket {
  public:
   virtual ~Socket() {}
 
-  // Read data, up to buf_len bytes, from the socket.  The number of bytes read
-  // is returned, or an error is returned upon failure.  Zero is returned to
-  // indicate end-of-file.  ERR_IO_PENDING is returned if the operation could
-  // not be completed synchronously, in which case the result will be passed to
-  // the callback when available.
+  // Reads data, up to buf_len bytes, from the socket.  The number of bytes
+  // read is returned, or an error is returned upon failure.  Zero is returned
+  // to indicate end-of-file.  ERR_IO_PENDING is returned if the operation
+  // could not be completed synchronously, in which case the result will be
+  // passed to the callback when available.
   virtual int Read(char* buf, int buf_len,
                    CompletionCallback* callback) = 0;
 
diff --git a/net/base/tcp_client_socket.cc b/net/base/tcp_client_socket.cc
index 6f9e52b..cef7ef2 100644
--- a/net/base/tcp_client_socket.cc
+++ b/net/base/tcp_client_socket.cc
@@ -45,19 +45,26 @@ static int MapWinsockError(DWORD err) {
     case WSAETIMEDOUT:
       return ERR_TIMED_OUT;
     case WSAECONNRESET:
-    case WSAENETRESET:
+    case WSAENETRESET:  // Related to keep-alive
       return ERR_CONNECTION_RESET;
     case WSAECONNABORTED:
       return ERR_CONNECTION_ABORTED;
     case WSAECONNREFUSED:
       return ERR_CONNECTION_REFUSED;
     case WSAEDISCON:
+      // Returned by WSARecv or WSARecvFrom for message-oriented sockets (where
+      // a return value of zero means a zero-byte message) to indicate graceful
+      // connection shutdown.  We should not ever see this error code for TCP
+      // sockets, which are byte stream oriented.
+      NOTREACHED();
       return ERR_CONNECTION_CLOSED;
     case WSAEHOSTUNREACH:
     case WSAENETUNREACH:
       return ERR_ADDRESS_UNREACHABLE;
     case WSAEADDRNOTAVAIL:
       return ERR_ADDRESS_INVALID;
+    case WSA_IO_INCOMPLETE:
+      return ERR_UNEXPECTED;
     case ERROR_SUCCESS:
       return OK;
     default:
@@ -92,6 +99,11 @@ int TCPClientSocket::Connect(CompletionCallback* callback) {
   if (rv != OK)
     return rv;
 
+  overlapped_.hEvent = WSACreateEvent();
+  // WSAEventSelect sets the socket to non-blocking mode as a side effect.
+  // Our connect() and recv() calls require that the socket be non-blocking.
+  WSAEventSelect(socket_, overlapped_.hEvent, FD_CONNECT);
+
   if (!connect(socket_, ai->ai_addr, static_cast<int>(ai->ai_addrlen))) {
     // Connected without waiting!
     return OK;
@@ -103,9 +115,6 @@ int TCPClientSocket::Connect(CompletionCallback* callback) {
     return MapWinsockError(err);
   }
 
-  overlapped_.hEvent = WSACreateEvent();
-  WSAEventSelect(socket_, overlapped_.hEvent, FD_CONNECT);
-
   watcher_.StartWatching(overlapped_.hEvent, this);
   wait_state_ = WAITING_CONNECT;
   callback_ = callback;
@@ -124,12 +133,18 @@ void TCPClientSocket::Disconnect() {
   // Make sure the message loop is not watching this object anymore.
   watcher_.StopWatching();
 
+  // In most socket implementations, closing a socket results in a graceful
+  // connection shutdown, but in Winsock we have to call shutdown explicitly.
+  // See the MSDN page "Graceful Shutdown, Linger Options, and Socket Closure"
+  // at http://msdn.microsoft.com/en-us/library/ms738547.aspx
+  shutdown(socket_, SD_SEND);
+
   // This cancels any pending IO.
   closesocket(socket_);
   socket_ = INVALID_SOCKET;
 
   WSACloseEvent(overlapped_.hEvent);
-  overlapped_.hEvent = NULL;
+  memset(&overlapped_, 0, sizeof(overlapped_));
 
   // Reset for next time.
   current_ai_ = addresses_.head();
@@ -150,7 +165,9 @@ bool TCPClientSocket::IsConnected() const {
   return true;
 }
 
-int TCPClientSocket::Read(char* buf, int buf_len, CompletionCallback* callback) {
+int TCPClientSocket::Read(char* buf,
+                          int buf_len,
+                          CompletionCallback* callback) {
   DCHECK(socket_ != INVALID_SOCKET);
   DCHECK(wait_state_ == NOT_WAITING);
   DCHECK(!callback_);
@@ -171,7 +188,9 @@ int TCPClientSocket::Read(char* buf, int buf_len, CompletionCallback* callback)
   return MapWinsockError(WSAGetLastError());
 }
 
-int TCPClientSocket::Write(const char* buf, int buf_len, CompletionCallback* callback) {
+int TCPClientSocket::Write(const char* buf,
+                           int buf_len,
+                           CompletionCallback* callback) {
   DCHECK(socket_ != INVALID_SOCKET);
   DCHECK(wait_state_ == NOT_WAITING);
   DCHECK(!callback_);
@@ -196,17 +215,10 @@ int TCPClientSocket::CreateSocket(const struct addrinfo* ai) {
   socket_ = WSASocket(ai->ai_family, ai->ai_socktype, ai->ai_protocol, NULL, 0,
                       WSA_FLAG_OVERLAPPED);
   if (socket_ == INVALID_SOCKET) {
-    LOG(ERROR) << "WSASocket failed: " << WSAGetLastError();
-    return ERR_FAILED;
-  }
-
-  // Configure non-blocking mode.
-  u_long non_blocking_mode = 1;
-  if (ioctlsocket(socket_, FIONBIO, &non_blocking_mode)) {
-    LOG(ERROR) << "ioctlsocket failed: " << WSAGetLastError();
-    return ERR_FAILED;
+    DWORD err = WSAGetLastError();
+    LOG(ERROR) << "WSASocket failed: " << err;
+    return MapWinsockError(err);
   }
-
   return OK;
 }
 
@@ -226,8 +238,11 @@ void TCPClientSocket::DidCompleteConnect() {
   wait_state_ = NOT_WAITING;
 
   WSANETWORKEVENTS events;
-  WSAEnumNetworkEvents(socket_, overlapped_.hEvent, &events);
-  if (events.lNetworkEvents & FD_CONNECT) {
+  int rv = WSAEnumNetworkEvents(socket_, overlapped_.hEvent, &events);
+  if (rv == SOCKET_ERROR) {
+    NOTREACHED();
+    result = MapWinsockError(WSAGetLastError());
+  } else if (events.lNetworkEvents & FD_CONNECT) {
     wait_state_ = NOT_WAITING;
     DWORD error_code = static_cast<DWORD>(events.iErrorCode[FD_CONNECT_BIT]);
     if (current_ai_->ai_next && (
@@ -258,6 +273,7 @@ void TCPClientSocket::DidCompleteIO() {
   DWORD num_bytes, flags;
   BOOL ok = WSAGetOverlappedResult(
       socket_, &overlapped_, &num_bytes, FALSE, &flags);
+  WSAResetEvent(overlapped_.hEvent);
   wait_state_ = NOT_WAITING;
   DoCallback(ok ? num_bytes : MapWinsockError(WSAGetLastError()));
 }
@@ -273,6 +289,9 @@ void TCPClientSocket::OnObjectSignaled(HANDLE object) {
     case WAITING_WRITE:
       DidCompleteIO();
       break;
+    default:
+      NOTREACHED();
+      break;
   }
 }
 
diff --git a/net/base/tcp_client_socket.h b/net/base/tcp_client_socket.h
index e5c8ca9..3885026 100644
--- a/net/base/tcp_client_socket.h
+++ b/net/base/tcp_client_socket.h
@@ -38,13 +38,17 @@
 
 namespace net {
 
+// A client socket that uses TCP as the transport layer.
+//
+// NOTE: The implementation supports half duplex only.  Read and Write calls
+// must not be in progress at the same time.
 class TCPClientSocket : public ClientSocket,
                         public base::ObjectWatcher::Delegate {
  public:
   // The IP address(es) and port number to connect to.  The TCP socket will try
   // each IP address in the list until it succeeds in establishing a
   // connection.
-  TCPClientSocket(const AddressList& addresses);
+  explicit TCPClientSocket(const AddressList& addresses);
 
   ~TCPClientSocket();
 
@@ -64,6 +68,7 @@ class TCPClientSocket : public ClientSocket,
   void DidCompleteConnect();
   void DidCompleteIO();
 
+  // MessageLoop::Watcher methods:
   virtual void OnObjectSignaled(HANDLE object);
 
   SOCKET socket_;
@@ -74,11 +79,11 @@ class TCPClientSocket : public ClientSocket,
 
   CompletionCallback* callback_;
 
-  // Stored outside of the context so we can both lazily construct the context
-  // as well as construct a new one if Connect is called after Close.
+  // The list of addresses we should try in order to establish a connection.
   AddressList addresses_;
 
-  // The addrinfo that we are attempting to use or NULL if uninitialized.
+  // The addrinfo that we are attempting to use or NULL if all addrinfos have
+  // been tried.
   const struct addrinfo* current_ai_;
 
   enum WaitState {