net: FileStream cleanup

net/base/file_stream.h

Index: net/base/file_stream.h
diff --git a/net/base/file_stream.h b/net/base/file_stream.h
index 57011b77decb8103fee14803db14c5e3046df764..90df4e633644a22b7ebd412d3aaa1ffe55d6a483 100644
--- a/net/base/file_stream.h
+++ b/net/base/file_stream.h
@@ -12,7 +12,6 @@
 
 #include "base/files/file.h"
 #include "net/base/completion_callback.h"
-#include "net/base/file_stream_whence.h"
 #include "net/base/net_export.h"
 
 namespace base {
@@ -69,7 +68,7 @@ class NET_EXPORT FileStream {
   // relative to the start of the file.  Otherwise, an error code is returned.
   // It is invalid to request any asynchronous operations while there is an
   // in-flight asynchronous operation.
-  virtual int Seek(Whence whence, int64 offset,
+  virtual int Seek(base::File::Whence whence, int64 offset,
                    const Int64CompletionCallback& callback);
 
   // Call this method to read data from the current stream position
@@ -95,6 +94,11 @@ class NET_EXPORT FileStream {
   virtual int Read(IOBuffer* buf, int buf_len,
                    const CompletionCallback& callback);
 
+  // Same as Read, but delays starting the operation until it should complete
+  // without blocking, reducing the load on the task_runner.
+  virtual int ReadNoBlocking(IOBuffer* buf, int buf_len,

[Sergey Ulanov, 2014/06/12 08:10:21]

Do we really need to differentiate between blocking and non-blocking Read()? For
the caller they are essentially the same. Can FileStream set O_NONBLOCK for the
file and always read in non-blocking mode, same as TCPSocketLibevent works for
sockets?

[rvargas (doing something else), 2014/06/13 02:49:10]

Unfortunately, I think that decision should be taken by the caller. I think the
caller is the only one that knows what it is trying to access and if it makes
sense to do blocking or non blocking IO (or if it cares or not about blocking a
worker thread for a while). TCPSocket, on the other side, has a clear knowledge
of what it is trying to access, and in fact doesn't even use worker threads.

A good example is NativeMessagingTest.SingleSendMessageRead. It uses a plain
file and the non-blocking call just sits there forever waiting to be signaled.

I'm expanding the comment here.

[Sergey Ulanov, 2014/06/13 19:31:50]

Read() can be made non-blocking and such that it will work correctly for both
regular files and pipes. It can work as follows:
 1. Set O_NONBLOCK flag when FileStream is created.
 2. When Read() is called call read(). If EAGAIN or EWOULDBLOCK is returned then
add a libevents watcher for the file.
 3. When the watcher is fired call read() again.

This approach will work correctly for pipes and regular files. It will also work
without a background thread, but it's still better to call read() on a
background thread, because when data comes from the disk OS will still block to
load the data from the disk, even with O_NONBLOCK.
Also note that in this CL ReadNoBlocking() doesn't ensure that O_NONBLOCK is
set, so read() call may still block the calling thread. What we had for
NativeMessaging code was a hack, not a proper solution. You wouldn't need
DontWaitToReadForTesting() with the approach I described above.

[rvargas (doing something else), 2014/06/13 19:58:24]

I'm not really trying to change the way FileStream works for all consumers, as
I'm not really convinced that we'd want to do that, or that we want to enforce
that any handle given to a FileStream has O_NONBLOCK (we accept external handles
that we don't fully control, and as far as I can tell, the documentation for
O_NONBLOCK says that the behavior is actually device dependent)

Granted, I'm just moving code from NativeMessaging to a lower level, but if we
believe that it is either not worth it (/ confusing / error prone)or not clean
enough to live in net, I'm more than happy to just remove the code from
NativeMessaging and let it do what all other consumers do.

[Sergey Ulanov, 2014/06/14 00:58:22]

FileStream owns the file descriptors and it doesn't provide any way to get these
descriptors out, except for tests (see GetFileForTesting()), so I think if
FileStream sets O_NONBLOCK flag it shouldn't matter anywhere outside
implementation of this class.

My problem with Read() is that it makes it easy easy to add new code that
consumes background threads for no good reason.

[rvargas (doing something else), 2014/06/14 01:42:55]

FileStream can receive a file on construction, and that file can come from
anywhere (besides, even when we receive a path we don't really know what that
is). Webkit/FileApi is one such source of handles.

Maybe this is not the right doc, but
http://pubs.opengroup.org/onlinepubs/7908799/xsh/open.html states that for
something other than pipes, what O_NONBLOCK does is not specified (after open).

The last detail (and the most important for me) is that when I've heard people
way more familiar with POSIX than me talk about how should "async" IO be
performed for random files, their opinion has consistently be to do blocking IO
on a worker thread, hence the current implementation of FileStream.

+                             const CompletionCallback& callback);
+
   // Call this method to write data at the current stream position
   // asynchronously.  Up to buf_len bytes will be written from buf. (In
   // other words, partial writes are allowed.)  Returns the number of
@@ -120,6 +124,11 @@ class NET_EXPORT FileStream {
   virtual int Write(IOBuffer* buf, int buf_len,
                     const CompletionCallback& callback);
 
+  // Same as Write, but delays starting the operation until it should complete
+  // without blocking, reducing the load on the task_runner.
+  virtual int WriteNoBlocking(IOBuffer* buf, int buf_len,
+                              const CompletionCallback& callback);
+
   // Forces out a filesystem sync on this file to make sure that the file was
   // written out to disk and is not currently sitting in the buffer. This does
   // not have to be called, it just forces one to happen at the time of