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

base/sync_socket_posix.cc

Index: base/sync_socket_posix.cc
diff --git a/base/sync_socket_posix.cc b/base/sync_socket_posix.cc
index 257916df3357a21962a4f6b9fe7561a09c286d50..d76b00833c10b7d6332952b3ae196ea89ff175ba 100644
--- a/base/sync_socket_posix.cc
+++ b/base/sync_socket_posix.cc
@@ -19,7 +19,6 @@
 #include "base/file_util.h"
 #include "base/logging.h"
 
-
 namespace base {
 
 namespace {
@@ -93,6 +92,7 @@ bool SyncSocket::Close() {
 }
 
 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);
@@ -101,6 +101,7 @@ size_t SyncSocket::Send(const void* buffer, size_t length) {
 }
 
 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))
@@ -108,13 +109,56 @@ size_t SyncSocket::Receive(void* buffer, size_t 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() {}