Timeouts across all redirects

[pineman]

Hi @HoneyryderChuck!

It seems to me that the request_timeout timer is reset on each redirect. Concretely:

require "httpx"
start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
r = HTTPX
  .plugin(:follow_redirects, max_redirects: 90)
  .with(timeout: { request_timeout: 1 })
  .get("https://httpbin.org/redirect/100")
pp Process.clock_gettime(Process::CLOCK_MONOTONIC) - start
pp r

I was expecting this code to timeout after 1s, globally, across all redirects. In reality, it will return after following 90 redirects (if all of those requests are under 1s), which in my machine is taking around 25s.

Is it possible to support a "global timeout" option, across all redirects? Or am I missing something here? Thanks!

[pineman]

I could use Timeout::timeout but it is fraught with peril. I was looking for a "global timeout" option inside HTTPX itself since it has good timeouts and doesn't use Timeout::timeout.

[HoneyryderChuck]

Hi @pineman ,

What a coincidence. I was reading about wcurl and some timeout feature curl supports, which is used to emulate wget redirect/retries support, and I thought to myself that httpx does not have support for such a thing.

Indeed, avoid Timeout::timeout at all cost. This must be supported upstream.

Perhaps you can help me design this one. I think this can be supported in the following ways:

1. request_timeout extend to redirects/retries
2. specific redirect_timeout and retries_timeouts
3. a la curl request_max_time, keep current request_timeout the way it is

I've been leaning towards the first (it'd transparently work for the faraday adapter as well, but there's certainly potential for confusion.

LMK what you think.

[pineman]

I'm no library author, but 3. certainly seems to be the easiest with regards to API compatibility 🤔

I personally feel the 'default' timeout option in an HTTP client should apply globally to all redirects if the follow redirects option is enabled. That's my expectation, but I'm not 100% sure that's the most popular across the ecosystem. HTTP.rb seems to apply the timeout globally, for what it's worth, so this gives weight to option 1.

Not sure if it's a common usecase, but having the ability to control both the 'global' timeout and the per-redirect timeout may be needed in certain usecases I suppose, like option 2. Maybe a fourth option to break request_timeout to now be global, but allowing a new option called redirect_timeout, which works how request_timeout presently does? Such that request_timeout >= redirect_timeout.

[HoneyryderChuck]

Unfortunately HTTP.rb timeouts are kind of broken (that global timeout can also apply to multiple requests in a block...), so I wouldn't draw conclusions from it.

i'll consider the feedback, thank you.

[HoneyryderChuck]

@pineman are you able to test the max-time branch? (MR here)

[pineman]

Just tried the code I had posted and it worked as expected, timing out after one second! Awesome!

[HoneyryderChuck]

good to hear! bear in mind that it will not work in the same manner for retries, i.e. it resets for subsequent retries. this mimicks curl's max-time behaviour, and I think it makes sense.