Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Timeout.timeout in Net:HTTP#connect #10

Merged
merged 1 commit into from
Feb 15, 2021

Conversation

mohamedhafez
Copy link
Contributor

Use socket timeout option instead.
See #6 for discussion

@mohamedhafez
Copy link
Contributor Author

mohamedhafez commented Jan 16, 2021

Tests are already passing on Ruby 2.6 and 2.7, once #8 gets merged the 3.0 tests pass as well (i tested it locally to be sure).

@carlhoerberg
Copy link

You need to introduce a resolv_timeout, see https://bugs.ruby-lang.org/issues/16381 and ruby/ruby#1806

@mohamedhafez
Copy link
Contributor Author

@carlhoerberg agreed, but I think I'd like to get this PR accepted first and then follow up with another for adding an option for a :resolv_timeout. Or feel free to make ruby/ruby#1806 against this repo and I can take this PR down, to be honest until we hear from the maintainers I'm just worried this will be ignored and am hesitant to put any more effort into it.

Use Socket.tcp's connect_timeout option instead
@mohamedhafez
Copy link
Contributor Author

All the tests are passing now that #8 got merged:)

Copy link

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

This changes seems a very good move to me!

@PhilCoggins
Copy link

When is this change expected to make it into Ruby stdlib? Ruby 3.0.1 and 3.0.2 were released since this PR has been merged but there hasn't been a new tagged release that includes this change, and it looks like those versions are still using 0.1.1. I may patch this in my Rails app, but would prefer to see it adopted into a core Ruby 😃. Forgive my ignorance, I don't understand the process of how stdlib updates are included in base Ruby install.

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

Successfully merging this pull request may close these issues.

6 participants