Skip to content

Use kwargs with resolv-replace gem version >= 0.2.0#1096

Merged
petergoldstein merged 2 commits into
petergoldstein:mainfrom
jasonpenny:resolv-replace-use-kwargs
May 14, 2026
Merged

Use kwargs with resolv-replace gem version >= 0.2.0#1096
petergoldstein merged 2 commits into
petergoldstein:mainfrom
jasonpenny:resolv-replace-use-kwargs

Conversation

@jasonpenny
Copy link
Copy Markdown
Contributor

When upgrading from v2 to latest, I hit a lot of errors about ruby/3.3.0/gems/timeout-0.4.3/lib/timeout.rb:95:in 'new': can't alloc thread (ThreadError) in a project that is using the resolv-replace bundled gem.

resolv-replace v0.2.0 updated its TCPSocket#initialize patch to forward **kwargs, which means connect_timeout: should now work correctly when that version is loaded. Previously, supports_connect_timeout? treated any resolv-replace-patched TCPSocket as incompatible.

What changed

supports_connect_timeout? now checks the parameter signature of TCPSocket#initialize more precisely instead of requiring an exact match against the native [[:rest]] signature:

  • Unpatched Ruby 3.0+ – parameters are [[:rest]], returns true (unchanged)
  • resolv-replace ≥ 0.2.0 – parameters include **kwargs (a :keyrest entry), returns true (new)
  • resolv-replace < 0.2.0 / socksify / other patchers without kwargs – no :keyrest entry, returns false (unchanged)

This PR was generated with the assistance of claude code

Copy link
Copy Markdown

@mattwd7 mattwd7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 🙏 🙏

@petergoldstein
Copy link
Copy Markdown
Owner

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

@petergoldstein
Copy link
Copy Markdown
Owner

The JRuby CI failure on this PR is caused by a pre-existing issue that your change exposes more broadly: JRuby 10.1.0.0 (Ruby 4.0 compat) does not support TCPSocket.instance_method because JRuby's TCPSocket is Java-backed and doesn't expose Ruby method introspection.

Error:

NoMethodError: undefined method 'instance_method' for class TCPSocket
    lib/dalli/socket.rb:116:in 'supports_connect_timeout?'

The fix (being applied to main separately) is to guard with RUBY_ENGINE == 'ruby' — consistent with the test that already has skip 'MRI-specific test' if RUBY_ENGINE != 'ruby':

@supports_connect_timeout = RUBY_ENGINE == 'ruby' && RUBY_VERSION >= '3.0' &&
                            (params == TCPSOCKET_NATIVE_PARAMETERS || params.any? { |type, _| type == :keyrest })

This PR will need to incorporate that guard before it can merge. On JRuby, supports_connect_timeout? will return false and Dalli will fall back to Timeout.timeout, which is the correct behavior since connect_timeout: is MRI-specific functionality.

🤖 Generated with Claude Code

@petergoldstein
Copy link
Copy Markdown
Owner

@jasonpenny can you please rebase against main and make any other changes that may be required per my last comment? Thanks.

@jasonpenny
Copy link
Copy Markdown
Contributor Author

@jasonpenny can you please rebase against main and make any other changes that may be required per my last comment? Thanks.

did you push a commit? I don't see it in GitHub (so this is still branched off of main)
I can apply the RUBY_ENGINE == 'ruby' update, but it sounds like you're going to do that separately?

@petergoldstein
Copy link
Copy Markdown
Owner

@jasonpenny Sorry, Claude didn't do the push - fixed now. Can you please rebase now and resolve? Thanks

@jasonpenny jasonpenny force-pushed the resolv-replace-use-kwargs branch from 58761ab to f1b51d5 Compare May 14, 2026 14:37
@jasonpenny
Copy link
Copy Markdown
Contributor Author

rebased, and I believe I resolved the RUBY_ENGINE issue

@petergoldstein petergoldstein merged commit 0dd2a63 into petergoldstein:main May 14, 2026
10 checks passed
@petergoldstein
Copy link
Copy Markdown
Owner

Thanks!

@mattwd7
Copy link
Copy Markdown

mattwd7 commented May 15, 2026

@petergoldstein can we anticipate a 5.0.3 release anytime soon incorporating these changes?

@petergoldstein petergoldstein mentioned this pull request May 15, 2026
2 tasks
@petergoldstein
Copy link
Copy Markdown
Owner

@mattwd7 Released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants