Add Socket::ReadIfReady()

net/socket/socket.h

Index: net/socket/socket.h
diff --git a/net/socket/socket.h b/net/socket/socket.h
index cda54063f3059e76726c896b5e6ae87508a5f483..4876c48ddb6e3c687da6a205de0aa8d0ee9fffd3 100644
--- a/net/socket/socket.h
+++ b/net/socket/socket.h
@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 
+#include "base/feature_list.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_export.h"
 
@@ -17,6 +18,9 @@ class IOBuffer;
 // Represents a read/write socket.
 class NET_EXPORT Socket {
  public:
+  // Name of the field trial for using ReadyIfReady() instead of Read().
+  static const base::Feature kReadIfReadyExperiment;
+
   virtual ~Socket() {}
 
   // Reads data, up to |buf_len| bytes, from the socket.  The number of bytes

[Bence, 2017/03/03 16:33:40]

Optional: add a note at the beginning of the comment about Read() holding on to
|buf|.

[xunjieli, 2017/03/03 19:41:05]

There is a comment about this actually in the paragraph (see below). I will keep
it this way. 

     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
  // closed.  

@@ -33,6 +37,21 @@ class NET_EXPORT Socket {
   virtual int Read(IOBuffer* buf, int buf_len,

[Bence, 2017/03/03 16:33:40]

<rant>

I am unhappy with how a scoped_refptr is passed around as a raw pointer
(produced by get() at the call site) just to be wrapped into another
scoped_refptr inside the Read() implementation.  This is bad mostly because the
argument type does not reflect the semantics.  If Read() took |buf| as
scoped_refptr by value while ReadIfReady() took it by raw pointer, that would
nicely reflect the semantical difference.

This is also bad because it is made possible by scoped_refptr being invasively
implemented (by ScopedRefPtr base class), which is not the only way: for
example, std::shared_ptr is not invasive (refcounter is not inside the instance
but in the wrapper), and taking a raw pointer and constructing a new
std::shared_ptr with that would make it be freed twice, and free after free is
an especially sad kind of use after free.  So being able to pass this by raw
pointer is an implementation detail.

Anyway, a few months ago I looked into what it would take to change |buf|
argument type to scoped_refptr, but there are so many implementations and call
sites that it did not seem like it was worth the effort.  Maybe we should
revisit that to make the distinction between Read() and ReadIfReady() clearer?

</rant>

[xunjieli, 2017/03/03 19:41:05]

Acknowledged. Rant acknowledged :)

                    const CompletionCallback& callback) = 0;
 
+  // Reads as much data as possible into |buf| without blocking. Default

[Bence, 2017/03/03 16:33:40]

Optional: add a note at the beginning of the comment stating that ReadIfReady()
does not hold on to |buf|.

[Bence, 2017/03/03 16:33:40]

What happens if a caller calls ReadIfReady(), that returns asynchronously, then
the caller calls Read()?  Or the other way around?  Is it undefined behavior? 
Should this be forbidden in comments here, or should it be enforced with DCHECKs
in each implementation?

[Bence, 2017/03/03 16:33:40]

If it reads as much data as possible, why does it take a |buf_len| argument?  Or
do you mean it reads at most |buf_len| bytes?

[xunjieli, 2017/03/03 19:41:05]

I will keep it this way. I have a comment in the later section about synchronous
completion and asynchronous completion.
I am having some trouble reorganizing the paragraph so that we can move the
async part to the very front. I don't want to give the impression that we always
hold on to |buf|. It should only happen in the async case for Read().

[xunjieli, 2017/03/03 19:41:05]

Yep, it reads at most |buf_len_|. Clarified in the doc. Done.

[xunjieli, 2017/03/03 19:41:05]

It should be the same as calling Read() twice when the first one didn't
complete. Yes, it's enforced by each implementation. I don't think we should
enforce any restriction on the interface level, because some implementations
Read() and ReadIfReady() can be used together, and some (e.g.
FakeBlockingStreamSocket) can't.

+  // implementation returns ERR_READ_IF_READY_NOT_IMPLEMENTED. Caller should
+  // fall back to Read() if receives ERR_READ_IF_READY_NOT_IMPLEMENTED.
+  // Upon synchronous completion, returns the number of bytes read or an error
+  // code if an error happens; If read cannot be completed synchronously,

[Bence, 2017/03/03 16:33:40]

Optional: replace every semicolon with full stop to decrease average sentence
length.  This sometimes makes is easier to read.

[xunjieli, 2017/03/03 19:41:05]

Done.

+  // returns ERR_IO_PENDING and does not hold on to |buf|. |callback| will be
+  // invoked with OK when data can be read, at which point, caller can call

[Bence, 2017/03/03 16:33:40]

Suppose ReadIfReady() returns ERR_IO_PENDING, later it calls |callback|, then
caller calls ReadIfReady() again.  Is it guaranteed at that point that it will
return synchronously, or is it allowed to return ERR_IO_PENDING again?  Maybe
spell this out in the comment.

[xunjieli, 2017/03/03 19:41:05]

Nope. it's not guaranteed. It's legal for an implementation to return
ERR_IO_PENDING repeatedly.

+  // ReadIfReady() again; if an error occurs asynchronously, |callback| will be
+  // invoked with the error code.
+  // Note: if 0 is returned synchronously, it means that EOF is reached; if 0/OK

[Bence, 2017/03/03 16:33:40]

Optional: move "if 0 is returned synchronously" into the text above by saying
"Upon synchronous completion, returns the number of bytes read, or 0 on EOF, or
an error code if an error happens.".

[xunjieli, 2017/03/03 19:41:05]

Done.

+  // is returned asynchronously, it means that ReadIfReady() can be retried.

[Bence, 2017/03/03 16:33:40]

Remove "0 is returned asynchronously" case because it is already explicitly
spelled out above.

[xunjieli, 2017/03/03 19:41:05]

Done.

+  virtual int ReadIfReady(IOBuffer* buf,
+                          int buf_len,
+                          const CompletionCallback& callback);
+
   // Writes data, up to |buf_len| bytes, to the socket.  Note: data may be
   // written partially.  The number of bytes written is returned, or an error
   // is returned upon failure.  ERR_SOCKET_NOT_CONNECTED should be returned if