Replace Timeout.timeout with TCPSocket.open(open_timeout:) when available #224
Conversation
…able This patch replaces the implementation of #open_timeout from Timeout.timeout from the builtin timeout in TCPSocket.open, which was introduced in Ruby 3.5 (https://bugs.ruby-lang.org/issues/21347). The builtin timeout in TCPSocket.open is better in several ways than Timeout.timeout. It does not rely on a separate Ruby Thread for monitoring Timeout (which is what the timeout library internally does). Furthermore, it is compatible with Ractors, as opposed to Timeout.timeout (it internally uses Thread::Mutex which can not be used in non-main Ractors). This change allows the following code to work. require 'net/http' Ractor.new { uri = URI('http://example.com/') http = Net::HTTP.new(uri.host, uri.port) http.open_timeout = 1 http.get(uri.path) }.value In Ruby <3.5 environments where `TCPSocket.open` does not have the `open_timeout` option, I have kept the behavior unchanged. net/http will use `Timeout.timeout { TCPSocket.open }`.
|
|
osyoyu
force-pushed
the
tcpsocket-open-timeout
branch
from
4666f41 to
728eb8f
Compare
| s = begin | ||
| # Use built-in timeout in TCPSocket.open if available | ||
| TCPSocket.open(conn_addr, conn_port, @local_host, @local_port, open_timeout: @open_timeout) | ||
| rescue ArgumentError => e |
There was a problem hiding this comment.
I couldn't come up with a better way to detect the absence of open_timeout: option. Is this a acceptable solution?
TCPSocket.open is basically (**args) from the perspective of Ruby, so Method#parameters wasn't an option:
irb(main):001> require 'socket'
=> true
irb(main):002> TCPSocket.method(:open).parameters
=> [[:rest]]
There was a problem hiding this comment.
I think it's fine, given this is only to keep things working for very old rubies, right, and the error message is not going to change for those rubies anyways.
There was a problem hiding this comment.
Actually it's not only very old Rubies, but even 3.4 raises on a TCPSocket.open call with open_timeout. It is true that the situation is different in Ruby 2.x, where keyword arguments were not a argument of its own kind (workaround in 09bf573).
There was a problem hiding this comment.
Oh, I thought your question was specifically for the Ruby 2.x workaround, since you need to parse a very generic error message, but I see how the detection for Ruby 3.4 is also brittle since it also involves parsing the error message, even if more specific. Unfortunately, I don't know of a better way, but I'd say this way is acceptable.
There was a problem hiding this comment.
Considering that every single new HTTP request is going to go through this raise/rescue flow on ruby < 3.5, and that exceptions are kind of expensive, personally I think a better option is just to have test for RUBY_VERSION.to_f < 3.5 directly to see if we use the old Timeout.timeout. I get that testing version numbers directly is a bit distasteful, but when there's actual performance issues and brittleness issues on the line... I'd say its merited
Resolves #6.
This patch replaces the implementation of #open_timeout from Timeout.timeout from the builtin timeout in TCPSocket.open, which was introduced in Ruby 3.5 (https://bugs.ruby-lang.org/issues/21347).
The builtin timeout in TCPSocket.open is better in several ways. First, it does not rely on a separate Ruby Thread for monitoring Timeout (which is what the timeout library internally does). Also, it is compatible with Ractors, since it does not rely on Mutexes (which is also what the timeout library does).
This change allows the following code to work.
In Ruby <3.5 environments where
TCPSocket.opendoes not have theopen_timeoutoption, I have kept the behavior unchanged. net/http will useTimeout.timeout { TCPSocket.open }.Changes in behavior
On timeout, the raised
Net::OpenTimeout's message has slightly changed, and also carrys aErrno::ETIMEDOUTas its cause.Previously, it looked like this.