Do not save the last error without sockets in the connection attempt (ruby#12153)

shioimm · web-flow · commit ff5fc4b5a13e · 2024-11-25T14:10:54.000+09:00
* Do not save the last_error if there are no sockets waiting to be connected

In this implementation, the results of both name resolution and connection attempts are awaited using select(2).
When it returned, the implementation attempted to check for connections even if there were no sockets currently attempting to connect, treating the absence of connected sockets as a connection failure.
With this fix, it will no longer check for connections when there are no sockets waiting to be connected.

Additionally, the following minor fixes have been made:

* Handle failure of getsockopt(2) and removed unnecessary continue in the loop

* Tweak: Use common API to check in_progress_fds

* Safely call TCPServer.new in test

* Set empty writefds when there is no socket waiting to be connected

* Enable fast_fallback option
diff --git a/ext/socket/ipsocket.c b/ext/socket/ipsocket.c
@@ -574,7 +574,7 @@ init_fast_fallback_inetsock_internal(VALUE v)
     struct wait_fast_fallback_arg wait_arg;
     struct timeval *ends_at = NULL;
     struct timeval delay = (struct timeval){ -1, -1 };
-    wait_arg.writefds = NULL;
+    wait_arg.writefds = &writefds;
     wait_arg.status = 0;
 
     struct hostname_resolution_store resolution_store;
@@ -859,7 +859,6 @@ init_fast_fallback_inetsock_internal(VALUE v)
                     }
                     arg->connection_attempt_fds[arg->connection_attempt_fds_size] = fd;
                     (arg->connection_attempt_fds_size)++;
-                    wait_arg.writefds = &writefds;
 
                     set_timeout_tv(&connection_attempt_delay_strage, 250, now);
                     connection_attempt_delay_expires_at = &connection_attempt_delay_strage;
@@ -922,8 +921,8 @@ init_fast_fallback_inetsock_internal(VALUE v)
         }
 
         wait_arg.nfds = 0;
-        if (arg->connection_attempt_fds_size) {
-            FD_ZERO(wait_arg.writefds);
+        FD_ZERO(wait_arg.writefds);
+        if (in_progress_fds(arg->connection_attempt_fds_size)) {
             int n = 0;
             for (int i = 0; i < arg->connection_attempt_fds_size; i++) {
                 int cfd = arg->connection_attempt_fds[i];
@@ -933,8 +932,6 @@ init_fast_fallback_inetsock_internal(VALUE v)
             }
             if (n > 0) n++;
             wait_arg.nfds = n;
-        } else {
-            wait_arg.writefds = NULL;
         }
 
         FD_ZERO(wait_arg.readfds);
@@ -967,14 +964,39 @@ init_fast_fallback_inetsock_internal(VALUE v)
 
         if (status > 0) {
             /* check for connection */
-            for (int i = 0; i < arg->connection_attempt_fds_size; i++) {
-                int fd = arg->connection_attempt_fds[i];
-                if (fd < 0 || !FD_ISSET(fd, wait_arg.writefds)) continue;
+            if (in_progress_fds(arg->connection_attempt_fds_size)) {
+                for (int i = 0; i < arg->connection_attempt_fds_size; i++) {
+                    int fd = arg->connection_attempt_fds[i];
+                    if (fd < 0 || !FD_ISSET(fd, wait_arg.writefds)) continue;
+
+                    int err;
+                    socklen_t len = sizeof(err);
+
+                    status = getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &len);
+
+                    if (status < 0) {
+                        last_error.type = SYSCALL_ERROR;
+                        last_error.ecode = errno;
+                        close(fd);
 
-                int err;
-                socklen_t len = sizeof(err);
+                        if (any_addrinfos(&resolution_store)) continue;
+                        if (in_progress_fds(arg->connection_attempt_fds_size)) break;
+                        if (!resolution_store.is_all_finised) break;
+
+                        if (local_status < 0) {
+                            host = arg->local.host;
+                            serv = arg->local.serv;
+                        } else {
+                            host = arg->remote.host;
+                            serv = arg->remote.serv;
+                        }
+                        if (last_error.type == RESOLUTION_ERROR) {
+                            rsock_raise_resolution_error(syscall, last_error.ecode);
+                        } else {
+                            rsock_syserr_fail_host_port(last_error.ecode, syscall, host, serv);
+                        }
+                    }
 
-                if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &len) == 0) {
                     if (err == 0) { /* success */
                         remove_connection_attempt_fd(
                             arg->connection_attempt_fds,
@@ -983,42 +1005,39 @@ init_fast_fallback_inetsock_internal(VALUE v)
                         );
                         connected_fd = fd;
                         break;
-                    };
-
-                    /* fail */
-                    errno = err;
-                    close(fd);
-                    remove_connection_attempt_fd(
-                        arg->connection_attempt_fds,
-                        &arg->connection_attempt_fds_size,
-                        fd
-                    );
-                    continue;
+                    } else { /* fail */
+                        close(fd);
+                        remove_connection_attempt_fd(
+                            arg->connection_attempt_fds,
+                            &arg->connection_attempt_fds_size,
+                            fd
+                        );
+                        last_error.type = SYSCALL_ERROR;
+                        last_error.ecode = err;
+                    }
                 }
-            }
 
-            if (connected_fd >= 0) break;
-            last_error.type = SYSCALL_ERROR;
-            last_error.ecode = errno;
+                if (connected_fd >= 0) break;
 
-            if (!in_progress_fds(arg->connection_attempt_fds_size)) {
-                if (any_addrinfos(&resolution_store)) {
-                    connection_attempt_delay_expires_at = NULL;
-                } else if (resolution_store.is_all_finised) {
-                    if (local_status < 0) {
-                        host = arg->local.host;
-                        serv = arg->local.serv;
-                    } else {
-                        host = arg->remote.host;
-                        serv = arg->remote.serv;
-                    }
-                    if (last_error.type == RESOLUTION_ERROR) {
-                        rsock_raise_resolution_error(syscall, last_error.ecode);
-                    } else {
-                        rsock_syserr_fail_host_port(last_error.ecode, syscall, host, serv);
+                if (!in_progress_fds(arg->connection_attempt_fds_size)) {
+                    if (any_addrinfos(&resolution_store)) {
+                        connection_attempt_delay_expires_at = NULL;
+                    } else if (resolution_store.is_all_finised) {
+                        if (local_status < 0) {
+                            host = arg->local.host;
+                            serv = arg->local.serv;
+                        } else {
+                            host = arg->remote.host;
+                            serv = arg->remote.serv;
+                        }
+                        if (last_error.type == RESOLUTION_ERROR) {
+                            rsock_raise_resolution_error(syscall, last_error.ecode);
+                        } else {
+                            rsock_syserr_fail_host_port(last_error.ecode, syscall, host, serv);
+                        }
                     }
+                    user_specified_connect_timeout_at = NULL;
                 }
-                user_specified_connect_timeout_at = NULL;
             }
 
             /* check for hostname resolution */
diff --git a/ext/socket/tcpsocket.c b/ext/socket/tcpsocket.c
@@ -20,14 +20,17 @@
  *
  * Starting from Ruby 3.4, this method operates according to the
  * Happy Eyeballs Version 2 ({RFC 8305}[https://datatracker.ietf.org/doc/html/rfc8305])
- * algorithm with *fast_fallback:true+, except on Windows.
+ * algorithm by default, except on Windows.
+ *
+ * To make it behave the same as in Ruby 3.3 and earlier,
+ * explicitly specify the option +fast_fallback:false+.
  *
  * Happy Eyeballs Version 2 is not provided on Windows,
  * and it behaves the same as in Ruby 3.3 and earlier.
  *
  * [:resolv_timeout] Specifies the timeout in seconds from when the hostname resolution starts.
  * [:connect_timeout] This method sequentially attempts connecting to all candidate destination addresses.<br>The +connect_timeout+ specifies the timeout in seconds from the start of the connection attempt to the last candidate.<br>By default, all connection attempts continue until the timeout occurs.<br>When +fast_fallback:false+ is explicitly specified,<br>a timeout is set for each connection attempt and any connection attempt that exceeds its timeout will be canceled.
- * [:fast_fallback] Enables the Happy Eyeballs Version 2 algorithm (disabled by default).
+ * [:fast_fallback] Enables the Happy Eyeballs Version 2 algorithm (enabled by default).
  *
  * === Happy Eyeballs Version 2
  * Happy Eyeballs Version 2 ({RFC 8305}[https://datatracker.ietf.org/doc/html/rfc8305])
@@ -57,7 +60,7 @@ tcp_init(int argc, VALUE *argv, VALUE sock)
     VALUE kwargs[4];
     VALUE resolv_timeout = Qnil;
     VALUE connect_timeout = Qnil;
-    VALUE fast_fallback = Qfalse;
+    VALUE fast_fallback = Qtrue;
     VALUE test_mode_settings = Qnil;
 
     if (!keyword_ids[0]) {
diff --git a/test/socket/test_tcp.rb b/test/socket/test_tcp.rb
@@ -142,7 +142,7 @@ def test_accept_multithread
   end
 
   def test_initialize_v6_hostname_resolved_earlier
-    pend "to suppress the output of test failure logs in CI temporarily"
+    # pend "to suppress the output of test failure logs in CI temporarily"
     return if RUBY_PLATFORM =~ /mswin|mingw|cygwin/
 
     begin
@@ -167,7 +167,7 @@ def test_initialize_v6_hostname_resolved_earlier
   end
 
   def test_initialize_v4_hostname_resolved_earlier
-    pend "to suppress the output of test failure logs in CI temporarily"
+    # pend "to suppress the output of test failure logs in CI temporarily"
     return if RUBY_PLATFORM =~ /mswin|mingw|cygwin/
 
     server = TCPServer.new("127.0.0.1", 0)
@@ -188,7 +188,7 @@ def test_initialize_v4_hostname_resolved_earlier
   end
 
   def test_initialize_v6_hostname_resolved_in_resolution_delay
-    pend "to suppress the output of test failure logs in CI temporarily"
+    # pend "to suppress the output of test failure logs in CI temporarily"
     return if RUBY_PLATFORM =~ /mswin|mingw|cygwin/
 
     begin
@@ -214,7 +214,7 @@ def test_initialize_v6_hostname_resolved_in_resolution_delay
   end
 
   def test_initialize_v6_hostname_resolved_earlier_and_v6_server_is_not_listening
-    pend "to suppress the output of test failure logs in CI temporarily"
+    # pend "to suppress the output of test failure logs in CI temporarily"
     return if RUBY_PLATFORM =~ /mswin|mingw|cygwin/
 
     ipv4_address = "127.0.0.1"
@@ -237,7 +237,7 @@ def test_initialize_v6_hostname_resolved_earlier_and_v6_server_is_not_listening
   end
 
   def test_initialize_v6_hostname_resolved_later_and_v6_server_is_not_listening
-    pend "to suppress the output of test failure logs in CI temporarily"
+    # pend "to suppress the output of test failure logs in CI temporarily"
     return if RUBY_PLATFORM =~ /mswin|mingw|cygwin/
 
     ipv4_server = Socket.new(Socket::AF_INET, :STREAM)
@@ -263,7 +263,7 @@ def test_initialize_v6_hostname_resolved_later_and_v6_server_is_not_listening
   end
 
   def test_initialize_v6_hostname_resolution_failed_and_v4_hostname_resolution_is_success
-    pend "to suppress the output of test failure logs in CI temporarily"
+    # pend "to suppress the output of test failure logs in CI temporarily"
     return if RUBY_PLATFORM =~ /mswin|mingw|cygwin/
 
     server = TCPServer.new("127.0.0.1", 0)
@@ -284,10 +284,15 @@ def test_initialize_v6_hostname_resolution_failed_and_v4_hostname_resolution_is_
   end
 
   def test_initialize_resolv_timeout_with_connection_failure
-    pend "to suppress the output of test failure logs in CI temporarily"
+    # pend "to suppress the output of test failure logs in CI temporarily"
     return if RUBY_PLATFORM =~ /mswin|mingw|cygwin/
 
-    server = TCPServer.new("::1", 0)
+    begin
+      server = TCPServer.new("::1", 0)
+    rescue Errno::EADDRNOTAVAIL # IPv6 is not supported
+      exit
+    end
+
     port = server.connect_address.ip_port
     server.close
 
@@ -303,7 +308,7 @@ def test_initialize_resolv_timeout_with_connection_failure
   end
 
   def test_initialize_with_hostname_resolution_failure_after_connection_failure
-    pend "to suppress the output of test failure logs in CI temporarily"
+    # pend "to suppress the output of test failure logs in CI temporarily"
     return if RUBY_PLATFORM =~ /mswin|mingw|cygwin/
 
     begin
@@ -325,7 +330,7 @@ def test_initialize_with_hostname_resolution_failure_after_connection_failure
   end
 
   def test_initialize_with_connection_failure_after_hostname_resolution_failure
-    pend "to suppress the output of test failure logs in CI temporarily"
+    # pend "to suppress the output of test failure logs in CI temporarily"
     return if RUBY_PLATFORM =~ /mswin|mingw|cygwin/
 
     server = TCPServer.new("127.0.0.1", 0)
