Treat EAGAIN/EWOULDBLOCK as requests for retry on socket accept. (issue 9837060)

[ag...@google.com]

Reviewers: Ivan Posva, Søren Gjesse,

Description:
Treat EAGAIN/EWOULDBLOCK as requests for retry on socket accept.

On Linux there are a number of protocol errors that can be
returned from accept. According to the linux man pages they
should be treated as EAGAIN.

This should hopefully fix the flakiness of EchoServerStreamTest.

R=ipo...@google.com,sgj...@google.com
BUG=2262
TEST=


Please review this at https://chromiumcodereview.appspot.com/9837060/

SVN Base: https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Affected files:
M runtime/bin/socket.h
M runtime/bin/socket.cc
M runtime/bin/socket_impl.dart
M runtime/bin/socket_linux.cc
M runtime/bin/socket_macos.cc


Index: runtime/bin/socket.cc
diff --git a/runtime/bin/socket.cc b/runtime/bin/socket.cc
index
054136030fa854631de37c9307ed1c9ae607bb13..71a08c76757fcb747f4668488b9401b0a609ca49
100644
--- a/runtime/bin/socket.cc
+++ b/runtime/bin/socket.cc
@@ -211,6 +211,8 @@ void
FUNCTION_NAME(ServerSocket_Accept)(Dart_NativeArguments args) {
DartUtils::SetIntegerField(
socketobj, DartUtils::kIdFieldName, newSocket);
Dart_SetReturnValue(args, Dart_True());
+ } else if (newSocket == ServerSocket::kTemporaryFailure) {
+ Dart_SetReturnValue(args, Dart_False());
} else {
Dart_SetReturnValue(args, DartUtils::NewDartOSError());
}
Index: runtime/bin/socket.h
diff --git a/runtime/bin/socket.h b/runtime/bin/socket.h
index
1d3a324eba0240f04f99b242749e5c630cc5caba..8ecff7ea553b0bc12188a725fa9420771b38f95a
100644
--- a/runtime/bin/socket.h
+++ b/runtime/bin/socket.h
@@ -45,6 +45,8 @@ class Socket {

class ServerSocket {
public:
+ static const intptr_t kTemporaryFailure = -2;
+
static intptr_t Accept(intptr_t fd);
static intptr_t CreateBindListen(const char* bindAddress,
intptr_t port,
Index: runtime/bin/socket_impl.dart
diff --git a/runtime/bin/socket_impl.dart b/runtime/bin/socket_impl.dart
index
3a8ecd9027e6c3cec89dafbd101c3fc8fbabae89..a0b5d563253dc3dcb1bafbe2afbff5895860e91e
100644
--- a/runtime/bin/socket_impl.dart
+++ b/runtime/bin/socket_impl.dart
@@ -292,8 +292,12 @@ class _ServerSocket extends _SocketBase implements
ServerSocket {
var result = _accept(socket);
if (result is OSError) {
_reportError(result, "Accept failed");
- } else {
+ } else if (result) {
_clientConnectionHandler(socket);
+ } else {
+ // Temporary failure accepting the connection. Ignoring
+ // temporary failures lets us retry when we wake up with data
+ // on the listening socket again.
}
}
}
Index: runtime/bin/socket_linux.cc
diff --git a/runtime/bin/socket_linux.cc b/runtime/bin/socket_linux.cc
index
5f4096bb75f3120625ccbc2bb3fb705f70fdeeee..8e37849f3d2fda52ab4a047809085a766e1a29f5
100644
--- a/runtime/bin/socket_linux.cc
+++ b/runtime/bin/socket_linux.cc
@@ -188,13 +188,32 @@ intptr_t ServerSocket::CreateBindListen(const char*
host,
return fd;
}

+
+static bool IsTemporaryAcceptError(int error) {
+ // On Linux a number of protocol errors should be treated as EAGAIN.
+ // These are the ones for TCP/IP.
+ return (error == EAGAIN) || (error == ENETDOWN) || (error == EPROTO) ||
+ (error == ENOPROTOOPT) || (error == EHOSTDOWN) || (error == ENONET) |
|
+ (error == EHOSTUNREACH) || (error == EOPNOTSUPP) ||
+ (error == ENETUNREACH);
+}
+
+
intptr_t ServerSocket::Accept(intptr_t fd) {
intptr_t socket;
struct sockaddr clientaddr;
socklen_t addrlen = sizeof(clientaddr);
socket = TEMP_FAILURE_RETRY(accept(fd, &clientaddr, &addrlen));
- if (socket < 0) {
- fprintf(stderr, "Error Accept: %s\n", strerror(errno));
+ if (socket == -1) {
+ if (IsTemporaryAcceptError(errno)) {
+ // We need to signal to the caller that this is actually not an
+ // error. We got woken up from the poll on the listening socket,
+ // but there is no connection ready to be accepted.
+ ASSERT(kTemporaryFailure != -1);
+ socket = kTemporaryFailure;
+ } else {
+ fprintf(stderr, "Error Accept: %s\n", strerror(errno));
+ }
} else {
FDUtils::SetNonBlocking(socket);
}
Index: runtime/bin/socket_macos.cc
diff --git a/runtime/bin/socket_macos.cc b/runtime/bin/socket_macos.cc
index
c9b82049ab7d31b0cdd7686bc96586dfb99bab9b..a460ed97bf46c50521fae47e02b09cb37d61b62a
100644
--- a/runtime/bin/socket_macos.cc
+++ b/runtime/bin/socket_macos.cc
@@ -189,13 +189,22 @@ intptr_t ServerSocket::CreateBindListen(const char*
host,
return fd;
}

+
intptr_t ServerSocket::Accept(intptr_t fd) {
intptr_t socket;
struct sockaddr clientaddr;
socklen_t addrlen = sizeof(clientaddr);
socket = TEMP_FAILURE_RETRY(accept(fd, &clientaddr, &addrlen));
- if (socket < 0) {
- fprintf(stderr, "Error Accept: %s\n", strerror(errno));
+ if (socket == -1) {
+ if (errno == EAGAIN) {
+ // We need to signal to the caller that this is actually not an
+ // error. We got woken up from the poll on the listening socket,
+ // but there is no connection ready to be accepted.
+ ASSERT(kTemporaryFailure != -1);
+ socket = kTemporaryFailure;
+ } else {
+ fprintf(stderr, "Error Accept: %s\n", strerror(errno));
+ }
} else {
FDUtils::SetNonBlocking(socket);
}

[ipo...@google.com]

LGTM -ip

https://chromiumcodereview.appspot.com/9837060/diff/1/runtime/bin/socket_linux.cc
File runtime/bin/socket_linux.cc (right):

https://chromiumcodereview.appspot.com/9837060/diff/1/runtime/bin/socket_linux.cc#newcode215
runtime/bin/socket_linux.cc:215: fprintf(stderr, "Error Accept: %s\n",
strerror(errno));
Mads says this should be removed.

https://chromiumcodereview.appspot.com/9837060/diff/1/runtime/bin/socket_macos.cc
File runtime/bin/socket_macos.cc (right):

https://chromiumcodereview.appspot.com/9837060/diff/1/runtime/bin/socket_macos.cc#newcode206
runtime/bin/socket_macos.cc:206: fprintf(stderr, "Error Accept: %s\n",
strerror(errno));
ditto.

https://chromiumcodereview.appspot.com/9837060/