socket (rsock_connect): fix and refactor for blocking

* ext/socket/init.c (rsock_connect): refactor for blocking
  (wait_connectable): clear error before wait
  [Bug #9356]

We no longer use non-blocking sockets to emulate blocking behavior,
so eliminate error-prone and confusing platform-dependent code.
According to POSIX, connect() only needs to be called once in the
face of EINTR, so do not loop on it.

Before waiting on connect, drop any pending errors, since
rb_wait_for_single_fd may not clear the existing error
properly.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@47617 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

1 files changed, 58 insertions, 120 deletions

diff --git a/ext/socket/init.c b/ext/socket/init.c
index f6cd2bce7b..670349ae14 100644
--- a/ext/socket/init.c
+++ b/ext/socket/init.c
@@ -339,67 +339,64 @@ rsock_socket(int domain, int type, int proto)
     return fd;
 }
 
+/* emulate blocking connect behavior on EINTR or non-blocking socket */
 static int
 wait_connectable(int fd)
 {
-    int sockerr;
+    int sockerr, revents;
     socklen_t sockerrlen;
-    int revents;
-    int ret;
 
-    for (;;) {
-	/*
-	 * Stevens book says, successful finish turn on RB_WAITFD_OUT and
-	 * failure finish turn on both RB_WAITFD_IN and RB_WAITFD_OUT.
-	 */
-	revents = rb_wait_for_single_fd(fd, RB_WAITFD_IN|RB_WAITFD_OUT, NULL);
-
-	if (revents & (RB_WAITFD_IN|RB_WAITFD_OUT)) {
-	    sockerrlen = (socklen_t)sizeof(sockerr);
-	    ret = getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen);
-
-	    /*
-	     * Solaris getsockopt(SO_ERROR) return -1 and set errno
-	     * in getsockopt(). Let's return immediately.
-	     */
-	    if (ret < 0)
-		break;
-	    if (sockerr == 0) {
-		if (revents & RB_WAITFD_OUT)
-		    break;
-		else
-		    continue;	/* workaround for winsock */
-	    }
-
-	    /* BSD and Linux use sockerr. */
-	    errno = sockerr;
-	    ret = -1;
-	    break;
-	}
+    /* only to clear pending error */
+    sockerrlen = (socklen_t)sizeof(sockerr);
+    if (getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen) < 0)
+        return -1;
 
-	if ((revents & (RB_WAITFD_IN|RB_WAITFD_OUT)) == RB_WAITFD_OUT) {
-	    ret = 0;
-	    break;
-	}
-    }
+    /*
+     * Stevens book says, successful finish turn on RB_WAITFD_OUT and
+     * failure finish turn on both RB_WAITFD_IN and RB_WAITFD_OUT.
+     * So it's enough to wait only RB_WAITFD_OUT and check the pending error
+     * by getsockopt().
+     *
+     * Note: rb_wait_for_single_fd already retries on EINTR/ERESTART
+     */
+    revents = rb_wait_for_single_fd(fd, RB_WAITFD_IN|RB_WAITFD_OUT, NULL);
 
-    return ret;
-}
+    if (revents < 0)
+        return -1;
+
+    sockerrlen = (socklen_t)sizeof(sockerr);
+    if (getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen) < 0)
+        return -1;
 
-#ifdef __CYGWIN__
-#define WAIT_IN_PROGRESS 10
+    switch (sockerr) {
+      case 0:
+      /*
+       * be defensive in case some platforms set SO_ERROR on the original,
+       * interrupted connect()
+       */
+      case EINTR:
+#ifdef ERESTART
+      case ERESTART:
 #endif
-#ifdef __APPLE__
-#define WAIT_IN_PROGRESS 10
+      case EAGAIN:
+#ifdef EINPROGRESS
+      case EINPROGRESS:
 #endif
-#ifdef __linux__
-/* returns correct error */
-#define WAIT_IN_PROGRESS 0
+#ifdef EALREADY
+      case EALREADY:
 #endif
-#ifndef WAIT_IN_PROGRESS
-/* BSD origin code apparently has a problem */
-#define WAIT_IN_PROGRESS 1
+#ifdef EISCONN
+      case EISCONN:
 #endif
+	return 0; /* success */
+      default:
+        /* likely (but not limited to): ECONNREFUSED, ETIMEDOUT, EHOSTUNREACH */
+        errno = sockerr;
+        return -1;
+    }
+
+    return 0;
+}
 
 struct connect_arg {
     int fd;
@@ -429,11 +426,6 @@ rsock_connect(int fd, const struct sockaddr *sockaddr, int len, int socks)
     int status;
     rb_blocking_function_t *func = connect_blocking;
     struct connect_arg arg;
-#if WAIT_IN_PROGRESS > 0
-    int wait_in_progress = -1;
-    int sockerr;
-    socklen_t sockerrlen;
-#endif
 
     arg.fd = fd;
     arg.sockaddr = sockaddr;
@@ -441,76 +433,22 @@ rsock_connect(int fd, const struct sockaddr *sockaddr, int len, int socks)
 #if defined(SOCKS) && !defined(SOCKS5)
     if (socks) func = socks_connect_blocking;
 #endif
-    for (;;) {
-	status = (int)BLOCKING_REGION_FD(func, &arg);
-	if (status < 0) {
-	    switch (errno) {
-	      case EINTR:
-#if defined(ERESTART)
-	      case ERESTART:
-#endif
-		continue;
+    status = (int)BLOCKING_REGION_FD(func, &arg);
 
-	      case EAGAIN:
-#ifdef EINPROGRESS
-	      case EINPROGRESS:
+    if (status < 0) {
+        switch (errno) {
+          case EINTR:
+#ifdef ERESTART
+          case ERESTART:
 #endif
-#if WAIT_IN_PROGRESS > 0
-		sockerrlen = (socklen_t)sizeof(sockerr);
-		status = getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen);
-		if (status) break;
-		if (sockerr) {
-		    status = -1;
-		    errno = sockerr;
-		    break;
-		}
-#endif
-#ifdef EALREADY
-	      case EALREADY:
-#endif
-#if WAIT_IN_PROGRESS > 0
-		wait_in_progress = WAIT_IN_PROGRESS;
-#endif
-		status = wait_connectable(fd);
-		if (status) {
-		    break;
-		}
-		errno = 0;
-		continue;
-
-#if WAIT_IN_PROGRESS > 0
-	      case EINVAL:
-		if (wait_in_progress-- > 0) {
-		    /*
-		     * connect() after EINPROGRESS returns EINVAL on
-		     * some platforms, need to check true error
-		     * status.
-		     */
-		    sockerrlen = (socklen_t)sizeof(sockerr);
-		    status = getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen);
-		    if (!status && !sockerr) {
-			struct timeval tv = {0, 100000};
-			rb_thread_wait_for(tv);
-			continue;
-		    }
-		    status = -1;
-		    errno = sockerr;
-		}
-		break;
-#endif
-
-#ifdef EISCONN
-	      case EISCONN:
-		status = 0;
-		errno = 0;
-		break;
+          case EAGAIN:
+#ifdef EINPROGRESS
+          case EINPROGRESS:
 #endif
-	      default:
-		break;
-	    }
-	}
-	return status;
+            return wait_connectable(fd);
+        }
     }
+    return status;
 }
 
 static void