From 319c3c70385a63aaea3c1a68a79c70cfd533c6b6 Mon Sep 17 00:00:00 2001 From: Takashi Kokubun Date: Thu, 13 Feb 2025 21:13:47 -0800 Subject: [PATCH] merge revision(s) 1683dadb19876f0a64589bdbbcf6fff8143f78ff: [Backport #21088] Do not save ResolutionError if resolution succeeds for any address family (#12678) * Do not save ResolutionError if resolution succeeds for any address family Socket with Happy Eyeballs Version 2 performs connection attempts and name resolution in parallel. In the existing implementation, if a connection attempt failed for one address family while name resolution was still in progress for the other, and that name resolution later failed, the method would terminate with a name resolution error. This behavior was intended to ensure that the final error reflected the most recent failure, potentially overriding an earlier error. However, [Bug #21088](https://bugs.ruby-lang.org/issues/21088) made me realize that terminating with a name resolution error is unnatural when name resolution succeeded for at least one address family. This PR modifies the behavior so that if name resolution succeeds for one address family, any name resolution error from the other is not saved. This PR includes the following changes: * Do not display select(2) as the system call that caused the raised error, as it is for internal processing * Fix bug: Get errno with Socket::SO_ERROR in Windows environment with a workaround for tests not passing --- ext/socket/ipsocket.c | 33 ++++++++++++++++++++------------- ext/socket/lib/socket.rb | 9 ++++++--- test/socket/test_socket.rb | 22 ++++++++++++++++++++++ test/socket/test_tcp.rb | 2 +- version.h | 2 +- 5 files changed, 50 insertions(+), 18 deletions(-) diff --git a/ext/socket/ipsocket.c b/ext/socket/ipsocket.c index 3e497a43be..ee1bc4a77b 100644 --- a/ext/socket/ipsocket.c +++ b/ext/socket/ipsocket.c @@ -892,7 +892,6 @@ init_fast_fallback_inetsock_internal(VALUE v) } status = rb_thread_fd_select(nfds, &arg->readfds, &arg->writefds, NULL, delay_p); - syscall = "select(2)"; now = current_clocktime_ts(); if (is_timeout_tv(resolution_delay_expires_at, now)) { @@ -998,9 +997,11 @@ init_fast_fallback_inetsock_internal(VALUE v) if (arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err && arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err != EAI_ADDRFAMILY) { - last_error.type = RESOLUTION_ERROR; - last_error.ecode = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err; - syscall = "getaddrinfo(3)"; + if (!resolution_store.v4.finished || resolution_store.v4.has_error) { + last_error.type = RESOLUTION_ERROR; + last_error.ecode = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err; + syscall = "getaddrinfo(3)"; + } resolution_store.v6.has_error = true; } else { resolution_store.v6.ai = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->ai; @@ -1015,9 +1016,11 @@ init_fast_fallback_inetsock_internal(VALUE v) resolution_store.v4.finished = true; if (arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err) { - last_error.type = RESOLUTION_ERROR; - last_error.ecode = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err; - syscall = "getaddrinfo(3)"; + if (!resolution_store.v6.finished || resolution_store.v6.has_error) { + last_error.type = RESOLUTION_ERROR; + last_error.ecode = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err; + syscall = "getaddrinfo(3)"; + } resolution_store.v4.has_error = true; } else { resolution_store.v4.ai = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->ai; @@ -1057,9 +1060,11 @@ init_fast_fallback_inetsock_internal(VALUE v) resolution_store.v6.finished = true; if (arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err) { - last_error.type = RESOLUTION_ERROR; - last_error.ecode = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err; - syscall = "getaddrinfo(3)"; + if (!resolution_store.v4.finished || resolution_store.v4.has_error) { + last_error.type = RESOLUTION_ERROR; + last_error.ecode = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err; + syscall = "getaddrinfo(3)"; + } resolution_store.v6.has_error = true; } else { resolution_store.v6.ai = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->ai; @@ -1075,9 +1080,11 @@ init_fast_fallback_inetsock_internal(VALUE v) resolution_store.v4.finished = true; if (arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err) { - last_error.type = RESOLUTION_ERROR; - last_error.ecode = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err; - syscall = "getaddrinfo(3)"; + if (!resolution_store.v6.finished || resolution_store.v6.has_error) { + last_error.type = RESOLUTION_ERROR; + last_error.ecode = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err; + syscall = "getaddrinfo(3)"; + } resolution_store.v4.has_error = true; } else { resolution_store.v4.ai = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->ai; diff --git a/ext/socket/lib/socket.rb b/ext/socket/lib/socket.rb index 6a1ddf9eda..5852934ef0 100644 --- a/ext/socket/lib/socket.rb +++ b/ext/socket/lib/socket.rb @@ -833,7 +833,7 @@ class Socket < BasicSocket if except_sockets&.any? except_sockets.each do |except_socket| failed_ai = connecting_sockets.delete except_socket - sockopt = except_socket.getsockopt(Socket::SOL_SOCKET, Socket::SO_CONNECT_TIME) + sockopt = except_socket.getsockopt(Socket::SOL_SOCKET, Socket::SO_ERROR) except_socket.close ip_address = failed_ai.ipv6? ? "[#{failed_ai.ip_address}]" : failed_ai.ip_address last_error = SystemCallError.new("connect(2) for #{ip_address}:#{failed_ai.ip_port}", sockopt.int) @@ -862,7 +862,10 @@ class Socket < BasicSocket unless (Socket.const_defined?(:EAI_ADDRFAMILY)) && (result.is_a?(Socket::ResolutionError)) && (result.error_code == Socket::EAI_ADDRFAMILY) - last_error = result + other = family_name == :ipv6 ? :ipv4 : :ipv6 + if !resolution_store.resolved?(other) || !resolution_store.resolved_successfully?(other) + last_error = result + end end else resolution_store.add_resolved(family_name, result) @@ -1068,7 +1071,7 @@ class Socket < BasicSocket end def resolved_successfully?(family) - resolved?(family) && !!@error_dict[family] + resolved?(family) && !@error_dict[family] end def resolved_all_families? diff --git a/test/socket/test_socket.rb b/test/socket/test_socket.rb index 4d75caab50..27e60b3335 100644 --- a/test/socket/test_socket.rb +++ b/test/socket/test_socket.rb @@ -995,6 +995,28 @@ class TestSocket < Test::Unit::TestCase RUBY end + def test_tcp_socket_hostname_resolution_failed_after_connection_failure + opts = %w[-rsocket -W1] + assert_separately opts, <<~RUBY + server = TCPServer.new("127.0.0.1", 0) + port = server.connect_address.ip_port + + Addrinfo.define_singleton_method(:getaddrinfo) do |_, _, family, *_| + case family + when Socket::AF_INET6 then sleep(0.1); raise Socket::ResolutionError + when Socket::AF_INET then [Addrinfo.tcp("127.0.0.1", port)] + end + end + + server.close + + # SystemCallError is a workaround for Windows environment + assert_raise(Errno::ECONNREFUSED, SystemCallError) do + Socket.tcp("localhost", port) + end + RUBY + end + def test_tcp_socket_v6_address_passed opts = %w[-rsocket -W1] assert_separately opts, <<~RUBY diff --git a/test/socket/test_tcp.rb b/test/socket/test_tcp.rb index 4984a7e7bc..e6a41f5660 100644 --- a/test/socket/test_tcp.rb +++ b/test/socket/test_tcp.rb @@ -316,7 +316,7 @@ class TestSocket_TCPSocket < Test::Unit::TestCase port = server.connect_address.ip_port server.close - assert_raise(Socket::ResolutionError) do + assert_raise(Errno::ECONNREFUSED) do TCPSocket.new( "localhost", port, diff --git a/version.h b/version.h index 2d56275a35..07c23c93a8 100644 --- a/version.h +++ b/version.h @@ -11,7 +11,7 @@ # define RUBY_VERSION_MINOR RUBY_API_VERSION_MINOR #define RUBY_VERSION_TEENY 1 #define RUBY_RELEASE_DATE RUBY_RELEASE_YEAR_STR"-"RUBY_RELEASE_MONTH_STR"-"RUBY_RELEASE_DAY_STR -#define RUBY_PATCHLEVEL 23 +#define RUBY_PATCHLEVEL 24 #include "ruby/version.h" #include "ruby/internal/abi.h"