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 setting Ruby Net::HTTP read_timeout option separately #1003

Conversation

springerigor
Copy link
Contributor

Description

Right now timeout setting sets all the timeout values (read, open & write if available). To unify the API with Net::HTTP one (which has a dedicated method for read_timeout as well) I propose extending the RequestOptions by read_timeout.

@springerigor springerigor force-pushed the read-timeout-request-option branch from bde6809 to 4fde5d7 Compare July 18, 2019 08:58
Right now `timeout` setting sets all the timeout values (`read`, `open` & `write` if available). To unify the API with `Net::HTTP` one (which has a dedicated method for `read_timeout` as well) I propose extending the `RequestOptions` by `read_timeout`.
@springerigor springerigor force-pushed the read-timeout-request-option branch from 4fde5d7 to 2597943 Compare July 18, 2019 09:04
@iMacTia
Copy link
Member

iMacTia commented Jul 18, 2019

Hi @springerigor, thanks for the addition.
I'm not sure why we were not supporting this yet, so thanks for adding it to the Net::HTTP adapter.
Do you think you might have some extra time to have a look at the other adapters and see if the new options can be easily supported there as well? This would help keeping them consistent as much as possible.

@technoweenie
Copy link
Member

Hi @springerigor: You never replied, so I've continued work on it in #1022. Thanks for your help, I'll preserve your original commit :)

@iMacTia:

I'm not sure why we were not supporting this yet

I think you can track my understanding of HTTP timeouts through Faraday ;) Basically, :timeout was the original setting, while :write_timeout and :open_timeout came later as users had to set other timeout options. I don't think :read_timeout is required, but it's consistent with the other timeout settings. I suppose :timeout can stay as a default setting.

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