http icon indicating copy to clipboard operation
http copied to clipboard

confusion about docs/api with global timeouts

Open jrochkind opened this issue 6 years ago • 7 comments

The wiki page on Timeouts suggests:

Global timeouts let you set an upper bound of how long a request can take, without having to rely on Timeout.timeout:

   HTTP.timeout(:global, :write => 1, :connect => 1, :read => 1)
      .get "http://example.com"

Uses a timeout of 3 seconds, for the entire get call.

This sounds just right -- avoiding Timeout.timeout is always great for well-known reasons, and my use cases want a timeout for the entire get call, I don't care how long connect/write/read individually take, I just care that the end-to-end time times out after N seconds.

But I don't understand these docs. If it's a timeout for the "entire get call", why am i providing write/connect/read separately? In the above example, if the connect takes more than one second... will it timeout? Even though it hasn't reached the "3 seconds, for the entire get call"? Or are the individual numbers ignored, and just summed up, as "Uses a timeout of 3 seconds, for the entire get call" suggests? But then how does that API make any sense?

With the description of what it does, I would have expected an API more like HTTP.timeout(global: 3).

What's going on here? Can you advise? Thanks!

jrochkind avatar Jul 05 '18 18:07 jrochkind

It looks like the docs are out of date, and the proper syntax is HTTP.timeout(3). I've updated them to use that. Is that clearer now?

zanker avatar Jul 05 '18 19:07 zanker

Hmm, that would be more clear if it worked! But if I try with http.rb 3.3.0, ruby 2.5.1

HTTP.timeout(10).get("https://github.com/httprb/http")

I get:

NoMethodError: undefined method `to_sym' for 10:Integer
Did you mean?  to_s
  /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/chainable.rb:104:in `timeout'

???

Reading the source code, it looked to me like timeout(10) should work, and maybe so should timeout(global: 10). Neither seems to work for me, although in different ways.

jrochkind avatar Jul 05 '18 19:07 jrochkind

When I try HTTP.timeout(global: 10).get("https://github.com/httprb/http") I am getting:

       12: from /Users/jrochkind/.rubies/ruby-2.5.1/bin/irb:11:in `<main>'
       11: from (irb):2
       10: from /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/chainable.rb:20:in `get'
        9: from /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/client.rb:30:in `request'
        8: from /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/client.rb:71:in `perform'
        7: from /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/connection.rb:102:in `read_headers!'
        6: from /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/connection.rb:102:in `loop'
        5: from /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/connection.rb:103:in `block in read_headers!'
        4: from /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/connection.rb:216:in `read_more'
        3: from /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/timeout/per_operation.rb:64:in `readpartial'
        2: from /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/timeout/per_operation.rb:64:in `loop'
        1: from /Users/jrochkind/.gem/ruby/2.5.1/gems/http-3.3.0/lib/http/timeout/per_operation.rb:70:in `block in readpartial'
HTTP::TimeoutError (Read timed out after 0.25 seconds)

0.25 seconds what? Where is that even coming from, I never said anything about a 0.25 second timeout -- looks like it's defaulting to 0.25 per-operation timeouts when you don't specify them? Which is not what is documented.

jrochkind avatar Jul 05 '18 19:07 jrochkind

This might be working, at least it doens't error, not totally sure it's doing what I expect with regard to timeouts. .timeout(global: 10, write: nil, connect: nil, read: nil)

Oops no, I think that's doing no timeout at all.

jrochkind avatar Jul 05 '18 19:07 jrochkind

Ack, sorry. I forgot that a 4.0 version is on master at this point. The API is fixed in 4.0, but the reason it requires the awkward setup of calling read/write/connect is mostly a mistake that was made when the feature was initially built.

So it'll get better in 4.0, but for the time being, the options are correct.

zanker avatar Jul 05 '18 19:07 zanker

Experimenting, I believe that the options as documented really do just sum read/connect/write and use that as the global timeout. The individual values don't matter.

Eg, :global, read: 1, write: 1, connect: 5 means exactly the same thing as :global, read: 5, write: 1, connect: 1.

Confirm?

jrochkind avatar Jul 05 '18 19:07 jrochkind

That's correct

zanker avatar Jul 05 '18 20:07 zanker