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

Added timeout and open_timeout options to Web API. #135

Merged
merged 1 commit into from
Mar 8, 2017

Conversation

dblock
Copy link
Collaborator

@dblock dblock commented Mar 8, 2017

Related to #134.

@macobo
Copy link

macobo commented Mar 8, 2017

Are there any downsides to also increasing the default?

@dblock
Copy link
Collaborator Author

dblock commented Mar 8, 2017

Faraday doesn't default the value to anything, letting the underlying Net::HTTP library handle this. I am a little wary setting it to something, but maybe I can be convinced otherwise.

@dblock
Copy link
Collaborator Author

dblock commented Mar 8, 2017

Check out my note in README though.

@macobo
Copy link

macobo commented Mar 8, 2017

👍 This is definitely an improvement, thanks for fixing.

One upside of a larger default is that if this extension is ever used in say an open-source chatbot (pagerbot) having a large problem would avoid per-bot hacks and troubles for some of the larger users who try to use the bot and fail to log in.

It might be worthwhile to merge this, then reach out to some of the people having issues and figuring out if a larger but still sane baseline setting as a default would make sense.

@dblock
Copy link
Collaborator Author

dblock commented Mar 8, 2017

I agree, we can save a lot of people debugging. What value are you using @macobo? Also are you changing only timeout or open_timeout as well? Note I wasn't able to find what the actual value is provided by Net::HTTP.

@dblock
Copy link
Collaborator Author

dblock commented Mar 8, 2017

Oh according to https://ruby-doc.org/stdlib-2.4.0/libdoc/net/http/rdoc/Net/HTTP.html both of these are 60 (!) seconds.

@macobo
Copy link

macobo commented Mar 8, 2017

Sadly I don't have access to a large organization to test with, sorry!

@dblock dblock merged commit a9e7069 into slack-ruby:master Mar 8, 2017
@dblock dblock deleted the timeout branch March 8, 2017 19:37
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.

2 participants