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

device/serial/serial_io_handler_posix.cc

Index: device/serial/serial_io_handler_posix.cc
diff --git a/device/serial/serial_io_handler_posix.cc b/device/serial/serial_io_handler_posix.cc
index 158c374a03b0858cc74f94205704e6682ea87026..7a9e4fb63402381765f4b025c35fc8429d51dcef 100644
--- a/device/serial/serial_io_handler_posix.cc
+++ b/device/serial/serial_io_handler_posix.cc
@@ -124,6 +124,12 @@ void SerialIoHandlerPosix::ReadImpl() {
   DCHECK(file().IsValid());
 
   EnsureWatchingReads();
+
+  // Try to read immediately. This is needed because on some platforms
+  // (e.g., OSX) there may not be a notification from the message loop
+  // when the fd is ready to read immediately after it is opened. There
+  // is no danger of blocking because the fd is opened with async flag.
+  AttemptRead(true);
 }
 
 void SerialIoHandlerPosix::WriteImpl() {
@@ -294,18 +300,25 @@ void SerialIoHandlerPosix::OnFileCanReadWithoutBlocking(int fd) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(fd, file().GetPlatformFile());
 
+  AttemptRead(false);
+}
+
+void SerialIoHandlerPosix::AttemptRead(bool within_read) {
   if (pending_read_buffer()) {
     int bytes_read = HANDLE_EINTR(read(file().GetPlatformFile(),
                                        pending_read_buffer(),
                                        pending_read_buffer_len()));
     if (bytes_read < 0) {
-      if (errno == ENXIO) {
-        ReadCompleted(0, serial::ReceiveError::DEVICE_LOST);
+      if (errno == EAGAIN) {
+        // The fd does not have data to read yet so continue waiting.
+        return;
+      } else if (errno == ENXIO) {
+        RunReadCompleted(within_read, 0, serial::ReceiveError::DEVICE_LOST);
       } else {
-        ReadCompleted(0, serial::ReceiveError::SYSTEM_ERROR);
+        RunReadCompleted(within_read, 0, serial::ReceiveError::SYSTEM_ERROR);
       }
     } else if (bytes_read == 0) {
-      ReadCompleted(0, serial::ReceiveError::DEVICE_LOST);
+      RunReadCompleted(within_read, 0, serial::ReceiveError::DEVICE_LOST);
     } else {
       bool break_detected = false;
       bool parity_error_detected = false;
@@ -314,11 +327,14 @@ void SerialIoHandlerPosix::OnFileCanReadWithoutBlocking(int fd) {
                             bytes_read, break_detected, parity_error_detected);
 
       if (break_detected) {
-        ReadCompleted(new_bytes_read, serial::ReceiveError::BREAK);
+        RunReadCompleted(within_read, new_bytes_read,
+                         serial::ReceiveError::BREAK);
       } else if (parity_error_detected) {
-        ReadCompleted(new_bytes_read, serial::ReceiveError::PARITY_ERROR);
+        RunReadCompleted(within_read, new_bytes_read,
+                         serial::ReceiveError::PARITY_ERROR);
       } else {
-        ReadCompleted(new_bytes_read, serial::ReceiveError::NONE);
+        RunReadCompleted(within_read, new_bytes_read,
+                         serial::ReceiveError::NONE);
       }
     }
   } else {
@@ -329,6 +345,20 @@ void SerialIoHandlerPosix::OnFileCanReadWithoutBlocking(int fd) {
   }
 }
 
+void SerialIoHandlerPosix::RunReadCompleted(bool within_read,
+                                            int bytes_read,
+                                            serial::ReceiveError error) {
+  if (within_read) {
+    // Stop watching the fd because a read completed within the read call.

[Reilly Grant (use Gerrit), 2016/07/27 21:23:38]

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]

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?

+    is_watching_reads_ = false;
+    file_read_watcher_.StopWatchingFileDescriptor();
+
+    QueueReadCompleted(bytes_read, error);
+  } else {
+    ReadCompleted(bytes_read, error);
+  }
+}
+
 void SerialIoHandlerPosix::OnFileCanWriteWithoutBlocking(int fd) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(fd, file().GetPlatformFile());