Fix conflict with resolv-replace gem

[y9v]

Closes #987

In version 3.2.7 socket_timeout option was introduced for TCPSocket.

This works unless resolv-replace gem is loaded (which was added to ruby standard library since version 3.0.0).

This commit adds another check besides the ruby version check to avoid breaking dalli for applications that have resolv-replace gem required.

[y9v]

I'm not sure this is possible to test without requiring resolv-replace in test.

And it looks like resolv-replace will be removed from standard library in Ruby 3.4.0:

irb(Dalli::Socket::TCP):001> ::TCPSocket.private_instance_methods.include?(:original_resolv_initialize)
=> false
irb(Dalli::Socket::TCP):002> require 'resolv-replace'
/Users/yury.lebedev@carwow.de/development/personal/os/dalli/lib/dalli/socket.rb:2: warning: resolv-replace was loaded from the standard library, but will no longer be part of the default gems since Ruby 3.4.0. Add resolv-replace to your Gemfile or gemspec.

=> true
irb(Dalli::Socket::TCP):003> ::TCPSocket.private_instance_methods.include?(:original_resolv_initialize)
=> true

[petergoldstein]

@y9v Given the very specific nature of this condition, can you please add a comment to the code explaining it? Thanks.

[y9v]

@y9v Given the very specific nature of this condition, can you please add a comment to the code explaining it? Thanks.

Added a comment in f8743b3

[petergoldstein]

Thanks @y9v

[y9v]

@petergoldstein thank you!

[nirvdrum]

@petergoldstein @y9v This is very similar to the problem with released versions of TruffleRuby. I had already been working on a PR to fix that when this one landed. If I add yet another check for TruffleRuby here the code is going to start looking a little long in the tooth.

Assuming you want to continue using connect_timeout, would you be open to feature-detection? Something like:

HAS_CONNECT_TIMEOUT = RUBY_VERSION >= '3.0' && TCPSocket.instance_method(:initialize).parameters.include?(%i[key connect_timeout])

if HAS_CONNECT_TIMEOUT
  # use TCPSocket with :connect_timeout
else
  # use Timeout.timeout around TCPSocket
end

In my PR I was trying to limit that check to TruffleRuby so CRuby doesn't have to pay the cost of the method inspection, but it looks like we need to do something on CRuby anyway due to resolv-replace. I think the feature inspection approach addresses the resolv-replace case as well as currently released versions of TruffleRuby.

[y9v]

@nirvdrum I personally like the idea, but in this particular case a constant wouldn't work, since we don't have a guarantee that dalli will be required after resolv-replace gem. Setting the constant to a lambda function that checks conditions and returns a boolean might work though.

[y9v]

@nirvdrum I think I should have linked this PR, where I had to move this feature detection inside the Dalli::Socket::TCP.open method.

[eregon]

(I'm following this since it affects TruffleRuby compatibility)

RUBY_VERSION >= '3.0' && !::TCPSocket.private_instance_methods.include? seems quite expensive to check on every use.

I would like to suggest to either simply always use Timeout.timeout to simplify (timeout 0.3.0+ is pretty efficient and uses a single background thread, default on 3.2+, can be installed on older Rubies).

Or alternatively then try to pass the connect_timeout: kwarg, and if that raises (I checked, CRuby 2.7, TruffleRuby 23.1 and resolv-replace all raise a TypeError for TCPSocket.new("::1", 80, connect_timeout: 1)) remember it via an ivar on the class and don't try the kwarg again if that ivar is set.

[eregon]

I found the original PR adding connect_timeout: #977
I'm not sure why it was added, maybe because it's thought to be more efficient? @mlarraz might know. If it's for performance it seems worth to benchmark given the complications this implies.
In general it feels brittle to set individual timeouts vs using Timeout.timeout as it could easily miss something (OTOH Timeout.timeout has its own problems due to using Thread#raise).

Also there is another problem in that PR: https://github.com/petergoldstein/dalli/pull/977/files#r1482024393.
So maybe the safest would be to revert these timeout changes as they seem to cause more problems than they solve.

[eregon]

One last comment for tonight, the resolv-replace issue was filed as ruby/resolv-replace#2, great.
A third approach would be to use Timeout.timeout always until a Ruby release comes with a resolv-replace version handling that correctly.

[nirvdrum]

@petergoldstein Do you have thoughts on this discussion? I'm willing to do the work to clean things up but wanted to make sure I was solving it in an acceptable way.

[petergoldstein]

@nirvdrum At this point my inclination is to revert the changes wholesale. I merged them in the first place because they were a better match for the actual (C)Ruby socket API since 3.0, as I read the intention of that API. But the level of complexity necessary to deal with incompatibilities - resolv-replace, socksify, TruffleRuby, indicates that it simply may not be worth it. And I am concerned about a potential performance hit.