net: FileStream cleanup

net: FileStream cleanup

Remove old "Async" prefix from a time when FileStream could perform sync
operations, and use base::File methods instead of reimplement them.

Remove a TODO from native_message_process_host given that we decided it is
not worth moving that functionality to FileStream ATM.

BUG=322664
TBR=sergeyu@chromium.org, kinuko@chromium.org
R=hashimoto@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278857

[rvargas (doing something else), 2014-06-12 01:47:20]

PTAL

sergeyu -> extensions
hashimoto -> everything

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

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h
File net/base/file_stream.h (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h#...
net/base/file_stream.h:99: virtual int ReadNoBlocking(IOBuffer* buf, int
buf_len,
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?

[hashimoto, 2014-06-12 13:03:37]

https://codereview.chromium.org/323683002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/323683002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.h:13:
#include "base/message_loop/message_loop.h"
nit: No need to include this?

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
File net/base/file_stream_context.h (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context.h:166: void OnFileOpened() {}
nit: This can be put outside of #if? You can put an empty function body in
file_stream_context_posix.cc

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context.h:194: scoped_refptr<IOBuffer> buf_;
nit: How about renaming this to |in_flight_buf_| to align with WIN
implementation and put outside of #if?

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context.h:196: CompletionCallback callback_;
nit: This can be put outside of #if?

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
File net/base/file_stream_context_posix.cc (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:27: orphaned_(false),
nit: Please don't leave buf_len_ uninitialized.

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:73: base::MessageLoopForIO::WATCH_READ,
&file_watcher_, this);
I'm quite unfamiliar with libevent nor details of message pump.
Isn't there any case where something goes wrong (e.g. file get deleted, disk
gets removed physically) and we end up waiting for the event forever?

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_un...
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_un...
net/base/file_stream_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
Please add tests for the new methods.

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_un...
net/base/file_stream_unittest.cc:71: TEST_F(FileStreamTest,
AsyncOpenExplicitClose) {
nit: Could you remove "Async" from this file too?

[rvargas (doing something else), 2014-06-13 02:49:11]

Thanks

https://codereview.chromium.org/323683002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/323683002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.h:13:
#include "base/message_loop/message_loop.h"
On 2014/06/12 13:03:36, hashimoto wrote:
> nit: No need to include this?

Done.

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h
File net/base/file_stream.h (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h#...
net/base/file_stream.h:99: virtual int ReadNoBlocking(IOBuffer* buf, int
buf_len,
On 2014/06/12 08:10:21, Sergey Ulanov wrote:
> 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?

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.

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
File net/base/file_stream_context.h (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context.h:166: void OnFileOpened() {}
On 2014/06/12 13:03:36, hashimoto wrote:
> nit: This can be put outside of #if? You can put an empty function body in
> file_stream_context_posix.cc

Done.

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context.h:194: scoped_refptr<IOBuffer> buf_;
On 2014/06/12 13:03:36, hashimoto wrote:
> nit: How about renaming this to |in_flight_buf_| to align with WIN
> implementation and put outside of #if?

I didn't even look at the win members!. done.

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context.h:196: CompletionCallback callback_;
On 2014/06/12 13:03:37, hashimoto wrote:
> nit: This can be put outside of #if?

Done.

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
File net/base/file_stream_context_posix.cc (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:27: orphaned_(false),
On 2014/06/12 13:03:37, hashimoto wrote:
> nit: Please don't leave buf_len_ uninitialized.

Done.

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:73: base::MessageLoopForIO::WATCH_READ,
&file_watcher_, this);
On 2014/06/12 13:03:37, hashimoto wrote:
> I'm quite unfamiliar with libevent nor details of message pump.
> Isn't there any case where something goes wrong (e.g. file get deleted, disk
> gets removed physically) and we end up waiting for the event forever?

Good point.

I don't know if there is something to do in the case of critical failures. I
know that we can wait forever if this is used for a regular file. I know some
types of fd will just trigger the notification on "critical" failures, but I
suspect that some devices won't do that.

I added code to allow proper cleanup when the consumer gets tired of waiting (or
detect a failure by some other means).

I guess it is up to the caller to decide if it is safe to use non-blocking io.

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_un...
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_un...
net/base/file_stream_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2014/06/12 13:03:37, hashimoto wrote:
> Please add tests for the new methods.

Done.

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_un...
net/base/file_stream_unittest.cc:71: TEST_F(FileStreamTest,
AsyncOpenExplicitClose) {
On 2014/06/12 13:03:37, hashimoto wrote:
> nit: Could you remove "Async" from this file too?

Done.

[hashimoto, 2014-06-13 06:10:42]

lgtm

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
File net/base/file_stream_context_posix.cc (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:73: base::MessageLoopForIO::WATCH_READ,
&file_watcher_, this);
On 2014/06/13 02:49:10, rvargas wrote:
> On 2014/06/12 13:03:37, hashimoto wrote:
> > I'm quite unfamiliar with libevent nor details of message pump.
> > Isn't there any case where something goes wrong (e.g. file get deleted, disk
> > gets removed physically) and we end up waiting for the event forever?
> 
> Good point.
> 
> I don't know if there is something to do in the case of critical failures. I
> know that we can wait forever if this is used for a regular file. I know some
> types of fd will just trigger the notification on "critical" failures, but I
> suspect that some devices won't do that.
> 
> I added code to allow proper cleanup when the consumer gets tired of waiting
(or
> detect a failure by some other means).
> 
> I guess it is up to the caller to decide if it is safe to use non-blocking io.

Makes sense.
Hope the comment in file_stream.h prevents users from carelessly choosing the
non-blocking variant when inappropriate.

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_co...
File net/base/file_stream_context_posix.cc (right):

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:29: in_flight_buf_(0) {
nit: You meant to initialize buf_len_? No need to initialize in_flight_buf_ with
0(=NULL) as it's initialized by its own ctor.

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_un...
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_un...
net/base/file_stream_unittest.cc:217: int fds[2], rv;
Oh, I wasn't aware that libevent does nothing for regular files.
Maybe it's worth noting on file_stream.h that the non-blocking methods cannot
work with regular files on POSIX.

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_un...
net/base/file_stream_unittest.cc:879: // Verify that async Write() errors are
mapped correctly.
nit: "async" still mentioned here.

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_un...
net/base/file_stream_unittest.cc:904: // Verify that async Read() errors are
mapped correctly.
ditto.

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

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h
File net/base/file_stream.h (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h#...
net/base/file_stream.h:99: virtual int ReadNoBlocking(IOBuffer* buf, int
buf_len,
On 2014/06/13 02:49:10, rvargas wrote:
> On 2014/06/12 08:10:21, Sergey Ulanov wrote:
> > 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?
> 
> 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.

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.


> 
> I'm expanding the comment here.

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

Thanks!

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h
File net/base/file_stream.h (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h#...
net/base/file_stream.h:99: virtual int ReadNoBlocking(IOBuffer* buf, int
buf_len,
On 2014/06/13 19:31:50, Sergey Ulanov wrote:
> On 2014/06/13 02:49:10, rvargas wrote:
> > On 2014/06/12 08:10:21, Sergey Ulanov wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

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.


> 
> 
> > 
> > I'm expanding the comment here.
>

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_co...
File net/base/file_stream_context_posix.cc (right):

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:29: in_flight_buf_(0) {
On 2014/06/13 06:10:42, hashimoto wrote:
> nit: You meant to initialize buf_len_? No need to initialize in_flight_buf_
with
> 0(=NULL) as it's initialized by its own ctor.

yes I meant it to be the len :(

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_un...
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_un...
net/base/file_stream_unittest.cc:217: int fds[2], rv;
On 2014/06/13 06:10:42, hashimoto wrote:
> Oh, I wasn't aware that libevent does nothing for regular files.
> Maybe it's worth noting on file_stream.h that the non-blocking methods cannot
> work with regular files on POSIX.

Expanded the comment.

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_un...
net/base/file_stream_unittest.cc:879: // Verify that async Write() errors are
mapped correctly.
On 2014/06/13 06:10:42, hashimoto wrote:
> nit: "async" still mentioned here.

Done.

https://codereview.chromium.org/323683002/diff/300001/net/base/file_stream_un...
net/base/file_stream_unittest.cc:904: // Verify that async Read() errors are
mapped correctly.
On 2014/06/13 06:10:42, hashimoto wrote:
> ditto.

Done.

[rvargas (doing something else), 2014-06-13 21:12:57]

+wtc to get another opinion about if it makes sense to add these methods to net
or not.

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

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h
File net/base/file_stream.h (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h#...
net/base/file_stream.h:99: virtual int ReadNoBlocking(IOBuffer* buf, int
buf_len,
On 2014/06/13 19:58:24, rvargas wrote:
> 
> 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)

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.

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

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

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h
File net/base/file_stream.h (right):

https://codereview.chromium.org/323683002/diff/220001/net/base/file_stream.h#...
net/base/file_stream.h:99: virtual int ReadNoBlocking(IOBuffer* buf, int
buf_len,
On 2014/06/14 00:58:22, Sergey Ulanov wrote:
> On 2014/06/13 19:58:24, rvargas wrote:
> > 
> > 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)
> 
> 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.
> 
> > 
> > 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.
> > 

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.

[wtc, 2014-06-16 23:01:44]

Review comments on patch set 4:

Ricardo: if I only read the comments for ReadNonBlocking and WriteNonBlocking in
file_stream.h, I would not know whether I should use them or not. Even after I
read the implementations in file_stream_context_posix.cc, I do understand how
the NonBlocking functions work, but I still don't know when they are
preferrable.

https://codereview.chromium.org/323683002/diff/320001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/323683002/diff/320001/base/files/file_posix.c...
base/files/file_posix.cc:284: return lseek(file_.get(),
static_cast<off_t>(offset),

We should assert that sizeof(off_t) == 8. If sizeof(off_t) is 4, this
static_cast<off_t> will truncate |offset|.

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream.cc...
net/base/file_stream.cc:86: // write(..., 0) will return 0, which indicates
end-of-file.

This comment is wrong. A write byte count of 0 does not indicate end-of-file. So
I think this comment and the DCHECK_GT(buf_len, 0) can be removed.

If you remove this, please also remove lines 98-99.

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
File net/base/file_stream_context.h (left):

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
net/base/file_stream_context.h:172: // Flushes all data written to the stream.

Should we preserve this comment in the new code?

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
File net/base/file_stream_context.h (right):

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
net/base/file_stream_context.h:195: int buf_len_;

Document this member. I found that it is used in conjunction with
in_flight_buf_, so we can also just rename it in_flight_buf_len_.

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
File net/base/file_stream_context_posix.cc (right):

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:50: in_flight_buf_ = NULL;

Also reset buf_len_ to 0.

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:87: in_flight_buf_ = NULL;

Also reset buf_len_ to 0.

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:163: void
FileStream::Context::OnFileCanReadWithoutBlocking(int fd) {

IMPORTANT: are OnFileCanReadWithoutBlocking and OnFileCanWriteWithoutBlocking
used? I can't figure out how they are invoked.

[rvargas (doing something else), 2014-06-16 23:41:44]

Thanks Wan-Teh,

I think the question that I'm pondering is, should we add the new methods at all
or not?

Part of me tells me that no matter what amount of comments we add to the
description, it will not be clear enough what the advantage is in any case (or
whether it is safe to use it). For example, just the name of ReadNonBlocking may
suggest that it implements a more traditional non-blocking read pattern, or that
it does all the work to make that happen.

As you said, even if we spell out the actual implementation on the header (a
smell by itself), things may not be clear enough and it may lead to confusion
and incorrect usage.

I'm actually leaning towards dropping this.

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
File net/base/file_stream_context_posix.cc (right):

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:163: void
FileStream::Context::OnFileCanReadWithoutBlocking(int fd) {
On 2014/06/16 23:01:44, wtc wrote:
> 
> IMPORTANT: are OnFileCanReadWithoutBlocking and OnFileCanWriteWithoutBlocking
> used? I can't figure out how they are invoked.

These methods are the implementation of the MessageLoopForIO::Watcher interface.
They are invoked by the message loop when the appropriate libevent event fires.

[wtc, 2014-06-17 18:05:26]

On 2014/06/16 23:41:44, rvargas wrote:
> 
> I think the question that I'm pondering is, should we add the new methods at
all
> or not?
> 
> Part of me tells me that no matter what amount of comments we add to the
> description, it will not be clear enough what the advantage is in any case (or
> whether it is safe to use it). For example, just the name of ReadNonBlocking
may
> suggest that it implements a more traditional non-blocking read pattern, or
that
> it does all the work to make that happen.
> 
> As you said, even if we spell out the actual implementation on the header (a
> smell by itself), things may not be clear enough and it may lead to confusion
> and incorrect usage.
> 
> I'm actually leaning towards dropping this.

When I reviewed patch set 4, I didn't realize the FileStream class is also used
on a pipe (for IPC). I thought the NonBlocking variants of Read and Write you
added were intended to handle regular files on a network filesystem better (as
opposed to local regular files).

With this new info, I think we can also add a PipeStream class for pipes.
Alternatively, add a setter method for FileStream or a new parameter for the
second FileStream constructor that says the base::File imported into the
FileStream is a pipe. Then the Read and Write methods will do the right thing
automatically.

[rvargas (doing something else), 2014-06-17 19:16:12]

On 2014/06/17 18:05:26, wtc wrote:
> On 2014/06/16 23:41:44, rvargas wrote:
> > 
> > I think the question that I'm pondering is, should we add the new methods at
> all
> > or not?
> > 
> > Part of me tells me that no matter what amount of comments we add to the
> > description, it will not be clear enough what the advantage is in any case
(or
> > whether it is safe to use it). For example, just the name of ReadNonBlocking
> may
> > suggest that it implements a more traditional non-blocking read pattern, or
> that
> > it does all the work to make that happen.
> > 
> > As you said, even if we spell out the actual implementation on the header (a
> > smell by itself), things may not be clear enough and it may lead to
confusion
> > and incorrect usage.
> > 
> > I'm actually leaning towards dropping this.
> 
> When I reviewed patch set 4, I didn't realize the FileStream class is also
used
> on a pipe (for IPC). I thought the NonBlocking variants of Read and Write you
> added were intended to handle regular files on a network filesystem better (as
> opposed to local regular files).
> 
> With this new info, I think we can also add a PipeStream class for pipes.
> Alternatively, add a setter method for FileStream or a new parameter for the
> second FileStream constructor that says the base::File imported into the
> FileStream is a pipe. Then the Read and Write methods will do the right thing
> automatically.

Having a class that wraps a pipe seems like something completely reasonable, but
I'm not sure net is the place to have such a class. I see net utility functions
as something that is needed directly by the net code, or something that is
tightly coupled to network functionality. FileStream fits the first criteria,
but PipeStream doesn't fit either one.

On the other hand, if we implement a wrapper for efficient pipe handling geared
towards IPC, we should end up with ipc_channel_posix & co. If we say that it's
not specific to IPC, we probably would end up duplicating a lot of the code of
those files. If we say that it's more general purpose and maybe not that
efficient, we should get to base/file, base/file_proxy or net/file_stream, which
is where we are today (in the sense that FileStream works, but it may burn a
worker thread if the other side is not responsive... if the other side of the
pipe is responsive, that should not be an issue).

We could have some pipe-specific methods in FileStream. The reason I was
somewhat avoiding that route is that there's a lot of possible types of fd. It
could be a local file, or a remote file, or a pipe, or a socket, or a serial
port etc. It felt a little weird to single out pipe for the wait before read
case, because plain Read works, and the new Read would also work for something
other than pipes.

I also think that a single use case is probably not enough to attempt to
generalize the code and have a new base facility. Makes sense?

[wtc, 2014-06-17 22:50:28]

Ricardo: I agree with your assessment.

[rvargas (doing something else), 2014-06-18 01:36:38]

Thanks.

I removed the new methods so this is now doing only the cleanup.

https://codereview.chromium.org/323683002/diff/320001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/323683002/diff/320001/base/files/file_posix.c...
base/files/file_posix.cc:284: return lseek(file_.get(),
static_cast<off_t>(offset),
On 2014/06/16 23:01:44, wtc wrote:
> 
> We should assert that sizeof(off_t) == 8. If sizeof(off_t) is 4, this
> static_cast<off_t> will truncate |offset|.

Done.

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream.cc...
net/base/file_stream.cc:86: // write(..., 0) will return 0, which indicates
end-of-file.
On 2014/06/16 23:01:44, wtc wrote:
> 
> This comment is wrong. A write byte count of 0 does not indicate end-of-file.
So
> I think this comment and the DCHECK_GT(buf_len, 0) can be removed.
> 
> If you remove this, please also remove lines 98-99.

Done.

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
File net/base/file_stream_context.h (left):

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
net/base/file_stream_context.h:172: // Flushes all data written to the stream.
On 2014/06/16 23:01:44, wtc wrote:
> 
> Should we preserve this comment in the new code?

Looks kind of an empty comment to me, especially given that this is a private
method and is now declared among other self descriptive methods.

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
File net/base/file_stream_context.h (right):

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
net/base/file_stream_context.h:195: int buf_len_;
On 2014/06/16 23:01:44, wtc wrote:
> 
> Document this member. I found that it is used in conjunction with
> in_flight_buf_, so we can also just rename it in_flight_buf_len_.

removed

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
File net/base/file_stream_context_posix.cc (right):

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:50: in_flight_buf_ = NULL;
On 2014/06/16 23:01:44, wtc wrote:
> 
> Also reset buf_len_ to 0.

removed

https://codereview.chromium.org/323683002/diff/320001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:87: in_flight_buf_ = NULL;
On 2014/06/16 23:01:44, wtc wrote:
> 
> Also reset buf_len_ to 0.

removed

[Sergey Ulanov, 2014-06-18 04:03:38]

Native Messaging changes - LGTM

[rvargas (doing something else), 2014-06-19 00:09:50]

On 2014/06/18 04:03:38, Sergey Ulanov wrote:
> Native Messaging changes - LGTM

Thanks.

hashimoto@, does it look good still?

[hashimoto, 2014-06-19 01:51:56]

slgtm

https://codereview.chromium.org/323683002/diff/340001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/323683002/diff/340001/base/files/file_posix.c...
base/files/file_posix.cc:289: COMPILE_ASSERT(sizeof(int64) == sizeof(off_t),
off_t_64_bit);
nit: s/off_t_64_bit/off64_t_bit/?

https://codereview.chromium.org/323683002/diff/340001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

https://codereview.chromium.org/323683002/diff/340001/net/base/file_stream.cc...
net/base/file_stream.cc:74: return context_->Write(buf, buf_len, callback);
nit: Why did you remove DCHECK here while keeping the one in Read()?

[rvargas (doing something else), 2014-06-19 17:11:43]

Thanks!

https://codereview.chromium.org/323683002/diff/340001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/323683002/diff/340001/base/files/file_posix.c...
base/files/file_posix.cc:289: COMPILE_ASSERT(sizeof(int64) == sizeof(off_t),
off_t_64_bit);
On 2014/06/19 01:51:56, hashimoto wrote:
> nit: s/off_t_64_bit/off64_t_bit/?

Actually, the first message should be off64_t_64_bit instead.

https://codereview.chromium.org/323683002/diff/340001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

https://codereview.chromium.org/323683002/diff/340001/net/base/file_stream.cc...
net/base/file_stream.cc:74: return context_->Write(buf, buf_len, callback);
On 2014/06/19 01:51:56, hashimoto wrote:
> nit: Why did you remove DCHECK here while keeping the one in Read()?

It was suggested by wtc, given that there is no EOF and writing 0 is valid.
Thinking about it a little more, we can still DCHECK no negative values so I
just did that instead.

[rvargas (doing something else), 2014-06-19 17:11:57]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-06-19 17:13:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/323683002/360001

[wtc, 2014-06-19 18:20:49]

Patch set 6 LGTM.

https://codereview.chromium.org/323683002/diff/360001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/323683002/diff/360001/base/files/file_posix.c...
base/files/file_posix.cc:286: return lseek64(file_.get(),
static_cast<off64_t>(offset),

The COMPILE_ASSERT and the static_cast<off64_t> should be unnecessary in this
case, because by definition off64_t is 64 bits. But it's fine to have them for
symmetry with the other case.

https://codereview.chromium.org/323683002/diff/360001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/323683002/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:107:
read_file_(-1),

Nit: the original code (base::kInvalidPlatformFileValue) seems fine to me.

https://codereview.chromium.org/323683002/diff/360001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/323683002/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.h:11:
#include "base/files/file.h"

Nit: we don't need this header now because there is no other change to this
file, other than the removal of a TODO comment.

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
net/base/file_stream_context.cc:82: CancelIo(file_.GetPlatformFile());

Hmm... how could the original code be compiled on non-Windows platforms before?!
CancelIo is a Win32 function.

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
File net/base/file_stream_context_posix.cc (left):

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:6: #define _FILE_OFFSET_BITS 64

Should we move this to base/files/file_posix.cc?

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:54: // TODO(hashimoto): Check that file_ is
async.

Should we remove this TODO comment? It makes more sense with the original code
(OnAsyncFileOpened).

[rvargas (doing something else), 2014-06-19 18:46:43]

Thanks!

https://codereview.chromium.org/323683002/diff/360001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/323683002/diff/360001/base/files/file_posix.c...
base/files/file_posix.cc:286: return lseek64(file_.get(),
static_cast<off64_t>(offset),
On 2014/06/19 18:20:49, wtc wrote:
> 
> The COMPILE_ASSERT and the static_cast<off64_t> should be unnecessary in this
> case, because by definition off64_t is 64 bits. But it's fine to have them for
> symmetry with the other case.

That was exactly my thought :)

https://codereview.chromium.org/323683002/diff/360001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.cc
(right):

https://codereview.chromium.org/323683002/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.cc:107:
read_file_(-1),
On 2014/06/19 18:20:49, wtc wrote:
> 
> Nit: the original code (base::kInvalidPlatformFileValue) seems fine to me.

Unfortunately, my next patch will completely remove platform_file.h, so
base::kInvalidPlatformFileValue will be undefined. After I remove that file,
base::PlatformFile is only defined by file.h, but not to keep variables of that
type (which mean initializations, checks for validity etc) but to use the
underlying handle of a base::File on a lower level call.

That's the detail about this code that I was trying to fix: for POSIX, it stores
a file descriptor that is owned by another object. What makes it acceptable is
that the owner of that file is a FileStream that this class also owns.

https://codereview.chromium.org/323683002/diff/360001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/323683002/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/native_message_process_host.h:11:
#include "base/files/file.h"
On 2014/06/19 18:20:49, wtc wrote:
> 
> Nit: we don't need this header now because there is no other change to this
> file, other than the removal of a TODO comment.

It is needed to define PlatformFile for read_file_

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
net/base/file_stream_context.cc:82: CancelIo(file_.GetPlatformFile());
On 2014/06/19 18:20:49, wtc wrote:
> 
> Hmm... how could the original code be compiled on non-Windows platforms
before?!
> CancelIo is a Win32 function.

There was a void method with that name defined on the header under an #ifdef.

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
File net/base/file_stream_context_posix.cc (left):

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
net/base/file_stream_context_posix.cc:6: #define _FILE_OFFSET_BITS 64
On 2014/06/19 18:20:49, wtc wrote:
> 
> Should we move this to base/files/file_posix.cc?

I thought about it, but file_posix has a lot of other code that this define
would end up affecting and I don't know if that would be OK for all platforms
(for example, it does nothing for Android). That's why I decided to be explicit
about calling lstat64 directly, which is more in line with what other methods on
file_posix.cc do.

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/323683002/diff/360001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:54: // TODO(hashimoto): Check that file_ is
async.
On 2014/06/19 18:20:49, wtc wrote:
> 
> Should we remove this TODO comment? It makes more sense with the original code
> (OnAsyncFileOpened).

I thought about it, but this is really related to what flags were used to create
the file that we are receiving, and not to the presence of a synchronous
implementation in this class.

I think users are still divided, with some of them adding FLAG_ASYNC only for
Windows and others for any platform.

[commit-bot: I haz the power, 2014-06-19 20:57:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 20:57:31]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[rvargas (doing something else), 2014-06-20 00:28:19]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-06-20 00:30:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/323683002/360001

[commit-bot: I haz the power, 2014-06-20 01:21:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-20 01:30:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 01:30:54]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[rvargas (doing something else), 2014-06-20 17:59:06]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-06-20 18:01:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/323683002/360001

[commit-bot: I haz the power, 2014-06-20 18:19:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-20 18:33:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 18:33:17]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[rvargas (doing something else), 2014-06-20 22:12:20]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-06-20 22:14:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/323683002/360001

[commit-bot: I haz the power, 2014-06-20 23:30:23]

Change committed as 278857