Add Socket::ReadIfReady()

net/socket/socket_posix.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/socket_posix.h"
 
 #include <errno.h>
 #include <netinet/in.h>
 #include <sys/socket.h>
 #include <utility>
@@ skipping 257 matching lines @@
     PLOG(ERROR) << "WatchFileDescriptor failed on read, errno " << errno;
     return MapSystemError(errno);
   }
 
   read_buf_ = buf;
   read_buf_len_ = buf_len;
   read_callback_ = callback;
   return ERR_IO_PENDING;
 }
 
+int SocketPosix::ReadIfReady(IOBuffer* buf,
+                             int buf_len,
+                             const CompletionCallback& callback) {
+  // Reuse Read() but do not hold on to |buf| and |buf_len|.
+  int rv = Read(buf, buf_len, callback);
+  read_buf_ = nullptr;
+  read_buf_len_ = 0;
+  return rv;
+}

[davidben, 2017/02/01 22:25:57]

Nit: It might be a little clearer to make lines 253 through 270 be ReadIfReady
and then do:

int SocketPosix::Read(...) {
  int rv = ReadIfReady(buf, buf_len, callback);
  if (rv == ERR_IO_PENDING) {
    // Save |buf| so ReadCompleted does not call the callback until
    // driving the read to completion.
    read_buf_ = buf;
    read_buf_len_ = callback;
  }
  return rv;
}

It's the same thing, but ReadIfReady feels like the more low-level operation.

-----

An alternative to this and the comment below, though it involves stashing two
callbacks, is to implement Read() entirely on top of ReadIfReady(). This could
be done for basically all sockets. So you'd store two operations worth of state:

ReadIfReady():
  read_if_ready_callback_

Read():
  read_buf_
  read_buf_len_
  read_callback_

Everything would act as if Read() and its state doesn't exist. Then you
implement Read() basically universally as:

  int Read(buf, buf_len, callback) {
    int rv = ReadIfReady(buf, buf_len, &RetryRead);
    if (rv != ERR_IO_PENDING)
      return rv;

    read_buf_ = buf;
    read_buf_len_ = buf_len;
    read_callback_ = callback;
  }

  void RetryRead(int rv) {
    if (rv == OK) {
      rv = ReadIfReady(read_buf_, read_buf_len_, &RetryRead);
      if (rv == ERR_IO_PENDING)
        return;
    }

    read_buf_ = nullptr;
    read_buf_len_ = 0;
    ResetAndReturn(&read_callback, rv);
  }

Though if you do this for SocketPosix, you'll bounce on
{Start,Stop}WatchingFileDescriptor which is a little unfortunate. Maybe this
would be better for the higher-level ones?

-----

What do you think?

[xunjieli, 2017/02/03 16:35:33]

Done. I like your approach better. It's easier to understand. Thanks for the
suggestion.

But we wouldn't bounce on {Start, Stop}WatchingFileDescriptor more often for the
regular Read() than before, right?

Read() -> ReadIfReady() -> StartWatching -> ReadCompleted() -> StopWatching ->
RetryRead -> DoRead().
There's only one pair of {Start, Stop} for a single Read(), same as before,
right?

+
 int SocketPosix::Write(IOBuffer* buf,
                        int buf_len,
                        const CompletionCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_NE(kInvalidSocket, socket_fd_);
   DCHECK(!waiting_connect_);
   CHECK(write_callback_.is_null());
   // Synchronous operation not supported
   DCHECK(!callback.is_null());
   DCHECK_LT(0, buf_len);
@@ skipping 156 matching lines @@
   waiting_connect_ = false;
   base::ResetAndReturn(&write_callback_).Run(rv);
 }
 
 int SocketPosix::DoRead(IOBuffer* buf, int buf_len) {
   int rv = HANDLE_EINTR(read(socket_fd_, buf->data(), buf_len));
   return rv >= 0 ? rv : MapSystemError(errno);
 }
 
 void SocketPosix::ReadCompleted() {
+  // If |read_buf_| is nullptr, ReadIfReady() is used instead of Read(), so
+  // do not continue reading but wait for the caller to retry.
+  if (read_buf_ == nullptr) {
+    bool ok = read_socket_watcher_.StopWatchingFileDescriptor();
+    DCHECK(ok);
+    base::ResetAndReturn(&read_callback_).Run(OK);
+    return;
+  }
   int rv = DoRead(read_buf_.get(), read_buf_len_);
   if (rv == ERR_IO_PENDING)
     return;
 
   bool ok = read_socket_watcher_.StopWatchingFileDescriptor();
   DCHECK(ok);
   read_buf_ = NULL;
   read_buf_len_ = 0;
   base::ResetAndReturn(&read_callback_).Run(rv);

[davidben, 2017/02/01 22:25:57]

You could also write this:

  int rv = OK;
  // If |read_buf_| is set, the caller used Read() rather than
  // ReadIfReady(). Continue attempting to read and only return
  // when completed.
  if (read_buf_) {
    rv = DoRead(read_buf_.get(), read_buf_len_);
    if (rv == ERR_IO_PENDING)
      return;
  }

  bool ok = ....
  ...
  base::ResetAndReturn(....
}

This avoids duplicating the chunk at the bottom

[xunjieli, 2017/02/03 16:35:33]

Acknowledged. Thanks. This is no longer applicable after applying the the
previous suggestion.

 }
 
 int SocketPosix::DoWrite(IOBuffer* buf, int buf_len) {
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   // Disable SIGPIPE for this write. Although Chromium globally disables
   // SIGPIPE, the net stack may be used in other consumers which do not do
   // this. MSG_NOSIGNAL is a Linux-only API. On OS X, this is a setsockopt on
   // socket creation.
   int rv = HANDLE_EINTR(send(socket_fd_, buf->data(), buf_len, MSG_NOSIGNAL));
 #else
@@ skipping 37 matching lines @@
     write_buf_ = NULL;
     write_buf_len_ = 0;
     write_callback_.Reset();
   }
 
   waiting_connect_ = false;
   peer_address_.reset();
 }
 
 }  // namespace net