Skip to content

Commit 1683dad

Browse files
shioimmshioimm
authored
Do not save ResolutionError if resolution succeeds for any address family (ruby#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
1 parent f84d75e commit 1683dad

File tree

4 files changed

+49
-17
lines changed

4 files changed

+49
-17
lines changed

‎ext/socket/ipsocket.c‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -892,7 +892,6 @@ init_fast_fallback_inetsock_internal(VALUE v)
892892
}
893893

894894
status = rb_thread_fd_select(nfds, &arg->readfds, &arg->writefds, NULL, delay_p);
895-
syscall = "select(2)";
896895

897896
now = current_clocktime_ts();
898897
if (is_timeout_tv(resolution_delay_expires_at, now)) {
@@ -998,9 +997,11 @@ init_fast_fallback_inetsock_internal(VALUE v)
998997

999998
if (arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err &&
1000999
arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err != EAI_ADDRFAMILY) {
1001-
last_error.type = RESOLUTION_ERROR;
1002-
last_error.ecode = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err;
1003-
syscall = "getaddrinfo(3)";
1000+
if (!resolution_store.v4.finished || resolution_store.v4.has_error) {
1001+
last_error.type = RESOLUTION_ERROR;
1002+
last_error.ecode = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err;
1003+
syscall = "getaddrinfo(3)";
1004+
}
10041005
resolution_store.v6.has_error = true;
10051006
} else {
10061007
resolution_store.v6.ai = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->ai;
@@ -1015,9 +1016,11 @@ init_fast_fallback_inetsock_internal(VALUE v)
10151016
resolution_store.v4.finished = true;
10161017

10171018
if (arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err) {
1018-
last_error.type = RESOLUTION_ERROR;
1019-
last_error.ecode = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err;
1020-
syscall = "getaddrinfo(3)";
1019+
if (!resolution_store.v6.finished || resolution_store.v6.has_error) {
1020+
last_error.type = RESOLUTION_ERROR;
1021+
last_error.ecode = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err;
1022+
syscall = "getaddrinfo(3)";
1023+
}
10211024
resolution_store.v4.has_error = true;
10221025
} else {
10231026
resolution_store.v4.ai = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->ai;
@@ -1057,9 +1060,11 @@ init_fast_fallback_inetsock_internal(VALUE v)
10571060
resolution_store.v6.finished = true;
10581061

10591062
if (arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err) {
1060-
last_error.type = RESOLUTION_ERROR;
1061-
last_error.ecode = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err;
1062-
syscall = "getaddrinfo(3)";
1063+
if (!resolution_store.v4.finished || resolution_store.v4.has_error) {
1064+
last_error.type = RESOLUTION_ERROR;
1065+
last_error.ecode = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->err;
1066+
syscall = "getaddrinfo(3)";
1067+
}
10631068
resolution_store.v6.has_error = true;
10641069
} else {
10651070
resolution_store.v6.ai = arg->getaddrinfo_entries[IPV6_ENTRY_POS]->ai;
@@ -1075,9 +1080,11 @@ init_fast_fallback_inetsock_internal(VALUE v)
10751080
resolution_store.v4.finished = true;
10761081

10771082
if (arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err) {
1078-
last_error.type = RESOLUTION_ERROR;
1079-
last_error.ecode = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err;
1080-
syscall = "getaddrinfo(3)";
1083+
if (!resolution_store.v6.finished || resolution_store.v6.has_error) {
1084+
last_error.type = RESOLUTION_ERROR;
1085+
last_error.ecode = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->err;
1086+
syscall = "getaddrinfo(3)";
1087+
}
10811088
resolution_store.v4.has_error = true;
10821089
} else {
10831090
resolution_store.v4.ai = arg->getaddrinfo_entries[IPV4_ENTRY_POS]->ai;

‎ext/socket/lib/socket.rb‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -833,7 +833,7 @@ def self.tcp_with_fast_fallback(host, port, local_host = nil, local_port = nil,
833833
if except_sockets&.any?
834834
except_sockets.each do |except_socket|
835835
failed_ai = connecting_sockets.delete except_socket
836-
sockopt = except_socket.getsockopt(Socket::SOL_SOCKET, Socket::SO_CONNECT_TIME)
836+
sockopt = except_socket.getsockopt(Socket::SOL_SOCKET, Socket::SO_ERROR)
837837
except_socket.close
838838
ip_address = failed_ai.ipv6? ? "[#{failed_ai.ip_address}]" : failed_ai.ip_address
839839
last_error = SystemCallError.new("connect(2) for #{ip_address}:#{failed_ai.ip_port}", sockopt.int)
@@ -862,7 +862,10 @@ def self.tcp_with_fast_fallback(host, port, local_host = nil, local_port = nil,
862862
unless (Socket.const_defined?(:EAI_ADDRFAMILY)) &&
863863
(result.is_a?(Socket::ResolutionError)) &&
864864
(result.error_code == Socket::EAI_ADDRFAMILY)
865-
last_error = result
865+
other = family_name == :ipv6 ? :ipv4 : :ipv6
866+
if !resolution_store.resolved?(other) || !resolution_store.resolved_successfully?(other)
867+
last_error = result
868+
end
866869
end
867870
else
868871
resolution_store.add_resolved(family_name, result)
@@ -1068,7 +1071,7 @@ def resolved?(family)
10681071
end
10691072

10701073
def resolved_successfully?(family)
1071-
resolved?(family) && !!@error_dict[family]
1074+
resolved?(family) && !@error_dict[family]
10721075
end
10731076

10741077
def resolved_all_families?

‎test/socket/test_socket.rb‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -995,6 +995,28 @@ def test_tcp_socket_all_hostname_resolution_failed
995995
RUBY
996996
end
997997

998+
def test_tcp_socket_hostname_resolution_failed_after_connection_failure
999+
opts = %w[-rsocket -W1]
1000+
assert_separately opts, <<~RUBY
1001+
server = TCPServer.new("127.0.0.1", 0)
1002+
port = server.connect_address.ip_port
1003+
1004+
Addrinfo.define_singleton_method(:getaddrinfo) do |_, _, family, *_|
1005+
case family
1006+
when Socket::AF_INET6 then sleep(0.1); raise Socket::ResolutionError
1007+
when Socket::AF_INET then [Addrinfo.tcp("127.0.0.1", port)]
1008+
end
1009+
end
1010+
1011+
server.close
1012+
1013+
# SystemCallError is a workaround for Windows environment
1014+
assert_raise(Errno::ECONNREFUSED, SystemCallError) do
1015+
Socket.tcp("localhost", port)
1016+
end
1017+
RUBY
1018+
end
1019+
9981020
def test_tcp_socket_v6_address_passed
9991021
opts = %w[-rsocket -W1]
10001022
assert_separately opts, <<~RUBY

‎test/socket/test_tcp.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ def test_initialize_with_hostname_resolution_failure_after_connection_failure
316316
port = server.connect_address.ip_port
317317
server.close
318318

319-
assert_raise(Socket::ResolutionError) do
319+
assert_raise(Errno::ECONNREFUSED) do
320320
TCPSocket.new(
321321
"localhost",
322322
port,

0 commit comments

Comments
 (0)