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

Add support for `Net::HTTP#write_timeout` method (Ruby 2.6.0) #647

Merged

Conversation

Projects
None yet
3 participants
@springerigor
Copy link
Contributor

commented Feb 18, 2019

The method was introduced in Ruby 2.6.0.

The gem already supports open_timeout & read_timeout options, so it would be nice to provide support for write_timeout as well.

From the official documentation:

Number of seconds to wait for one block to be written (via one write(2) call). Any number may be used, including Floats for fractional seconds. If the HTTP object cannot write data in this many seconds, it raises a Net::WriteTimeout exception. The default value is 60 seconds. Net::WriteTimeout is not raised on Windows.

@springerigor springerigor marked this pull request as ready for review Feb 18, 2019

Add support for `Net::HTTP#write_timeout` method (Ruby 2.6.0)
[The method](https://ruby-doc.org/stdlib-2.6/libdoc/net/http/rdoc/Net/HTTP.html#method-i-write_timeout-3D) was introduced in Ruby 2.6.0.

The gem already supports `open_timeout` & `read_timeout`, so it would be nice to provide support for `write_timeout` as well.

From the official documentation:

> Number of seconds to wait for one block to be written (via one write(2) call). Any number may be used, including Floats for fractional seconds. If the HTTP object cannot write data in this many seconds, it raises a Net::WriteTimeout exception. The default value is 60 seconds. Net::WriteTimeout is not raised on Windows.

@springerigor springerigor force-pushed the springerigor:support-write-timeout-http-option branch from 02e11b3 to ab729db Feb 18, 2019

@jnunemaker

This comment has been minimized.

Copy link
Owner

commented Feb 18, 2019

Seems good to me. @TheSmartnik any thoughts either way?

@TheSmartnik
Copy link
Collaborator

left a comment

Thanks for the pr!

I've added some comments. It's not about your changes, really. Mostly about an old code, that needs to be DRY-ed up.

Also, please add some examples

# write_timeout 10
# end
def write_timeout(t)
raise ArgumentError, 'write_timeout must be an integer or float' unless t && (t.is_a?(Integer) || t.is_a?(Float))

This comment has been minimized.

Copy link
@TheSmartnik

TheSmartnik Feb 18, 2019

Collaborator

Could extract it to method and refactor other timeout methods as well, please?
Something like validate_timeout_argument(timeout) or check_timeout_argument(timeout)

@@ -126,6 +133,14 @@ def connection
http.open_timeout = options[:open_timeout]
end

if options[:write_timeout] && (options[:write_timeout].is_a?(Integer) || options[:write_timeout].is_a?(Float))

This comment has been minimized.

Copy link
@TheSmartnik

TheSmartnik Feb 18, 2019

Collaborator

There is a lot of duplication here. Would be better to extract it to a private method add_timeout?(options[:write_timeout])

if RUBY_VERSION >= "2.6.0"
http.write_timeout = options[:timeout]
else
Kernel.warn("Warning: option :write_timeout requires Ruby version 2.6.0 or later")

This comment has been minimized.

Copy link
@TheSmartnik

TheSmartnik Feb 18, 2019

Collaborator

It's a timeout for everything, we should set write_timeout if we can, but there is no need to add warning here.

@@ -126,6 +133,14 @@ def connection
http.open_timeout = options[:open_timeout]
end

if options[:write_timeout] && (options[:write_timeout].is_a?(Integer) || options[:write_timeout].is_a?(Float))
if RUBY_VERSION >= "2.6.0"

This comment has been minimized.

Copy link
@TheSmartnik

TheSmartnik Feb 18, 2019

Collaborator

There is a lot of duplication as well. Probably a would be better, to create a wrapper method like so (just an idea, not sure about the naming)

for_versions_above('2.6', option: :write_timeout) do
  http.write_timeout = options[:write_timeout]
end
it "should not set the write_timeout" do
http = double(
"http",
:null_object => true,

This comment has been minimized.

Copy link
@TheSmartnik

TheSmartnik Feb 18, 2019

Collaborator

Please, use colon notation for symbol keyword arguments

This comment has been minimized.

Copy link
@springerigor

springerigor Feb 19, 2019

Author Contributor

Hey @TheSmartnik thanks for the comments 👍 I addressed all of them, but this one. I would like to discuss one thing here.

Due to the fact that the doubles contain names with special characters (e.g. :use_ssl= => false) converting them to Ruby 1.9 hash syntax would require using ' to wrap them. Not sure if it is worth doing in this case. I am totally fine with leaving the current syntax.

Besides that, feel free to take a look at the pushed commits. The code looks better now IMO.

@springerigor springerigor force-pushed the springerigor:support-write-timeout-http-option branch from 4fb8646 to 2a4b359 Feb 19, 2019

@springerigor springerigor force-pushed the springerigor:support-write-timeout-http-option branch from 2a4b359 to 46e84c0 Feb 19, 2019

@springerigor

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

@TheSmartnik A kind reminder for taking another look 👋

@TheSmartnik

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

@springerigor thanks a lot! Sorry for a long reply

@TheSmartnik TheSmartnik merged commit 413df85 into jnunemaker:master Mar 20, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@springerigor springerigor deleted the springerigor:support-write-timeout-http-option branch Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.