Clarify the semantics of Socket::Read() and Socket::Write(),

particularly as they related to closed / disconnected sockets.

Added a new ERR_SOCKET_NOT_CONNECTED error which is now returned by GetPeerAddress.

This addresses wtc's feedback on CL 3421028.

BUG=56423,56426
TEST=none

Review URL: http://codereview.chromium.org/3384034

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

7 files changed, 38 insertions, 31 deletions

diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
index 9e4ec6a..b96524d 100644
--- a/net/base/net_error_list.h
+++ b/net/base/net_error_list.h
@@ -66,6 +66,9 @@ NET_ERROR(OUT_OF_MEMORY, -13)
 // from the expectation.
 NET_ERROR(UPLOAD_FILE_CHANGED, -14)
 
+// The socket is not connected
+NET_ERROR(SOCKET_NOT_CONNECTED, -15)
+
 // A connection was closed (corresponding to a TCP FIN).
 NET_ERROR(CONNECTION_CLOSED, -100)
 
diff --git a/net/socket/client_socket.h b/net/socket/client_socket.h
index e7ba153..f44f1f1 100644
--- a/net/socket/client_socket.h
+++ b/net/socket/client_socket.h
@@ -52,7 +52,7 @@ class ClientSocket : public Socket {
   virtual bool IsConnectedAndIdle() const = 0;
 
   // Copies the peer address to |address| and returns a network error code.
-  // ERR_UNEXPECTED will be returned if the socket is not connected.
+  // ERR_SOCKET_NOT_CONNECTED will be returned if the socket is not connected.
   virtual int GetPeerAddress(AddressList* address) const = 0;
 
   // Gets the NetLog for this socket.
diff --git a/net/socket/socket.h b/net/socket/socket.h
index 8addca7..c3465ea 100644
--- a/net/socket/socket.h
+++ b/net/socket/socket.h
@@ -18,28 +18,31 @@ class Socket {
   virtual ~Socket() {}
 
   // 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 once
-  // to indicate end-of-file; subsequent calls return ERR_CONNECTION_CLOSED.
-  // 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. If the operation is not completed immediately, the socket
-  // acquires a reference to the provided buffer until the callback is invoked
-  // or the socket is destroyed.  If the socket is closed before the read
-  // completes, the callback will not be invoked.
+  // is returned, or an error is returned upon failure.
+  // ERR_SOCKET_NOT_CONNECTED should be returned if the socket is not currently
+  // connected.  Zero is returned once to indicate end-of-file; the return value
+  // of subsequent calls is undefined, and may be OS dependent.  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. If the
+  // operation is not completed immediately, the socket acquires a reference to
+  // the provided buffer until the callback is invoked or the socket is
+  // destroyed.  If the socket is closed before the read completes, the callback
+  // will not be invoked.
   virtual int Read(IOBuffer* buf, int buf_len,
                    CompletionCallback* callback) = 0;
 
   // Writes data, up to buf_len bytes, to the socket.  Note: only part of the
   // data may be written!  The number of bytes written is returned, or an error
-  // is returned upon failure.  ERR_CONNECTION_CLOSED is returned if the
-  // operation is attempted on a closed socket.  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. If the operation is
-  // not completed immediately, the socket acquires a reference to the provided
-  // buffer until the callback is invoked or the socket is destroyed.
-  // Implementations of this method should not modify the contents of the actual
-  // buffer that is written to the socket.  If the socket is closed before the
-  // write completes, the callback will not be invoked.
+  // is returned upon failure.  ERR_SOCKET_NOT_CONNECTED should be returned if
+  // the socket is not currently connected.  The return value when the
+  // connection is closed is undefined, and may be OS dependent.  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.  If the
+  // operation is not completed immediately, the socket acquires a reference to
+  // the provided buffer until the callback is invoked or the socket is
+  // destroyed.  Implementations of this method should not modify the contents
+  // of the actual buffer that is written to the socket.  If the socket is
+  // closed before the write completes, the callback will not be invoked.
   virtual int Write(IOBuffer* buf, int buf_len,
                     CompletionCallback* callback) = 0;
 
diff --git a/net/socket/tcp_client_socket_libevent.cc b/net/socket/tcp_client_socket_libevent.cc
index 6f7403b..0509388 100644
--- a/net/socket/tcp_client_socket_libevent.cc
+++ b/net/socket/tcp_client_socket_libevent.cc
@@ -529,7 +529,7 @@ int TCPClientSocketLibevent::GetPeerAddress(AddressList* address) const {
   DCHECK(CalledOnValidThread());
   DCHECK(address);
   if (!IsConnected())
-    return ERR_UNEXPECTED;
+    return ERR_SOCKET_NOT_CONNECTED;
   address->Copy(current_ai_, false);
   return OK;
 }
diff --git a/net/socket/tcp_client_socket_win.cc b/net/socket/tcp_client_socket_win.cc
index f6a2e81..e6d6b77 100644
--- a/net/socket/tcp_client_socket_win.cc
+++ b/net/socket/tcp_client_socket_win.cc
@@ -78,21 +78,20 @@ int MapWinsockError(int os_error) {
       return ERR_CONNECTION_ABORTED;
     case WSAECONNREFUSED:
       return ERR_CONNECTION_REFUSED;
+    case WSA_IO_INCOMPLETE:
     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();
+      // WSAEDISCON is 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.
+      LOG(DFATAL) << "Unexpected error " << os_error
+                  << " mapped to net::ERR_UNEXPECTED";
       return ERR_UNEXPECTED;
     case WSAEHOSTUNREACH:
     case WSAENETUNREACH:
       return ERR_ADDRESS_UNREACHABLE;
     case WSAEADDRNOTAVAIL:
       return ERR_ADDRESS_INVALID;
-    case WSA_IO_INCOMPLETE:
-      LOG(ERROR) << "Unexpected error " << os_error;
-      return ERR_UNEXPECTED;
     case ERROR_SUCCESS:
       return OK;
     default:
@@ -512,7 +511,7 @@ int TCPClientSocketWin::GetPeerAddress(AddressList* address) const {
   DCHECK(CalledOnValidThread());
   DCHECK(address);
   if (!IsConnected())
-    return ERR_UNEXPECTED;
+    return ERR_SOCKET_NOT_CONNECTED;
   address->Copy(current_ai_, false);
   return OK;
 }
diff --git a/net/spdy/spdy_proxy_client_socket.cc b/net/spdy/spdy_proxy_client_socket.cc
index e282d76..966a82c 100644
--- a/net/spdy/spdy_proxy_client_socket.cc
+++ b/net/spdy/spdy_proxy_client_socket.cc
@@ -209,7 +209,7 @@ bool SpdyProxyClientSocket::SetSendBufferSize(int32 size) {
 
 int SpdyProxyClientSocket::GetPeerAddress(AddressList* address) const {
   if (!IsConnected())
-    return ERR_UNEXPECTED;
+    return ERR_SOCKET_NOT_CONNECTED;
   return spdy_stream_->GetPeerAddress(address);
 }
 
diff --git a/net/spdy/spdy_proxy_client_socket_unittest.cc b/net/spdy/spdy_proxy_client_socket_unittest.cc
index daf158c..274c22f 100644
--- a/net/spdy/spdy_proxy_client_socket_unittest.cc
+++ b/net/spdy/spdy_proxy_client_socket_unittest.cc
@@ -521,12 +521,14 @@ TEST_F(SpdyProxyClientSocketTest, GetPeerAddressReturnsCorrectValues) {
   Initialize(reads, arraysize(reads), writes, arraysize(writes));
 
   net::AddressList addr;
-  EXPECT_EQ(ERR_UNEXPECTED, sock_->GetPeerAddress(&addr));
+  EXPECT_EQ(ERR_SOCKET_NOT_CONNECTED, sock_->GetPeerAddress(&addr));
+
   AssertConnectSucceeds();
   EXPECT_TRUE(sock_->IsConnected());
   EXPECT_EQ(OK, sock_->GetPeerAddress(&addr));
+
   sock_->Disconnect();
-  EXPECT_EQ(ERR_UNEXPECTED, sock_->GetPeerAddress(&addr));
+  EXPECT_EQ(ERR_SOCKET_NOT_CONNECTED, sock_->GetPeerAddress(&addr));
 }
 
 // ----------- Write