Add Socket::ReadIfReady()

net/socket/ssl_client_socket_impl.cc

Index: net/socket/ssl_client_socket_impl.cc
diff --git a/net/socket/ssl_client_socket_impl.cc b/net/socket/ssl_client_socket_impl.cc
index c993fa8fb23da2d1d58c35731b3801a19283ff11..ea34fb6d593498cb19d7ab40c38e9cb36fa6478a 100644
--- a/net/socket/ssl_client_socket_impl.cc
+++ b/net/socket/ssl_client_socket_impl.cc
@@ -860,20 +860,25 @@ void SSLClientSocketImpl::DumpSSLClientSessionMemoryStats(
 int SSLClientSocketImpl::Read(IOBuffer* buf,
                               int buf_len,
                               const CompletionCallback& callback) {
-  user_read_buf_ = buf;
-  user_read_buf_len_ = buf_len;
+  int rv = ReadIfReady(buf, buf_len, callback);
+  if (rv == ERR_IO_PENDING) {
+    user_read_buf_ = buf;
+    user_read_buf_len_ = buf_len;
+  }
+  return rv;
+}
 
-  int rv = DoPayloadRead();
+int SSLClientSocketImpl::ReadIfReady(IOBuffer* buf,
+                                     int buf_len,
+                                     const CompletionCallback& callback) {
+  int rv = DoPayloadRead(buf, buf_len);
 
   if (rv == ERR_IO_PENDING) {
     user_read_callback_ = callback;
   } else {
     if (rv > 0)
       was_ever_used_ = true;
-    user_read_buf_ = NULL;
-    user_read_buf_len_ = 0;
   }
-
   return rv;
 }
 
@@ -905,16 +910,16 @@ int SSLClientSocketImpl::SetSendBufferSize(int32_t size) {
   return transport_->socket()->SetSendBufferSize(size);
 }
 
-void SSLClientSocketImpl::OnReadReady() {
+void SSLClientSocketImpl::OnReadReady(int rv) {
   // During a renegotiation, either Read or Write calls may be blocked on a
   // transport read.
-  RetryAllOperations();
+  RetryAllOperations(rv);
 }
 
-void SSLClientSocketImpl::OnWriteReady() {
+void SSLClientSocketImpl::OnWriteReady(int rv) {
   // During a renegotiation, either Read or Write calls may be blocked on a
   // transport read.
-  RetryAllOperations();
+  RetryAllOperations(rv);
 }
 
 int SSLClientSocketImpl::Init() {
@@ -1050,7 +1055,7 @@ void SSLClientSocketImpl::DoReadCallback(int rv) {
   // up front.
   if (rv > 0)
     was_ever_used_ = true;
-  user_read_buf_ = NULL;
+  user_read_buf_ = nullptr;
   user_read_buf_len_ = 0;
   base::ResetAndReturn(&user_read_callback_).Run(rv);
 }
@@ -1405,11 +1410,11 @@ int SSLClientSocketImpl::DoHandshakeLoop(int last_io_result) {
   return rv;
 }
 
-int SSLClientSocketImpl::DoPayloadRead() {
+int SSLClientSocketImpl::DoPayloadRead(IOBuffer* buf, int buf_len) {
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
 
-  DCHECK_LT(0, user_read_buf_len_);
-  DCHECK(user_read_buf_.get());
+  DCHECK_LT(0, buf_len);
+  DCHECK(buf);
 
   int rv;
   if (pending_read_error_ != kNoPendingResult) {
@@ -1417,7 +1422,7 @@ int SSLClientSocketImpl::DoPayloadRead() {
     pending_read_error_ = kNoPendingResult;
     if (rv == 0) {
       net_log_.AddByteTransferEvent(NetLogEventType::SSL_SOCKET_BYTES_RECEIVED,
-                                    rv, user_read_buf_->data());
+                                    rv, buf->data());
     } else {
       net_log_.AddEvent(
           NetLogEventType::SSL_READ_ERROR,
@@ -1432,11 +1437,11 @@ int SSLClientSocketImpl::DoPayloadRead() {
   int total_bytes_read = 0;
   int ssl_ret;
   do {
-    ssl_ret = SSL_read(ssl_.get(), user_read_buf_->data() + total_bytes_read,
-                       user_read_buf_len_ - total_bytes_read);
+    ssl_ret = SSL_read(ssl_.get(), buf->data() + total_bytes_read,
+                       buf_len - total_bytes_read);
     if (ssl_ret > 0)
       total_bytes_read += ssl_ret;
-  } while (total_bytes_read < user_read_buf_len_ && ssl_ret > 0);
+  } while (total_bytes_read < buf_len && ssl_ret > 0);
 
   // Although only the final SSL_read call may have failed, the failure needs to
   // processed immediately, while the information still available in OpenSSL's
@@ -1493,7 +1498,7 @@ int SSLClientSocketImpl::DoPayloadRead() {
 
   if (rv >= 0) {
     net_log_.AddByteTransferEvent(NetLogEventType::SSL_SOCKET_BYTES_RECEIVED,
-                                  rv, user_read_buf_->data());
+                                  rv, buf->data());
   } else if (rv != ERR_IO_PENDING) {
     net_log_.AddEvent(
         NetLogEventType::SSL_READ_ERROR,
@@ -1529,7 +1534,10 @@ int SSLClientSocketImpl::DoPayloadWrite() {
   return net_error;
 }
 
-void SSLClientSocketImpl::RetryAllOperations() {
+void SSLClientSocketImpl::RetryAllOperations(int rv) {
+  DCHECK_NE(ERR_IO_PENDING, rv);
+  DCHECK_GE(OK, rv);
+
   // SSL_do_handshake, SSL_read, and SSL_write may all be retried when blocked,
   // so retry all operations for simplicity. (Otherwise, SSL_get_error for each
   // operation may be remembered to retry only the blocked ones.)
@@ -1542,8 +1550,13 @@ void SSLClientSocketImpl::RetryAllOperations() {
 
   int rv_read = ERR_IO_PENDING;
   int rv_write = ERR_IO_PENDING;
-  if (user_read_buf_)
-    rv_read = DoPayloadRead();
+  if (user_read_buf_) {
+    rv_read = DoPayloadRead(user_read_buf_.get(), user_read_buf_len_);
+  } else if (!user_read_callback_.is_null()) {
+    // When ReadIfReady() is used, skip DoPayloadRead().
+    rv_read = rv;

[davidben, 2017/02/10 23:33:48]

Hrm. I'm not sure this is quite right. Suppose we get OnWriteReady(ERR_FAILED).
Read() may or may not have been blocked on the underlying write channel. You
only want to forward the error if it was blocked. All operations are safe to
pump whether they're ready or not, so rather than keep track of what everything
is blocked on, this code just retries everything for simplicity.

Instead, I believe if you remove the return value and just do:

  rv_read = OK;

It will work? Even if we woke up because the transport Read was stuck on
ERR_CONNECTION_CLOSED, the caller will ReadIfReady() again and then we'll
synchronously get back the ERR_CONNECTION_CLOSED back out. (SocketBIOAdapter
stashes the error and replays it, to satisfy the "it is always safe to retry a
blocked op" constraint.)

[xunjieli, 2017/02/13 20:28:18]

Done.

+  }
+
   if (user_write_buf_)
     rv_write = DoPayloadWrite();
 
@@ -1889,7 +1902,7 @@ void SSLClientSocketImpl::OnPrivateKeyComplete(
 
   // During a renegotiation, either Read or Write calls may be blocked on an
   // asynchronous private key operation.
-  RetryAllOperations();
+  RetryAllOperations(error);
 }
 
 int SSLClientSocketImpl::TokenBindingAdd(const uint8_t** out,