[serial] When a read is requested on posix platforms, try the read immediately.

[serial] When a read is requested on posix platforms, try the read immediately.

On some posix platforms (e.g., OSX) the first read after a serial device is
opened may never complete. The problem is on these platforms the message loop
will not notify that a file descriptor is ready to read if it was in that state
when it was opened.

This fixed this problem by trying to perform a read immediately after it is requested.

BUG=618911
Committed: https://crrev.com/b09276a67a58e6aa0c23d8a052f0b7bd4c9ca668
Cr-Commit-Position: refs/heads/master@{#408467}

[aschulman, 2016-06-10 03:35:01]

Description was changed from

==========
[serial] When a read is requested on posix platforms, try the read immediately.

On some posix platforms (e.g., OSX) the first read after a serial device is
opened may never complete. The problem is on these platforms the message loop
will not notify that a file descriptor is ready to read if it was in that state
when it was opened.

This fixed this problem by trying to perform a read immediately after it is
requested.

BUG=618911
==========

to

==========
[serial] When a read is requested on posix platforms, try the read immediately.

On some posix platforms (e.g., OSX) the first read after a serial device is
opened may never complete. The problem is on these platforms the message loop
will not notify that a file descriptor is ready to read if it was in that state
when it was opened.

This fixed this problem by trying to perform a read immediately after it is
requested.

BUG=618911
==========

[aschulman, 2016-06-10 03:35:02]

aschulman@chromium.org changed reviewers:
+ reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-06-10 17:48:37]

An annoying problem you're going to have with this patch is that by trying to
read immediately in some cases the read callback will be called from inside a
call to Read(). This could cause unexpected reentrancy in the caller's code. To
avoid this you should post the call to ReadCompleted to the IO thread's message
loop after canceling the the file descriptor watcher. Preferably only when you
have to.

[aschulman, 2016-07-27 06:24:32]

On 2016/06/10 17:48:37, Reilly Grant wrote:
> An annoying problem you're going to have with this patch is that by trying to
> read immediately in some cases the read callback will be called from inside a
> call to Read(). This could cause unexpected reentrancy in the caller's code.
To
> avoid this you should post the call to ReadCompleted to the IO thread's
message
> loop after canceling the the file descriptor watcher. Preferably only when you
> have to.

I finally got back around to working on this patch and Patch Set #2 is an
attempt at addressing your comments. However,
it is likely that my changes are not in ideal Chromium style.

[Reilly Grant (use Gerrit), 2016-07-27 21:23:39]

lgtm with a nit

https://codereview.chromium.org/2057973002/diff/20001/device/serial/serial_io...
File device/serial/serial_io_handler_posix.cc (right):

https://codereview.chromium.org/2057973002/diff/20001/device/serial/serial_io...
device/serial/serial_io_handler_posix.cc:352: // Stop watching the fd because a
read completed within the read call.
This isn't necessary. The code is optimized to assume that a new read call will
be made before the FD is ready again and there's no reason to not do that here
as well.

[aschulman, 2016-07-28 03:11:00]

https://codereview.chromium.org/2057973002/diff/20001/device/serial/serial_io...
File device/serial/serial_io_handler_posix.cc (right):

https://codereview.chromium.org/2057973002/diff/20001/device/serial/serial_io...
device/serial/serial_io_handler_posix.cc:352: // Stop watching the fd because a
read completed within the read call.
On 2016/07/27 21:23:38, Reilly Grant wrote:
> This isn't necessary. The code is optimized to assume that a new read call
will
> be made before the FD is ready again and there's no reason to not do that here
> as well.

I think I may be confused. In your prior comment you mentioned that I should
make sure that the fd watch is canceled if the read attempt succeeds within the
read call. This made sense to me because otherwise the user will call Read() and
there can be a ReadCompleted() after it due to a read attempt that succeeded
within the Read() and even if the user does not call read again, another
ReadCompleted() could be called when more data comes in and the fd watcher sees
that the fd has data again. In short, my understanding was that for each Read()
call there should only be one ReadCompleted() that could ever happen, is this
true?

[Reilly Grant (use Gerrit), 2016-07-28 18:01:19]

On 2016/07/28 at 03:11:00, aschulman wrote:
>
https://codereview.chromium.org/2057973002/diff/20001/device/serial/serial_io...
> File device/serial/serial_io_handler_posix.cc (right):
> 
>
https://codereview.chromium.org/2057973002/diff/20001/device/serial/serial_io...
> device/serial/serial_io_handler_posix.cc:352: // Stop watching the fd because
a read completed within the read call.
> On 2016/07/27 21:23:38, Reilly Grant wrote:
> > This isn't necessary. The code is optimized to assume that a new read call
will
> > be made before the FD is ready again and there's no reason to not do that
here
> > as well.
> 
> I think I may be confused. In your prior comment you mentioned that I should
make sure that the fd watch is canceled if the read attempt succeeds within the
read call. This made sense to me because otherwise the user will call Read() and
there can be a ReadCompleted() after it due to a read attempt that succeeded
within the Read() and even if the user does not call read again, another
ReadCompleted() could be called when more data comes in and the fd watcher sees
that the fd has data again. In short, my understanding was that for each Read()
call there should only be one ReadCompleted() that could ever happen, is this
true?

The code already ensures that read callbacks will only be called once. That's
why OnFileCanReadWithoutBlocking() always checks if it has a pending read buffer
before continuing and cancels the FD watch if it doesn't. What I was concerned
about in my original patch was re-entrancy caused by calling the read callback
from within a call to Read(). You've fixed this by calling QueueReadCallback()
in that case.

[aschulman, 2016-07-28 19:48:22]

The CQ bit was checked by aschulman@chromium.org

[aschulman, 2016-07-28 19:48:23]

The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2057973002/#ps40001
(title: "Removed unnecessary watch cancel.")

[commit-bot: I haz the power, 2016-07-28 19:48:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 20:39:33]

Description was changed from

==========
[serial] When a read is requested on posix platforms, try the read immediately.

On some posix platforms (e.g., OSX) the first read after a serial device is
opened may never complete. The problem is on these platforms the message loop
will not notify that a file descriptor is ready to read if it was in that state
when it was opened.

This fixed this problem by trying to perform a read immediately after it is
requested.

BUG=618911
==========

to

==========
[serial] When a read is requested on posix platforms, try the read immediately.

On some posix platforms (e.g., OSX) the first read after a serial device is
opened may never complete. The problem is on these platforms the message loop
will not notify that a file descriptor is ready to read if it was in that state
when it was opened.

This fixed this problem by trying to perform a read immediately after it is
requested.

BUG=618911
==========

[commit-bot: I haz the power, 2016-07-28 20:39:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-28 20:41:51]

Description was changed from

==========
[serial] When a read is requested on posix platforms, try the read immediately.

On some posix platforms (e.g., OSX) the first read after a serial device is
opened may never complete. The problem is on these platforms the message loop
will not notify that a file descriptor is ready to read if it was in that state
when it was opened.

This fixed this problem by trying to perform a read immediately after it is
requested.

BUG=618911
==========

to

==========
[serial] When a read is requested on posix platforms, try the read immediately.

On some posix platforms (e.g., OSX) the first read after a serial device is
opened may never complete. The problem is on these platforms the message loop
will not notify that a file descriptor is ready to read if it was in that state
when it was opened.

This fixed this problem by trying to perform a read immediately after it is
requested.

BUG=618911

Committed: https://crrev.com/b09276a67a58e6aa0c23d8a052f0b7bd4c9ca668
Cr-Commit-Position: refs/heads/master@{#408467}
==========

[commit-bot: I haz the power, 2016-07-28 20:41:52]

Patchset 3 (id:??) landed as
https://crrev.com/b09276a67a58e6aa0c23d8a052f0b7bd4c9ca668
Cr-Commit-Position: refs/heads/master@{#408467}