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

Bump default timeouts for http resource #1835

Merged
merged 1 commit into from May 29, 2017

Conversation

schisamo
Copy link
Contributor

This changes the default read and open timeouts to be 60 seconds which matches the defaults for Net::HTTP backend which Faraday uses by default:
https://ruby-doc.org/stdlib-2.4.1/libdoc/net/http/rdoc/Net/HTTP.html#read_timeout-attribute-method
https://ruby-doc.org/stdlib-2.4.1/libdoc/net/http/rdoc/Net/HTTP.html#open_timeout-attribute-method

The current timeout values are too small which causes tests to be flakey.

Signed-off-by: Seth Chisamore schisamo@chef.io

@adamleff
Copy link
Contributor

I imagine there might be some people counting on those smaller timeouts. I'm totally cool with changing the defaults, but I wonder if now's the time we expose a read_timeout and open_timeout parameter to the resource that defaults to your newly-propose options.

@schisamo schisamo force-pushed the schisamo/http-resource-default-timeouts branch 2 times, most recently from 3343ef4 to ed9a9fc Compare May 19, 2017 16:07
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

Thanks @schisamo! Sorry you were experiencing flaky tests, but this will help you and existing/future users too. I appreciate it!

This changes the default read and open timeouts to be 60 seconds which
matches the defaults for `Net::HTTP` backend which Faraday uses by
default:
https://ruby-doc.org/stdlib-2.4.1/libdoc/net/http/rdoc/Net/HTTP.html#read_timeout-attribute-method
https://ruby-doc.org/stdlib-2.4.1/libdoc/net/http/rdoc/Net/HTTP.html#open_timeout-attribute-method

The current timeout values are too small which causes tests to be
flakey.

Signed-off-by: Seth Chisamore <schisamo@chef.io>
@chris-rock chris-rock force-pushed the schisamo/http-resource-default-timeouts branch from ed9a9fc to 798aebf Compare May 29, 2017 19:08
Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

Great improvement @schisamo

@chris-rock chris-rock merged commit c9a7f65 into master May 29, 2017
@chris-rock chris-rock deleted the schisamo/http-resource-default-timeouts branch May 29, 2017 19:20
@adamleff adamleff added the Type: Enhancement Improves an existing feature label May 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants