Add Socket::ReadIfReady()

net/socket/socket_posix.cc

Index: net/socket/socket_posix.cc
diff --git a/net/socket/socket_posix.cc b/net/socket/socket_posix.cc
index 09d9baec87e0139e41226b8a1b8b5a8de5c6a6c6..aac5fbabc103e8fc1b660da10912bc197c19789b 100644
--- a/net/socket/socket_posix.cc
+++ b/net/socket/socket_posix.cc
@@ -275,6 +275,16 @@ int SocketPosix::Read(IOBuffer* buf,
   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) {
@@ -451,6 +461,14 @@ int SocketPosix::DoRead(IOBuffer* buf, int buf_len) {
 }
 
 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;