net: FileStream cleanup

net/base/file_stream.h

 // Copyright (c) 2012 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.
 
 // This file defines FileStream, a basic interface for reading and writing files
 // synchronously or asynchronously with support for seeking to an offset.
 // Note that even when used asynchronously, only one operation is supported at
 // a time.
 
 #ifndef NET_BASE_FILE_STREAM_H_
 #define NET_BASE_FILE_STREAM_H_
 
 #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 {
 class FilePath;
 class TaskRunner;
 }
 
 namespace net {
 
 class IOBuffer;
@@ skipping 36 matching lines @@
 
   // Returns true if Open succeeded and Close has not been called.
   virtual bool IsOpen() const;
 
   // Adjust the position from where data is read asynchronously.
   // Upon success, ERR_IO_PENDING is returned and |callback| will be run
   // on the thread where Seek() was called with the the stream position
   // 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
   // asynchronously. Up to buf_len bytes will be copied into buf.  (In
   // other words, partial reads are allowed.)  Returns the number of bytes
   // copied, 0 if at end-of-file, or an error code if the operation could
   // not be performed.
   //
   // The file must be opened with FLAG_ASYNC, and a non-null
   // callback must be passed to this method. If the read could not
   // complete synchronously, then ERR_IO_PENDING is returned, and the
   // callback will be run on the thread where Read() was called, when the
   // read has completed.
   //
   // It is valid to destroy or close the file stream while there is an
   // asynchronous read in progress.  That will cancel the read and allow
   // the buffer to be freed.
   //
   // It is invalid to request any asynchronous operations while there is an
   // in-flight asynchronous operation.
   //
   // This method must not be called if the stream was opened WRITE_ONLY.
   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
   // bytes written, or an error code if the operation could not be
   // performed.
   //
   // The file must be opened with FLAG_ASYNC, and a non-null
   // callback must be passed to this method. If the write could not
   // complete synchronously, then ERR_IO_PENDING is returned, and the
   // callback will be run on the thread where Write() was called when
   // the write has completed.
   //
   // It is valid to destroy or close the file stream while there is an
   // asynchronous write in progress.  That will cancel the write and allow
   // the buffer to be freed.
   //
   // It is invalid to request any asynchronous operations while there is an
   // in-flight asynchronous operation.
   //
   // This method must not be called if the stream was opened READ_ONLY.
   //
   // Zero byte writes are not allowed.
   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
   // calling.
   //
   // The file must be opened with FLAG_ASYNC, and a non-null
   // callback must be passed to this method. If the write could not
   // complete synchronously, then ERR_IO_PENDING is returned, and the
   // callback will be run on the thread where Flush() was called when
   // the write has completed.
@@ skipping 20 matching lines @@
   // without explicitly calling Close, the file should be closed asynchronously
   // without delaying FileStream's destructor.
   scoped_ptr<Context> context_;
 
   DISALLOW_COPY_AND_ASSIGN(FileStream);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_FILE_STREAM_H_