We were seeing segfaults when calling `pthread_detach`. Apparently in
some versions of glibc there is a race between when this is called
(usually right after starting a thread) and a short-lived thread's
shutdown routine. The bug has been reported to glibc:
https://sourceware.org/bugzilla/show_bug.cgi?id=19951
I haven't been able to reproduce it on my Linux desktop but apparently
it's easier to reproduce on certain kinds of servers.
As a workaround, we can set the thread's detach state before thread
start. I don't know of a platform that doesn't have
`pthread_attr_setdetachstate`, but to be safe we check for it in
`extconf.rb` and use `pthread_detach` as a backup if it isn't available.
Fixes [Bug #21679]
Stop documentation for undocumented private constants and private
class methods. And align private method definition styles.
Also `Socket.tcp_with_fast_fallback` looks private as well as
`Socket.tcp_without_fast_fallback`.
On OpenBSD:
```
ld: warning: namespace.c:731(namespace.o:(rb_namespace_local_extension)): warning: strcpy() is almost always misused, please use strlcpy()
```
When calling getnameinfo we spawn a thread because it may do a slow,
blocking reverse-DNS lookup. Spawning a thread is relatively fast (~20µs
on my Linux machine) but still an order of magnitude slower than when
getnameinfo is simply translating to a numeric IP or port, which, at
least in my tests on Linux, doesn't even make a syscall.
This commit adds a fast path for when reverse DNS isn't required: either
host isn't being fetched or NI_NUMERICHOST is set AND either the
service name isn't required or NI_NUMERICSERV is set. The service name
should only need to read /etc/services, which should be fast-ish, but
is still I/O so I kept the existing behaviour (it could be on a network
fs I guess).
I tested with:
s = TCPSocket.open("www.ruby-lang.org", 80)
500_000.times { Socket.unpack_sockaddr_in(s.getpeername) }
Before: 12.935s
After: 0.338s
This suppresses this warning:
../../../ext/socket/ipsocket.c: In function ‘rsock_raise_user_specified_timeout’:
../../../ext/socket/ipsocket.c:30:1: warning: function might be candidate for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn]
30 | rsock_raise_user_specified_timeout()
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
* [Feature #21347] Add `open_timeout` as an overall timeout option for `TCPSocket.new`
With this change, `TCPSocket.new` now accepts the `open_timeout` option.
This option raises an exception if the specified number of seconds has elapsed since the start of the method call,
even if the operation is still in the middle of name resolution or connection attempts.
The addition of this option follows the same intent as the previously merged change to `Socket.tcp`.
[Feature #21347](https://bugs.ruby-lang.org/issues/21347)
https://github.com/ruby/ruby/pull/13368
* Tidy up: Extract rsock_raise_user_specified_timeout()
* Added a note to the documentation of `Socket.tcp`
* Fix `rsock_init_inetsock` for `FAST_FALLBACK_INIT_INETSOCK_IMPL`
Socket.tcp_with_fast_fallback: Pass proper addr family to getaddrinfo
Addrinfo.getaddrinfo expects Socket::AF_INET or Socket::AF_INET6 as its
third argument (family). However Socket.tcp_with_fast_fallback was
incorrectly passing :ipv4 or :ipv6.
Repro:
require 'socket'
Socket.tcp_with_fast_fallback('example.com', 80, '127.0.0.1')
Expected behavior: Returns a Socket object
Actual: Raises unknown socket domain: ipv4 (SocketError)
This change addresses the following ASAN error:
```
==1973462==ERROR: AddressSanitizer: heap-use-after-free on address 0x5110002117dc at pc 0x749c307c8a65 bp 0x7ffc3af331d0 sp 0x7ffc3af331c8
READ of size 4 at 0x5110002117dc thread T0
#0 0x749c307c8a64 in rb_getaddrinfo /tmp/ruby/src/trunk_asan/ext/socket/raddrinfo.c:564:14
#1 0x749c307c8a64 in rsock_getaddrinfo /tmp/ruby/src/trunk_asan/ext/socket/raddrinfo.c:1008:21
#2 0x749c307cac48 in rsock_addrinfo /tmp/ruby/src/trunk_asan/ext/socket/raddrinfo.c:1049:12
#3 0x749c307b10ae in init_inetsock_internal /tmp/ruby/src/trunk_asan/ext/socket/ipsocket.c:62:23
#4 0x562c5b2e327e in rb_ensure /tmp/ruby/src/trunk_asan/eval.c:1080:18
#5 0x749c307aafd4 in rsock_init_inetsock /tmp/ruby/src/trunk_asan/ext/socket/ipsocket.c:1318:12
#6 0x749c307b3b78 in tcp_svr_init /tmp/ruby/src/trunk_asan/ext/socket/tcpserver.c:39:12
```
Fixed to avoid accessing memory that has already been freed after calling `free_getaddrinfo_arg`.
[Bug #21506] Fix timeout in Addrinfo.getaddrinfo to actually take effect
This change fixes an issue where the timeout option in `Addrinfo.getaddrinfo` was not functioning as expected.
It also addresses a related issue where specifying `fast_fallback: false` with `resolv_timeout` for `Socket.tcp` and`TCPSocket.new` would have no effect.
The timeout option was originally introduced in:
6382f5cc91
However, the value was noy used in current implementation:
3f0e0d5c8b/ext/socket/raddrinfo.c (L1282-1308)
Therefore, even if a timeout is specified and the duration elapses during name resolution, nothing happens. This is clearly not the intended behavior.
`Addrinfo.getaddrinfo` has been made interruptible as of Feature #19965.
This change uses that feature to interrupt name resolution when the specified timeout period elapses, raising a user-specified timeout error.
The timeout can be specified in milliseconds.
The same issue affects `Socket.tcp` and `TCPSocket.new` when `resolv_timeout` is set along with `fast_fallback: false`.
`resolv_timeout` was introduced in the following commits:
6382f5cc91511fe23fa2
The reason is that with `fast_fallback: false`, these methods internally call the same `rsock_getaddrinfo()` as `Addrinfo.getaddrinfo`.
This change addresses that as well.
otherwise, gcc 15 will complain:
> init.c:573:19: error: too many arguments to function ‘Rconnect’; expected 0, have 3
> 573 | return (VALUE)Rconnect(arg->fd, arg->sockaddr, arg->len);
> | ^~~~~~~~ ~~~~~~~
> In file included from init.c:11:
> rubysocket.h:294:5: note: declared here
> 294 | int Rconnect();
> | ^~~~~~~~
> sockssocket.c:33:9: error: too many arguments to function ‘SOCKSinit’; expected 0, have 1
> 33 | SOCKSinit("ruby");
> | ^~~~~~~~~ ~~~~~~
> In file included from sockssocket.c:11:
> rubysocket.h:293:6: note: declared here
> 293 | void SOCKSinit();
> | ^~~~~~~~~
Signed-off-by: Z. Liu <zhixu.liu@gmail.com>
* Add `open_timeout` as an overall timeout option for `Socket.tcp`
[Background]
Currently, `TCPSocket.new` and `Socket.tcp` accept two kind of timeout options:
- `resolv_timeout`, which controls the timeout for DNS resolution
- `connect_timeout`, which controls the timeout for the connection attempt
With the introduction of Happy Eyeballs Version 2 (as per [RFC 8305](https://datatracker.ietf.org/doc/html/rfc8305)) in[ Feature #20108](https://bugs.ruby-lang.org/issues/20108) and [Feature #20782](https://bugs.ruby-lang.org/issues/20782), both address resolution and connection attempts are now parallelized.
As a result, the sum of `resolv_timeout` and `connect_timeout` no longer represents the total timeout duration. This is because, in HEv2, name resolution and connection attempts are performed concurrently, causing the two timeouts to overlap.
Example:
When `resolv_timeout: 200ms` and `connect_timeout: 100ms` are set:
1. An IPv6 address is resolved after the method starts immediately (IPv4 is still being resolved).
2. A connection attempt is initiated to the IPv6 address
3. After 100ms, `connect_timeout` is exceeded. However, since `resolv_timeout` still has 100ms left, the IPv4 resolution continues.
4. After 200ms from the start, the method raises a `resolv_timeout` error.
In this case, the total elapsed time before a timeout is 200ms, not the expected 300ms (100ms + 200ms).
Furthermore, in HEv2, connection attempts are also parallelized.
It starts a new connection attempts every 250ms for resolved addresses. This makes the definition of `connect_timeout` even more ambiguous—specifically, it becomes unclear from which point the timeout is counted.
Additionally, these methods initiate new connection attempts every 250ms (Connection Attempt Delay) for each candidate address, thereby parallelizing connection attempts. However, this behavior makes it unclear from which point in time the connect_timeout is actually measured.
Currently, a `connect_timeout` is raised only after the last connection attempt exceeds the timeout.
Example:
When `connect_timeout: 100ms` is set and 3 address candidates:
1. Start a connection attempt to the address `a`
2. 250ms after step 1, start a new connection attempt to the address `b`
3. 500ms after step 1, start a new connection attempt to the address `c`
4. 1000ms after step 3 (1000ms after starting the connection to `c`, 1250ms after starting the connection to `b,` and 1500ms after starting the connection to `a`) `connect_timeout` is raised
This behavior aims to favor successful connections by allowing more time for each attempt, but it results in a timeout model that is difficult to reason about.
These methods have supported `resolv_timeout` and `connect_timeout` options even before the introduction of HEv2. However, in many use cases, it would be more convenient if a timeout occurred after a specified duration from the start of the method. Similar functions in other languages (such as PHP, Python, and Go) typically allow specifying only an overall timeout.
[Proposal]
I propose adding an `open_timeout` option to `Socket.tcp` in this PR, which triggers a timeout after a specified duration has elapsed from the start of the method.
The name `open_timeout` aligns with the existing accessor used in `Net::HTTP`.
If `open_timeout` is specified together with `resolv_timeout` and `connect_timeout`, I propose that only `open_timeout` be used and the others be ignored. While it is possible to support combinations of `open_timeout`, `resolv_timeout`, and `connect_timeout`, doing so would require defining which timeout takes precedence in which situations. In this case, I believe it is more valuable to keep the behavior simple and easy to understand, rather than supporting more complex use cases.
If this proposal is accepted, I also plan to extend `open_timeout` support to `TCPSocket.new`.
While the long-term future of `resolv_timeout` and `connect_timeout` may warrant further discussion, I believe the immediate priority is to offer a straightforward way to specify an overall timeout.
[Outcome]
If `open_timeout` is also supported by `TCPSocket.new`, users would be able to manage total connection timeouts directly in `Net::HTTP#connect` without relying on `Timeout.timeout`.
aa0f689bf4/lib/net/http.rb (L1657)
---
* Raise an exception if it is specified together with other timeout options
> If open_timeout is specified together with resolv_timeout and connect_timeout, I propose that only open_timeout be used and the others be ignored.
Since this approach may be unclear to users, I’ve decided to explicitly raise an `ArgumentError` if these options are specified together.
* Add doc
* Fix: open_timeout error should be raised even if there are still addresses that have not been tried
`rb_addrinfo_t` has `VALUE inspectname` and `VALUE canonname`, so this
triggers the write barrier for those. The `AddrInfo` wasn't readily
available where we need to call `RB_OBJ_WRITE`, so this involves passing
`self` around a bit.
Allow Addrinfo objects to be shared among Ractors. Addrinfo objects are
already immutable, so I think it's safe for us to tag them as
RUBY_TYPED_FROZEN_SHAREABLE shareable too.
This change addresses the following ASAN error:
```
==36597==ERROR: AddressSanitizer: heap-use-after-free on address 0x512000396ba8 at pc 0x7fcad5cbad9f bp 0x7fff19739af0 sp 0x7fff19739ae8
WRITE of size 8 at 0x512000396ba8 thread T0
[643/756] 36600=optparse/test_summary
#0 0x7fcad5cbad9e in free_fast_fallback_getaddrinfo_entry /home/runner/work/ruby-dev-builder/ruby-dev-builder/ext/socket/raddrinfo.c:3046:22
#1 0x7fcad5c9fb48 in fast_fallback_inetsock_cleanup /home/runner/work/ruby-dev-builder/ruby-dev-builder/ext/socket/ipsocket.c:1179:17
#2 0x7fcadf3b611a in rb_ensure /home/runner/work/ruby-dev-builder/ruby-dev-builder/eval.c:1081:5
#3 0x7fcad5c9b44b in rsock_init_inetsock /home/runner/work/ruby-dev-builder/ruby-dev-builder/ext/socket/ipsocket.c:1289:20
#4 0x7fcad5ca22b8 in tcp_init /home/runner/work/ruby-dev-builder/ruby-dev-builder/ext/socket/tcpsocket.c:76:12
#5 0x7fcadf83ba70 in vm_call0_cfunc_with_frame /home/runner/work/ruby-dev-builder/ruby-dev-builder/./vm_eval.c:164:15
...
```
A `struct fast_fallback_getaddrinfo_shared` is shared between the main thread and two child threads.
This struct contains an array of `fast_fallback_getaddrinfo_entry`.
`fast_fallback_getaddrinfo_entry` and `fast_fallback_getaddrinfo_shared` were freed separately, and if `fast_fallback_getaddrinfo_shared` was freed first and then an attempt was made to free a `fast_fallback_getaddrinfo_entry`, a `heap-use-after-free` could occur.
This change avoids that possibility by separating the deallocation of the addrinfo memory held by `fast_fallback_getaddrinfo_entry` from the access and lifecycle of the `fast_fallback_getaddrinfo_entry` itself.
[Bug #21211]
Socket errors raised from background threads are hard to track down because
their backtrace starts from the spawned thread.
To solve this we can raise a new error with the old one as `cause`.
[Bug #21179]
```
socket.rb:1046:in 'Socket::HostnameResolutionStore#get_addrinfo': can not access non-shareable objects in constant
Socket::HostnameResolutionStore::PRIORITY_ON_V6 by non-main ractor. (Ractor::IsolationError)
from socket.rb:724:in 'block in Socket.tcp_with_fast_fallback'
from socket.rb:720:in 'Socket.tcp_with_fast_fallback'
```
* 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
Rarely, there are cases where a write(2) call from a child thread
to notify the main thread of the completion of name resolution fails.
If this happens while the main thread is waiting in `rb_thread_fd_select`,
rb_thread_fd_select may not notice that the name resolution has completed and end up hanging.
This issue becomes a problem when there are no sockets currently being connected,
no addresses ready for immediate connection attempts,
and name resolution has already completed for one address family
while the main thread is waiting for the name resolution of the other address family.
(If name resolution is not completed for either address family,
the chances of write(2) failing in both child threads are likely low.)
To avoid this issue, a timeout is introduced to rb_thread_fd_select under the above conditions.
This way, even if the issue occurs,
the completion of name resolution should still be detected
in the subsequent `if (!resolution_store.is_all_finished) ...` block.
Wrap `do_fast_fallback_getaddrinfo` with `rb_thread_prevent_fork`
Referencing PR #10864,
wrap `do_fast_fallback_getaddrinfo` with `rb_thread_prevent_fork`
to avoid fork safety issues.
`do_fast_fallback_getaddrinfo` internally uses getaddrinfo(3),
leading to fork safety issues, as described in PR #10864.
This change ensures that `do_fast_fallback_getaddrinfo`
is guarded by `rb_thread_prevent_fork`,
preventing fork during its execution and avoiding related issues.
This change includes the following updates:
- Added an environment variable `RUBY_TCP_NO_FAST_FALLBACK` to control enabling/disabling fast_fallback
- Updated documentation and man pages
- Revised the implementation of Socket.tcp_fast_fallback= and Socket.tcp_fast_fallback, which previously performed dynamic name resolution of constants and variables. As a result, the following performance improvements were achieved:
(Case of 1000 executions of `TCPSocket.new` to the local host)
Rehearsal -----------------------------------------
before 0.031462 0.147946 0.179408 ( 0.249279)
after 0.031164 0.146839 0.178003 ( 0.346935)
-------------------------------- total: 0.178003sec
user system total real
before 0.027584 0.138712 0.166296 ( 0.233356)
after 0.025953 0.127608 0.153561 ( 0.237971)
Any memory allocated with xmalloc needs to be matched with xfree rather
than plain free.
Ruby unfortunately redefines strdup to be ruby_strdup, which uses
xmalloc so needs to be xfreed. Previously these were mismatched.
This commit changes the copy to be an explicit ruby_strdup (to avoid
confusion) and the free to be xfree.
* Use `rb_thread_fd_select` instead of select(2)
For fixing https://bugs.ruby-lang.org/issues/20932 .
`TCPSocket.new`, which internally uses select(2) for HEv2, can cause SEGV if the number of file descriptors exceeds `FD_SETSIZE`.
This change avoids that issue by replacing select(2) with `rb_thread_fd_select`, which is provided as part of Ruby's internal API.
---
This includes the following changes.
* rb_thread_fd_select does not need common pipe
The following two commits fix the proper clearing of the Connection Attempt Delay in `TCPSocket.new`.
- b2f610b0ed
- 6f4efaec53
The same fix will be applied to `Socket.tcp`.
