Simplify setting a timeout #446

mikegee · 2017-12-31T13:58:08Z

.timeout(<Numeric>) sets a global timeout value.
.timeout(connect: x, write: y, read: z) sets per operation timeouts.

The previous style: .timeout(:global|:per_operation, hash_of_options) doesn't work anymore.

fixes #444

tarcieri · 2017-12-31T14:40:58Z

RuboCop errors:

Offenses:
lib/http/chainable.rb:91:53: C: Layout/SpaceInsideHashLiteralBraces: Space inside { detected.
                       when Integer then [:global, { read: klass }]
                                                    ^
lib/http/chainable.rb:91:54: C: Style/HashSyntax: Use hash rockets syntax.
                       when Integer then [:global, { read: klass }]
                                                     ^^^^^
lib/http/chainable.rb:91:65: C: Layout/SpaceInsideHashLiteralBraces: Space inside } detected.
                       when Integer then [:global, { read: klass }]
                                                                ^
71 files inspected, 3 offenses detected
RuboCop failed!

janko · 2017-12-31T16:43:16Z

When I read #444, I imagined that specifying an integer would mean having a timeout around the whole request, in a way that the request can time out during any operation, not just read. With the currently proposed implementation the request can take longer than n seconds since connect and write operations are not limited. If one would specify 5 seconds, connecting could take 2 seconds, writing 3, and reading 4, in which case the request lasted 9 seconds in total.

mikegee · 2017-12-31T18:06:59Z

a timeout around the whole request

Indeed, I forgot to divide by 3. Global timeout doesn't work in the least surprising way, IMO.

mikegee · 2017-12-31T18:13:42Z

I forgot to divide by 3.

hrm. maybe setting only one to the expected global value will work as expected?

http/lib/http/timeout/global.rb

Line 20 in 70658da

@time_left = connect_timeout + read_timeout + write_timeout

In my apps, I've been setting all three of those to a third of my total time allowed. I'm thinking just setting one will do the trick.

zanker · 2017-12-31T18:43:25Z

Sorry about this, it's mostly a wart from how I implemented it a few years ago.

You can add a new field like total_time and use that if it's set, otherwise fallback to connect + read + write. We should deprecate the old one of adding things together, since it's not very clear.

tarcieri · 2017-12-31T18:48:31Z

I know we're at 3.0.0 now, but it might be nice to go to 4.0.0 and redesign the timeout API a bit. That would definitely simplify moving things over to Socketry underneath

mikegee · 2018-01-04T01:04:59Z

I created a global_timeout option like the PerOperation values. It still sums the per operation values if global_timeout isn't provided (explicitly or via .timeout(Numeric)).

I don't like that the deprecation warning requires using the connection, HTTP.timeout(:global, read: 3) is insufficient. I don't know if anything can be done about it or if it even matters.

I would have liked to make Global not inherit from PerOperation and simplify the code some, but backwards compatibility ...

tarcieri · 2018-01-04T01:13:29Z

@mikegee I'd suggest (based on the feedback above) rewriting the patch without regard to backwards compatibility

mikegee · 2018-01-04T12:36:01Z

@tarcieri alrighty. I'm working on it.

mikegee · 2018-01-04T15:16:08Z

I updated the title and description of this change. I would appreciate some review now.

ixti · 2018-01-04T23:00:34Z

lib/http/timeout/global.rb

      end

      # To future me: Don't remove this again, past you was smarter.
      def reset_counter
-        @time_left = connect_timeout + read_timeout + write_timeout
-        @total_timeout = time_left
+        @time_left = timeout


Personal flavor, but can you please use instance variable instead of attr reader here?

Yeah, sure. I'll change PerOperation to match your preferred style, too.

BTW, I avoid instance variables in my code since typos can lead to subtle bugs: @mispelled #=> nil, whereas misspelled method calls raise a NameError.

ixti · 2018-01-04T23:01:11Z

lib/http/timeout/global.rb

@@ -124,7 +126,7 @@ def reset_timer
      def log_time
        @time_left -= (Time.now - @started)
        if time_left <= 0


Same here. I know that it was before, but please update this one too to use instance variable instead.

ixti · 2018-01-04T23:03:20Z

lib/http/chainable.rb

+    #   Adds a global timeout to the full request
+    #   @param [Numeric] global_timeout
+    def timeout(options, extra_arg = nil)
+      raise ArgumentError, TIMEOUT_USAGE if extra_arg


This is gonna be a breaking change, so I think it's fine to just change method signature to accept single argument only.

I debated this, but thought a bit of effort to ease the transition was worth it. I'll remove it. No worries.

Per #447 we might end up redoing the whole thing with keyword arguments anyway, but don't worry about that for this PR

ixti · 2018-01-04T23:05:57Z

Couple of changes - and I will be glad to merge it.

tarcieri · 2018-01-04T23:43:08Z

Looks good to me too... aside from @ixti's nits

These were too noisy for me: WARNING: Using the `raise_error` matcher without providing a specific error or message risks false positives, since `raise_error` will match when Ruby raises a `NoMethodError`, `NameError` or `ArgumentError`, potentially allowing the expectation to pass without even executing the method you are intending to call. Actual error raised was #<NoMethodError: undefined method `fetch' for "[FOOBAR]":String>. Instead consider providing a specific error class or message. This message can be suppressed by setting: `RSpec::Expectations.configuration.on_potential_false_positives = :nothing`. Called from /home/travis/build/httprb/http/spec/lib/http_spec.rb:252:in `block (3 levels) in <top (required)>'. WARNING: Using the `raise_error` matcher without providing a specific error or message risks false positives, since `raise_error` will match when Ruby raises a `NoMethodError`, `NameError` or `ArgumentError`, potentially allowing the expectation to pass without even executing the method you are intending to call. Actual error raised was #<KeyError: key not found: :user>. Instead consider providing a specific error class or message. This message can be suppressed by setting: `RSpec::Expectations.configuration.on_potential_false_positives = :nothing`. Called from /home/travis/build/httprb/http/spec/lib/http_spec.rb:260:in `block (3 levels) in <top (required)>'. WARNING: Using the `raise_error` matcher without providing a specific error or message risks false positives, since `raise_error` will match when Ruby raises a `NoMethodError`, `NameError` or `ArgumentError`, potentially allowing the expectation to pass without even executing the method you are intending to call. Actual error raised was #<KeyError: key not found: :pass>. Instead consider providing a specific error class or message. This message can be suppressed by setting: `RSpec::Expectations.configuration.on_potential_false_positives = :nothing`. Called from /home/travis/build/httprb/http/spec/lib/http_spec.rb:256:in `block (3 levels) in <top (required)>'.

- `.timeout(<Numeric>)` sets a global timeout value. - `.timeout(connect: x, write: y, read: z)` sets per operation timeouts. The previous style: `.timeout(:global|:per_operation, hash_of_options)` doesn't work anymore.

mikegee · 2018-01-05T01:59:40Z

I don't understand the build failure. It is in unrelated code, I can't duplicate it locally in 2.4.1, and when I pushed a commit focused on that spec, it passed. Is there a way to tell Travis to retry?

ixti · 2018-01-05T02:22:53Z

Yeah, I have restarted.

ixti · 2018-01-05T02:24:23Z

spec/lib/http_spec.rb

    end

    it "fails when :user is not given" do
-      expect { HTTP.basic_auth :pass => "[PASS]" }.to raise_error
+      expect { HTTP.basic_auth :pass => "[PASS]" }.to raise_error(KeyError)


Huge thanks for fixing those!

ixti · 2018-01-05T02:26:28Z

Thank you for your contribution!

tarcieri · 2018-01-05T02:28:20Z

Thanks!

httprb/http#446

@ixti

Update ruby-http to 4.4.1. ## 4.4.1 (2020-03-29) * Backport [#590](httprb/http#590) Fix parser failing on some edge cases. ([@ixti]) ## 4.4.0 (2020-03-25) * Backport [#587](httprb/http#587) Fix redirections when server responds with multiple Location headers. ([@ixti]) * Backport [#599](httprb/http#599) Allow passing HTTP::FormData::{Multipart,UrlEncoded} object directly. ([@ixti]) ## 4.3.0 (2020-01-09) * Backport [#581](httprb/http#581) Add Ruby-2.7 compatibility. ([@ixti], [@janko]) ## 4.2.0 (2019-10-22) * Backport [#489](httprb/http#489) Fix HTTP parser. ([@ixti], [@fxposter]) ## 4.1.1 (2019-03-12) * Add `HTTP::Headers::ACCEPT_ENCODING` constant. ([@ixti]) ## 4.1.0 (2019-03-11) * [#533](httprb/http#533) Add URI normalizer feature that allows to swap default URI normalizer. ([@mamoonraja]) ## 4.0.5 (2019-02-15) * Backport [#532](httprb/http#532) from master. Fix pipes support in request bodies. ([@ixti]) ## 4.0.4 (2019-02-12) * Backport [#506](httprb/http#506) from master. Skip auto-deflate when there is no body. ([@Bonias]) ## 4.0.3 (2019-01-18) * Fix missing URL in response wrapped by auto inflate. ([@ixti]) * Provide `HTTP::Request#inspect` method for debugging purposes. ([@ixti]) ## 4.0.2 (2019-01-15) * [#506](httprb/http#506) Fix instrumentation feature. ([@paul]) ## 4.0.1 (2019-01-14) * [#515](httprb/http#515) Fix `#build_request` and `#request` to respect default options. ([@RickCSong]) ## 4.0.0 (2018-10-15) * [#482](httprb/http#482) [#499](httprb/http#499) Introduce new features injection API with 2 new feaures: instrumentation (compatible with ActiveSupport::Notification) and logging. ([@paul]) * [#473](httprb/http#473) Handle early responses. ([@janko-m]) * [#468](httprb/http#468) Rewind `HTTP::Request::Body#source` once `#each` is complete. ([@ixti]) * [#467](httprb/http#467) Drop Ruby 2.2 support. ([@ixti]) * [#436](httprb/http#436) Raise ConnectionError when writing to socket fails. ([@janko-m]) * [#438](httprb/http#438) Expose `HTTP::Request::Body#source`. ([@janko-m]) * [#446](httprb/http#446) Simplify setting a timeout. ([@mikegee]) * [#451](httprb/http#451) Reduce memory usage when reading response body. ([@janko-m]) * [#458](httprb/http#458) Extract HTTP::Client#build_request method. ([@tycoon]) * [#462](httprb/http#462) Fix HTTP::Request#headline to allow two leading slashes in path. ([@scarfacedeb]) * [#454](httprb/http#454) [#464](httprb/http#464) [#384](httprb/http#384) Fix #readpartial not respecting max length argument. ([@janko-m], [@marshall-lee])

Since http-rb 4.0, the http-rb gem has supported a better global timeout as `timeout(seconds)`. httprb/http#446 As the [http-rb wiki explains](https://github.com/httprb/http/wiki/Timeouts), using eg `timeout(3)` means that 3 seconds is an upper bound for the entire HTTP call. The old way of doing things, before this PR, setting a faraday `timeout` value of `3` would turn into `HTTP.timeout(connect: 3, write: 3, read: 3)`, which would mean the whole HTTP call could actually take up to **9** seconds: 3 for open, 3 for write, another 3 for read. The new way is better semantics for a single faraday `timeout` option. It's been supported since http-rb 4.0, and this Faraday adapter doesn't support any earlier http-rb. It makes sense to change to use the new better way. The existing functionality is not tested here; and I couldn't figure out any good way to test it. Mocking http-rb raising a timeout error would _not_ test this functionality of passing in timeout confiuration appropriately to http-rb. I spent some time on it, but couldn't figure out any good way to test, sorry. This is not a reduction in test coverage, it already wasn't tested.

As of httprb/http#446 Increasing the http dependancy to `4.0` where this change is released.

mikegee force-pushed the simplify-global-timeout branch from be95b24 to 873efbf Compare December 31, 2017 18:15

mikegee force-pushed the simplify-global-timeout branch 2 times, most recently from 98461eb to 99380ab Compare January 4, 2018 01:00

mikegee force-pushed the simplify-global-timeout branch 3 times, most recently from 1238cec to 0cb3f69 Compare January 4, 2018 15:07

mikegee changed the title ~~Simplify setting a global timeout~~ Simplify setting a timeout Jan 4, 2018

ixti requested changes Jan 4, 2018

View reviewed changes

tarcieri mentioned this pull request Jan 4, 2018

Additional breaking changes for 5.x #447

Open

mikegee added 2 commits January 4, 2018 20:22

Simplify setting timeouts

11e1580

- `.timeout(<Numeric>)` sets a global timeout value. - `.timeout(connect: x, write: y, read: z)` sets per operation timeouts. The previous style: `.timeout(:global|:per_operation, hash_of_options)` doesn't work anymore.

mikegee force-pushed the simplify-global-timeout branch 2 times, most recently from 8ec5597 to 11e1580 Compare January 5, 2018 01:55

ixti reviewed Jan 5, 2018

View reviewed changes

ixti approved these changes Jan 5, 2018

View reviewed changes

ixti merged commit c698449 into httprb:master Jan 5, 2018

mikegee deleted the simplify-global-timeout branch January 5, 2018 02:38

Eric-Guo added a commit to Eric-Guo/wechat that referenced this pull request Jan 22, 2019

Support httprb 4.0

40aa9ee

httprb/http#446

hophacker pushed a commit to ruilisi/wechat that referenced this pull request Jan 28, 2019

Support httprb 4.0

63586c6

httprb/http#446

cben mentioned this pull request Jun 2, 2019

update http client version ManageIQ/kubeclient#413

Merged

ixti mentioned this pull request Mar 28, 2020

timeout: undefined method `to_sym' for :Integer #603

Closed

jrochkind mentioned this pull request Oct 11, 2021

Use better http-rb global timeout value lostisland/faraday-http#28

Merged

Chrizpy added a commit to spinels/redd that referenced this pull request Mar 31, 2022

HTTP.timeout now assumes klass based on input

7b153c9

As of httprb/http#446 Increasing the http dependancy to `4.0` where this change is released.

This was referenced Mar 31, 2022

HTTP.timeout now assumes klass based on input avinashbot/redd#104

Closed

HTTP.timeout now assumes klass based on input spinels/redd#10

Merged

Chrizpy added a commit to spinels/redd that referenced this pull request Mar 31, 2022

HTTP.timeout now assumes klass based on input

382e050

As of httprb/http#446 Increasing the http dependancy to `4.0` where this change is released.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify setting a timeout #446

Simplify setting a timeout #446

mikegee commented Dec 31, 2017 •

edited

Loading

tarcieri commented Dec 31, 2017

janko commented Dec 31, 2017 •

edited

Loading

mikegee commented Dec 31, 2017

mikegee commented Dec 31, 2017 •

edited

Loading

zanker commented Dec 31, 2017

tarcieri commented Dec 31, 2017

mikegee commented Jan 4, 2018

tarcieri commented Jan 4, 2018 •

edited

Loading

mikegee commented Jan 4, 2018

mikegee commented Jan 4, 2018

ixti Jan 4, 2018

mikegee Jan 5, 2018

ixti Jan 4, 2018

ixti Jan 4, 2018

mikegee Jan 5, 2018

tarcieri Jan 5, 2018

ixti commented Jan 4, 2018

tarcieri commented Jan 4, 2018

mikegee commented Jan 5, 2018

ixti commented Jan 5, 2018

ixti Jan 5, 2018

ixti commented Jan 5, 2018

tarcieri commented Jan 5, 2018

Simplify setting a timeout #446

Simplify setting a timeout #446

Conversation

mikegee commented Dec 31, 2017 • edited Loading

tarcieri commented Dec 31, 2017

janko commented Dec 31, 2017 • edited Loading

mikegee commented Dec 31, 2017

mikegee commented Dec 31, 2017 • edited Loading

zanker commented Dec 31, 2017

tarcieri commented Dec 31, 2017

mikegee commented Jan 4, 2018

tarcieri commented Jan 4, 2018 • edited Loading

mikegee commented Jan 4, 2018

mikegee commented Jan 4, 2018

ixti Jan 4, 2018

Choose a reason for hiding this comment

mikegee Jan 5, 2018

Choose a reason for hiding this comment

ixti Jan 4, 2018

Choose a reason for hiding this comment

ixti Jan 4, 2018

Choose a reason for hiding this comment

mikegee Jan 5, 2018

Choose a reason for hiding this comment

tarcieri Jan 5, 2018

Choose a reason for hiding this comment

ixti commented Jan 4, 2018

tarcieri commented Jan 4, 2018

mikegee commented Jan 5, 2018

ixti commented Jan 5, 2018

ixti Jan 5, 2018

Choose a reason for hiding this comment

ixti commented Jan 5, 2018

tarcieri commented Jan 5, 2018

mikegee commented Dec 31, 2017 •

edited

Loading

janko commented Dec 31, 2017 •

edited

Loading

mikegee commented Dec 31, 2017 •

edited

Loading

tarcieri commented Jan 4, 2018 •

edited

Loading