net-http icon indicating copy to clipboard operation
net-http copied to clipboard

Replace Timeout.timeout with TCPSocket.open(open_timeout:) when available

Open osyoyu opened this issue 4 months ago • 3 comments

Resolves https://github.com/ruby/net-http/issues/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.

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 }.

Changes in behavior

On timeout, the raised Net::OpenTimeout's message has slightly changed, and also carrys a Errno::ETIMEDOUT as its cause.

/home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1670:in 'Net::HTTP#connect': Failed to open TCP connection to example.com:80 (Connection timed out - user specified timeout) (Net::OpenTimeout)
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1636:in 'Net::HTTP#do_start'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1625:in 'Net::HTTP#start'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:2378:in 'Net::HTTP#request'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1993:in 'Net::HTTP#get'
	from nethttptest.rb:13:in '<main>'
/home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1660:in 'TCPSocket#initialize': Connection timed out - user specified timeout (Errno::ETIMEDOUT)
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1660:in 'IO.open'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1660:in 'Net::HTTP#connect'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1636:in 'Net::HTTP#do_start'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1625:in 'Net::HTTP#start'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:2378:in 'Net::HTTP#request'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1993:in 'Net::HTTP#get'
	from nethttptest.rb:13:in '<main>'

Previously, it looked like this.

/home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1665:in 'TCPSocket#initialize': Failed to open TCP connection to example.com:80 (execution expired) (Net::OpenTimeout)
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1665:in 'IO.open'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1665:in 'block in Net::HTTP#connect'
	from /home/osyoyu/.rbenv/versions/3.4.5/lib/ruby/3.4.0/timeout.rb:185:in 'block in Timeout.timeout'
	from /home/osyoyu/.rbenv/versions/3.4.5/lib/ruby/3.4.0/timeout.rb:192:in 'Timeout.timeout'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1664:in 'Net::HTTP#connect'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1636:in 'Net::HTTP#do_start'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1625:in 'Net::HTTP#start'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:2378:in 'Net::HTTP#request'
	from /home/osyoyu/Development/rubyorg/net-http/lib/net/http.rb:1993:in 'Net::HTTP#get'
	from nethttptest.rb:13:in '<main>'

osyoyu avatar Jul 16 '25 15:07 osyoyu

~This version needs https://github.com/ruby/ruby/pull/13909.~ It's now merged!

osyoyu avatar Jul 16 '25 15:07 osyoyu

Here's what we've found so far:

  • TCPSocket.method(:open).parameters returns [[:rest]], so it cannot be used to determine if the open_timeout option is accepted.
  • While it's possible to call TCPSocket.open directly and branch the logic by rescuing ArgumentError, this approach makes the code somewhat complex.

Given this, I'd like to propose an alternative: What if we check the parameters of Socket.tcp to determine if TCPSocket.open accepts open_timeout?

Socket.tcp is closely related to TCPSocket.open; for instance, it was mentioned when the open_timeout argument was originally added to TCPSocket.open ( https://github.com/ruby/ruby/pull/13909 ).

Strictly speaking, a situation could exist where Socket.tcp accepts open_timeout but TCPSocket.open does not. However, looking at their relationship in the codebase, I believe they are tightly coupled and can be treated as effectively inseparable (i.e., they are likely to change together).

I'm thinking the code would look something like this:

      # Check if TCPSocket.open supports the open_timeout keyword argument.
      # We cannot use TCPSocket.method(:open).parameters directly because it always returns [[:rest]] due to being a C extension method.
      # Instead, we use Socket.tcp as a substitute since it's closely related (see PR https://github.com/ruby/ruby/pull/13909).
      # Socket.tcp is implemented in Ruby, so keyword arguments appear in its parameters.
      open_timeout_supported = Socket.method(:tcp).parameters.any? { |param| param[0] == :key && param[1] == :open_timeout }
      s = begin
            if open_timeout_supported
              TCPSocket.open(conn_addr, conn_port, @local_host, @local_port, open_timeout: @open_timeout)
            else
              Timeout.timeout(@open_timeout, Net::OpenTimeout) {
                TCPSocket.open(conn_addr, conn_port, @local_host, @local_port)
              }
            end
          rescue => e
              e = Net::OpenTimeout.new(e) if e.is_a?(Errno::ETIMEDOUT) # for compatibility with previous versions
              raise e, "Failed to open TCP connection to " +
                "#{conn_addr}:#{conn_port} (#{e.message})"
          end

niku avatar Oct 31 '25 13:10 niku

What if we check the parameters of Socket.tcp to determine if TCPSocket.open accepts open_timeout?

~~Actually I've never thought of that. This sounds like a good idea, if replacing TCPSocket.new with Socket.new is acceptable in terms of performance. My understanding is that Socket.new is slower than TCPSocket.new in theory since it is implemented in Ruby instead of C, but I am not sure if that is neglibigle or not for net/http.~~

Edit: I have misunderstood your proposal. I'm not sure if we can take chances here and assume that "if Socket.tcp supports open_timeout, TCPSocket.open will also do so" -- if that assumption breaks somewhere, net/http will raise an ArgumentError that is unfixable on the user's side. If we're to determine open_timeout availability via Socket.method(:tcp).parameters, we'd better directly use Socket.tcp.

Edit 2: I've obviously thought of the latter idea in https://github.com/ruby/net-http/pull/223 🤫

osyoyu avatar Nov 02 '25 12:11 osyoyu

Had a chance to chat with @shioimm regarding my previous comment and #223. She advised not to use Socket.tcp for net/http for performance reasons. (I don't have any numbers at hand, but maybe I can run some benchmarks later)

osyoyu avatar Nov 07 '25 00:11 osyoyu

We've identified several options at this point. To make them easier to review, I've compiled them into a table.

Method Advantages (Pros) Disadvantages (Cons) Notes
Check the result of TCPSocket.method(:open).parameters Most direct It's unknown/difficult to get a result more specific than [[:rest]] from a C extension. https://github.com/ruby/net-http/pull/224#discussion_r2217063020
Try calling TCPSocket.open with open_timeout and fallback on ArgumentError The check (if it can run) and the actual execution method are identical. There are no other considerations. Requires code to catch the ArgumentError and perform the fallback. this PR
Assume TCPSocket.method(:open) accepts open_timeout if Socket.method(:tcp).parameters does Keeps code complexity low. It's not intuitive why this workaround is used. / There is no guarantee this assumption is always true. https://github.com/ruby/net-http/pull/224#issuecomment-3473072497
Use Socket.tcp Keeps code complexity low. / The check and the method to be executed are identical, with no other considerations. Poor performance. https://github.com/ruby/net-http/pull/224#issuecomment-3500000909
Check the Ruby version (RUBY_VERSION) Simple May not work correctly on implementations other than CRuby. / Will fail if open_timeout is removed for some reason in a future version (e.g., Ruby 3.5+). https://github.com/ruby/net-http/pull/224#discussion_r2411676663 https://github.com/ruby/net-http/pull/224#discussion_r2500181341

At this point, this PR's changes seem to be the most suitable for me 👍

niku avatar Nov 07 '25 23:11 niku

I also support the approach of calling TCPSocket.open with open_timeout, and raising an ArgumentError if it’s not available. This makes the intention clear and ensures the behavior is independent of the Ruby implementation. For these reasons, I will merge this PR.

shioimm avatar Nov 10 '25 02:11 shioimm

Thank you everyone for comments & reviews!

osyoyu avatar Nov 10 '25 08:11 osyoyu

Acknowledging extensive discussions and changes around TCPSocket timeout. Is the exception expected behavior (in given configurations like Ruby < 3.5) and what is the recommended mitigation on the developer side?

# ruby 3.4.7 (2025-10-08 revision 7a5688e2a2)
# net-http (0.8.0, 0.6.0)
irb> Net::HTTP.start('foo.bar', 80)
/opt/homebrew/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1682:in 'TCPSocket#initialize': unknown keyword: :open_timeout (ArgumentError)

forthrin avatar Nov 19 '25 08:11 forthrin

Oh, definitely not expected. I'll take a look. I think there is no workaround besides downgrading net-http.

osyoyu avatar Nov 19 '25 08:11 osyoyu

@forthrin Are you pointing to the ArgumentError wrapped in the Socket::ResolutionError? In that case, this behavior is expected (I understand it's a bit cluttered). You should be able to rescue this as rescue Socket::ResolutionError. Or are you seeing a ArgumentError at the top level?

% irb
irb(main):001> RUBY_VERSION
=> "3.4.7"
irb(main):002> require 'net/http'
=> true
irb(main):003> Net::HTTP::VERSION
=> "0.8.0"
irb(main):004> Net::HTTP.start('foo.bar', 80)
/Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1691:in 'TCPSocket#initialize': Failed to open TCP connection to foo.bar:80 (getaddrinfo(3): nodename no
r servname provided, or not known) (Socket::ResolutionError)
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1691:in 'IO.open'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1691:in 'block in Net::HTTP#connect'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/timeout-0.4.4/lib/timeout.rb:188:in 'block in Timeout.timeout'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/timeout-0.4.4/lib/timeout.rb:195:in 'Timeout.timeout'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1690:in 'Net::HTTP#connect'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1655:in 'Net::HTTP#do_start'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1635:in 'Net::HTTP#start'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1064:in 'Net::HTTP.start'
        from (irb):4:in '<main>'
        from <internal:kernel>:168:in 'Kernel#loop'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/irb-1.15.3/exe/irb:9:in '<top (required)>'
        from /Users/osyoyu/.rbenv/versions/3.4.7/bin/irb:25:in 'Kernel#load'
        from /Users/osyoyu/.rbenv/versions/3.4.7/bin/irb:25:in '<main>'
/Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1682:in 'TCPSocket#initialize': unknown keyword: :open_timeout (ArgumentError)

              sock = TCPSocket.open(conn_addr, conn_port, @local_host, @local_port, open_timeout: @open_timeout)
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1682:in 'IO.open'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1682:in 'Net::HTTP#connect'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1655:in 'Net::HTTP#do_start'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1635:in 'Net::HTTP#start'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/net-http-0.8.0/lib/net/http.rb:1064:in 'Net::HTTP.start'
        from (irb):4:in '<main>'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/irb-1.15.3/lib/irb/workspace.rb:101:in 'Kernel#eval'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/irb-1.15.3/lib/irb/workspace.rb:101:in 'IRB::WorkSpace#evaluate'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/irb-1.15.3/lib/irb/context.rb:591:in 'IRB::Context#evaluate_expression'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/irb-1.15.3/lib/irb/context.rb:557:in 'IRB::Context#evaluate'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/irb-1.15.3/lib/irb.rb:202:in 'block (2 levels) in IRB::Irb#eval_input'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/irb-1.15.3/lib/irb.rb:521:in 'IRB::Irb#signal_status'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/irb-1.15.3/lib/irb.rb:194:in 'block in IRB::Irb#eval_input'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/irb-1.15.3/lib/irb.rb:281:in 'block in IRB::Irb#each_top_level_statement'
        from <internal:kernel>:168:in 'Kernel#loop'
        from /Users/osyoyu/.rbenv/versions/3.4.7/lib/ruby/gems/3.4.0/gems/irb-1.15.3/lib/irb.rb:278:in 'IRB::Irb#each_top_level_statement'
        ... 8 levels...

osyoyu avatar Nov 19 '25 08:11 osyoyu

@osyoyu: Same as you. First Socket::ResolutionError, then unknown keyword: :open_timeout (ArgumentError).

So yes, catching the prior would obviously eliminate the latter, but what is the purpose of raising the latter? If it's not supposed to be addressable by a developer at the far end, but more seems like an internal fixture of sorts.

Is it a temporary measure until another project (Ruby core / TCPSocket) fix things on their side?

forthrin avatar Nov 19 '25 09:11 forthrin

Yes, it is internal. There simply wasn't a way to remove the wrapped exception, as far as I know. I'd be happy to do so if possible.

Is it a temporary measure until another project (Ruby core / TCPSocket) fix things on their side?

Once you upgrade to Ruby 4.0 (the next version), you won't see this as TCPSocket.open will accept open_timeout.

osyoyu avatar Nov 19 '25 09:11 osyoyu

There simply wasn't a way to remove the wrapped exception, as far as I know. I'd be happy to do so if possible.

I think it should be possible if you extract the new logic in a new method, and then on ArgumentError do a recursive call after @tcpsocket_supports_open_timeout = false, the call should clear $! since that's only visible inside the rescue.

eregon avatar Nov 19 '25 11:11 eregon

Re

It's unknown/difficult to get a result more specific than [[:rest]] from a C extension.

Maybe it would make sense to define TCPSocket.open in Ruby code in CRuby, then the detection would work fine. It can make sense on its own to be written in Ruby as that's typically a lot more efficient to pass keyword arguments than passing them to a method defined in C.

Or alternatively maybe there should be a way to declare proper parameters for methods defined in C in the Ruby C API.

For the first, I think a PR to CRuby would be most actionable, and for the second a ticket on https://bugs.ruby-lang.org/.

The current solution seems fine to me though, but it's a problem that has come up a few times so it'd be nice to get a proper fix in CRuby.

eregon avatar Nov 19 '25 11:11 eregon

Maybe it would make sense to define TCPSocket.open in Ruby code in CRuby

Do you mean reimplementing entire TCPSocket.open in Ruby? Socket.tcp is exactly that, so in that case it’d be better to replace TCPSocket with Socket (which is believed to have degraded performance).

I think it should be possible if you extract the new logic in a new method, and then on ArgumentError do a recursive call

Thank you! I’ll work on that.

osyoyu avatar Nov 19 '25 12:11 osyoyu

Is there any place where a changelog should be recorded? I can send a patch to CRuby’s NEWS if that’s the place.

osyoyu avatar Nov 19 '25 12:11 osyoyu

Do you mean reimplementing entire TCPSocket.open in Ruby?

No, just TCPSocket.open. Actually there is no TCPSocket.open, it's just IO.open. IO.open is trivial to implement in Ruby code. But it won't solve our detection issue here as it's fully generic and takes arbitrary args & kwargs :sweat_smile:

Socket.tcp is exactly that, so in that case it’d be better to replace TCPSocket with Socket (which is believed to have degraded performance).

I think it'd be good to actually benchmark this. It's not because something is partially written in Ruby that it's slower than C. In fact, receiving keyword arguments, accessing ivars, calling other methods are all faster when done in Ruby code than in C code (basically because Ruby code has inline caches and C code doesn't).

eregon avatar Nov 19 '25 12:11 eregon

Is there any place where a changelog should be recorded? I can send a patch to CRuby’s NEWS if that’s the place.

I think just the release notes of this gem is fine (and that's automatic).

eregon avatar Nov 19 '25 12:11 eregon

No, just TCPSocket.open. Actually there is no TCPSocket.open, it's just IO.open

I see (The docs are also confusing for this reason!)

I have some spare time tonight, so I’ll try some benchmarks.

osyoyu avatar Nov 19 '25 12:11 osyoyu

~~TCPSocket.open was slightly faster in most scenarios.~~ Maybe the difference is negligible?

See the following Gist for code and details: https://gist.github.com/osyoyu/7c85e8120facba8cbe5e6b45b9038e19

Open TCP socket and close

This benchmark aims to compare performance of TCPSocket.open and Socket.tcp by doing nothing else, aside of close.

Calculating -------------------------------------
TCPSocket  w/o options
                         36.378k (±59.1%) i/s   (27.49 μs/i) -     13.095k in   5.380034s
Socket.tcp w/o options
                         37.031k (±39.7%) i/s   (27.00 μs/i) -     12.173k in   5.426029s
TCPSocket  w/ options
                         44.877k (±29.4%) i/s   (22.28 μs/i) -     12.864k in   5.430528s
Socket.tcp w/ options
                         40.331k (±28.4%) i/s   (24.80 μs/i) -     13.300k in   5.434711s

Comparison:
TCPSocket  w/ options:    44877.0 i/s
Socket.tcp w/ options:    40330.6 i/s - same-ish: difference falls within error
Socket.tcp w/o options:    37030.7 i/s - same-ish: difference falls within error
TCPSocket  w/o options:    36378.1 i/s - same-ish: difference falls within error

Single HTTP request (small response)

Network I/O shall be dominant in this scenario.

Calculating -------------------------------------
           TCPSocket      2.270k (±70.1%) i/s  (440.53 μs/i) -      9.696k in   5.063461s
          Socket.tcp      1.824k (±22.9%) i/s  (548.22 μs/i) -      8.816k in   5.061455s

Comparison:
           TCPSocket:     2270.0 i/s
          Socket.tcp:     1824.1 i/s - same-ish: difference falls within error

Single HTTP request (10M response)

Network I/O shall be even more dominant.

Calculating -------------------------------------
           TCPSocket    188.905 (± 5.3%) i/s    (5.29 ms/i) -    950.000 in   5.047272s
          Socket.tcp    190.200 (± 3.2%) i/s    (5.26 ms/i) -    950.000 in   5.000060s

Comparison:
          Socket.tcp:      190.2 i/s
           TCPSocket:      188.9 i/s - same-ish: difference falls within error

osyoyu avatar Nov 19 '25 14:11 osyoyu