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

device/serial/serial_io_handler_posix.cc

 // Copyright 2014 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.
 
 #include "device/serial/serial_io_handler_posix.h"
 
 #include <sys/ioctl.h>
 #include <termios.h>
 
 #include "base/posix/eintr_wrapper.h"
@@ skipping 106 matching lines @@
   return new SerialIoHandlerPosix(file_thread_task_runner,
                                   ui_thread_task_runner);
 }
 
 void SerialIoHandlerPosix::ReadImpl() {
   DCHECK(CalledOnValidThread());
   DCHECK(pending_read_buffer());
   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() {
   DCHECK(CalledOnValidThread());
   DCHECK(pending_write_buffer());
   DCHECK(file().IsValid());
 
   EnsureWatchingWrites();
 }
 
@@ skipping 150 matching lines @@
       is_watching_writes_(false) {
 }
 
 SerialIoHandlerPosix::~SerialIoHandlerPosix() {
 }
 
 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;
       int new_bytes_read =
           CheckReceiveError(pending_read_buffer(), pending_read_buffer_len(),
                             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 {
     // Stop watching the fd if we get notifications with no pending
     // reads or writes to avoid starving the message loop.
     is_watching_reads_ = false;
     file_read_watcher_.StopWatchingFileDescriptor();
   }
 }
 
+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());
 
   if (pending_write_buffer()) {
     int bytes_written = HANDLE_EINTR(write(file().GetPlatformFile(),
                                            pending_write_buffer(),
                                            pending_write_buffer_len()));
     if (bytes_written < 0) {
       WriteCompleted(0, serial::SendError::SYSTEM_ERROR);
@@ skipping 270 matching lines @@
   memcpy(buffer, chars_stashed_, std::min(new_bytes_read, 2));
   memcpy(chars_stashed_, tmp, num_chars_stashed_);
   return new_bytes_read;
 }
 
 std::string SerialIoHandler::MaybeFixUpPortName(const std::string& port_name) {
   return port_name;
 }
 
 }  // namespace device