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

ChangeLog

@@ -1,3 +1,9 @@
+Thu Sep 18 05:44:05 2014  Eric Wong  <e@80x24.org>
+
+	* ext/socket/init.c (rsock_connect): refactor for blocking
+	  (wait_connectable): clear error before wait
+	  [Bug #9356]
+
 Wed Sep 17 23:12:36 2014  NARUSE, Yui  <naruse@ruby-lang.org>
 
 	* lib/uri/rfc3986_parser.rb: specify a regexp for :OPAQUE; generic.rb

ext/socket/init.c

@@ -339,68 +339,65 @@ 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 (;;) {
+    /* only to clear pending error */
+    sockerrlen = (socklen_t)sizeof(sockerr);
+    if (getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen) < 0)
+        return -1;
+
     /*
      * 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);
 
-	if (revents & (RB_WAITFD_IN|RB_WAITFD_OUT)) {
+    if (revents < 0)
+        return -1;
+
     sockerrlen = (socklen_t)sizeof(sockerr);
-	    ret = getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen);
+    if (getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen) < 0)
+        return -1;
 
+    switch (sockerr) {
+      case 0:
       /*
-	     * Solaris getsockopt(SO_ERROR) return -1 and set errno
+       * be defensive in case some platforms set SO_ERROR on the original,
-	     * in getsockopt(). Let's return immediately.
+       * interrupted connect()
        */
-	    if (ret < 0)
+      case EINTR:
-		break;
+#ifdef ERESTART
-	    if (sockerr == 0) {
+      case ERESTART:
-		if (revents & RB_WAITFD_OUT)
+#endif
-		    break;
+      case EAGAIN:
-		else
+#ifdef EINPROGRESS
-		    continue;	/* workaround for winsock */
+      case EINPROGRESS:
-	    }
+#endif
-
+#ifdef EALREADY
-	    /* BSD and Linux use sockerr. */
+      case EALREADY:
+#endif
+#ifdef EISCONN
+      case EISCONN:
+#endif
+	return 0; /* success */
+      default:
+        /* likely (but not limited to): ECONNREFUSED, ETIMEDOUT, EHOSTUNREACH */
         errno = sockerr;
-	    ret = -1;
+        return -1;
-	    break;
     }
 
-	if ((revents & (RB_WAITFD_IN|RB_WAITFD_OUT)) == RB_WAITFD_OUT) {
+    return 0;
-	    ret = 0;
-	    break;
-	}
-    }
-
-    return ret;
 }
 
-#ifdef __CYGWIN__
-#define WAIT_IN_PROGRESS 10
-#endif
-#ifdef __APPLE__
-#define WAIT_IN_PROGRESS 10
-#endif
-#ifdef __linux__
-/* returns correct error */
-#define WAIT_IN_PROGRESS 0
-#endif
-#ifndef WAIT_IN_PROGRESS
-/* BSD origin code apparently has a problem */
-#define WAIT_IN_PROGRESS 1
-#endif
-
 struct connect_arg {
     int fd;
     const struct sockaddr *sockaddr;
@@ -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)
+#ifdef ERESTART
           case ERESTART:
 #endif
-		continue;
-
           case EAGAIN:
 #ifdef EINPROGRESS
           case EINPROGRESS:
 #endif
-#if WAIT_IN_PROGRESS > 0
+            return wait_connectable(fd);
-		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;
-#endif
-	      default:
-		break;
         }
     }
     return status;
-    }
 }
 
 static void