Add SyncSocket::ReceiveWithTimeout() and SyncSocket unit tests.

base/sync_socket_posix.cc

 // 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.
 
 #include "base/sync_socket.h"
 
 #include <errno.h>
 #include <limits.h>
 #include <fcntl.h>
 #include <stdio.h>
 #include <sys/ioctl.h>
 #include <sys/socket.h>
 #include <sys/types.h>
 
 #if defined(OS_SOLARIS)
 #include <sys/filio.h>
 #endif
 
 #include "base/file_util.h"
 #include "base/logging.h"
 
-
 namespace base {
 
 namespace {
 // To avoid users sending negative message lengths to Send/Receive
 // we clamp message lengths, which are size_t, to no more than INT_MAX.
 const size_t kMaxMessageLength = static_cast<size_t>(INT_MAX);
 
 }  // namespace
 
 const SyncSocket::Handle SyncSocket::kInvalidHandle = -1;
@@ skipping 53 matching lines @@
     return false;
   }
   int retval = HANDLE_EINTR(close(handle_));
   if (retval < 0)
     DPLOG(ERROR) << "close";
   handle_ = kInvalidHandle;
   return (retval == 0);
 }
 
 size_t SyncSocket::Send(const void* buffer, size_t length) {
+  DCHECK_GT(length, 0u);
   DCHECK_LE(length, kMaxMessageLength);
   const char* charbuffer = static_cast<const char*>(buffer);
   int len = file_util::WriteFileDescriptor(handle_, charbuffer, length);
 
   return (len == -1) ? 0 : static_cast<size_t>(len);
 }
 
 size_t SyncSocket::Receive(void* buffer, size_t length) {
+  DCHECK_GT(length, 0u);
   DCHECK_LE(length, kMaxMessageLength);
   char* charbuffer = static_cast<char*>(buffer);
   if (file_util::ReadFromFD(handle_, charbuffer, length))
     return length;
   return 0;
 }
 
+size_t SyncSocket::ReceiveWithTimeout(void* buffer,
+                                      size_t length,
+                                      TimeDelta timeout) {

[willchan no longer on Chromium, 2013/10/04 21:01:35]

Please use ThreadRestrictions::AssertIOAllowed();

All these functions really should be using this.

[DaleCurtis, 2013/10/04 21:08:39]

I'll check if that can be done.

[DaleCurtis, 2013/10/04 23:11:32]

Done.

+  DCHECK_GT(length, 0u);
+  DCHECK_LE(length, kMaxMessageLength);
+
+  // Only timeouts greater than zero and less than one second are allowed.
+  DCHECK_GT(timeout.InMicroseconds(), 0);
+  DCHECK(timeout.InMicroseconds() < Time::kMicrosecondsPerSecond);
+
+  // Track the start time so we can reduce the timeout as data is read.
+  TimeTicks start_time = base::TimeTicks::Now();

[willchan no longer on Chromium, 2013/10/04 21:01:35]

drop the base::

[DaleCurtis, 2013/10/04 23:11:32]

Done.

+
+  fd_set rfds;

[willchan no longer on Chromium, 2013/10/04 21:01:35]

Please choose a more descriptive name.

[DaleCurtis, 2013/10/04 23:11:32]

Done.

+  size_t bytes_read_total = 0;
+  char* charbuffer = static_cast<char*>(buffer);
+  do {
+    FD_ZERO(&rfds);
+    FD_SET(handle_, &rfds);

[Nico, 2013/10/04 05:35:12]

You need to check that handle_ is < FD_SETSIZE, else this will clobber memory

[DaleCurtis, 2013/10/04 23:11:32]

Done.

+
+    // Wait for data to become available.
+    struct timeval timeout_struct = { 0, timeout.InMicroseconds() };
+    const int select_result = HANDLE_EINTR(

[willchan no longer on Chromium, 2013/10/04 21:01:35]

I think there's a bug here, right? HANDLE_EINTR() is going to bail out each time
there's an interrupt, which is not going to update timeout_struct.

Can you write a test for this? You can probably use a separate thread and
tkill() this thread to deliver the EINTR.

[DaleCurtis, 2013/10/04 21:08:39]

select() will update the timeout struct on EINTR:

http://linux.die.net/man/2/select

[willchan no longer on Chromium, 2013/10/09 20:06:52]

How about the non-linux posix platforms? What happens on OS X?

[DaleCurtis, 2013/10/09 21:55:40]

Man page is sadly conflicting, saying both that it's not updated and not to rely
on it remaining static. After digging through the darwin open source code, it
looks like all usages end up reinitializing the timeout value on EINTR, so I'll
break out the EINTR handling.

+        select(handle_ + 1, &rfds, NULL, NULL, &timeout_struct));
+    if (select_result <= 0)
+      return bytes_read_total;
+
+    // select() only tells us that data is ready for reading, not how much.  We
+    // must Peek() for the amount ready for reading to avoid blocking.

[willchan no longer on Chromium, 2013/10/04 21:01:35]

Why don't you just use read() instead and do the loop here rather than in
Receive()'s file_util::ReadFromFD()?

[DaleCurtis, 2013/10/04 21:08:39]

Code reuse? What's the benefit of reimplementing the EINTR read loop?

[willchan no longer on Chromium, 2013/10/09 20:06:52]

Sorry, I was hung up on disliking the API that always reads the full buffer
length rather than returning ASAP. This is fine as is.

+    DCHECK(FD_ISSET(handle_, &rfds));
+    const size_t bytes_to_read = std::min(Peek(), length - bytes_read_total);
+    const size_t bytes_received =
+        Receive(charbuffer + bytes_read_total, bytes_to_read);
+    bytes_read_total += bytes_received;
+    if (bytes_received != bytes_to_read)
+      return bytes_read_total;
+  } while (bytes_read_total < length &&
+           (timeout -= (base::TimeTicks::Now() - start_time)) >

[willchan no longer on Chromium, 2013/10/04 21:01:35]

drop the base::

[DaleCurtis, 2013/10/04 23:11:32]

Done.

+               base::TimeDelta());
+  return bytes_read_total;
+}
+
 size_t SyncSocket::Peek() {
   int number_chars;
   if (-1 == ioctl(handle_, FIONREAD, &number_chars)) {
     // If there is an error in ioctl, signal that the channel would block.
     return 0;
   }
-  return (size_t) number_chars;
+  DCHECK_GE(number_chars, 0);
+  return number_chars;
 }
 
 CancelableSyncSocket::CancelableSyncSocket() {}
 CancelableSyncSocket::CancelableSyncSocket(Handle handle)
     : SyncSocket(handle) {
 }
 
 bool CancelableSyncSocket::Shutdown() {
   return HANDLE_EINTR(shutdown(handle(), SHUT_RDWR)) >= 0;
 }
@@ skipping 17 matching lines @@
   return len;
 }
 
 // static
 bool CancelableSyncSocket::CreatePair(CancelableSyncSocket* socket_a,
                                       CancelableSyncSocket* socket_b) {
   return SyncSocket::CreatePair(socket_a, socket_b);
 }
 
 }  // namespace base