httprb / http

HTTP (The Gem! a.k.a. http.rb) - a fast Ruby HTTP client with a chainable API, streaming support, and timeouts
MIT License
3.01k stars 321 forks source link

Timeout redesign for 6.x #773

Open tarcieri opened 11 months ago

tarcieri commented 11 months ago

This is a tracking issue for redesigning the timeout subsystem in http 6.x.

It would be good to come up with a new design first, and then PRs implementing it.

The largest single issue seems to be having a cumulative timeout over the entire request which actually works (originally requested in #562).

Several people have expressed interest in working on this, and I believe some code has already been written, although isn't available in a public PR.

cc @ClearlyClaire @misalcedo

misalcedo commented 11 months ago

I was comparing this HTTP client to others in Ruby and one thing I would like to add for timeouts is that global and per-operation are not mutually exclusive. I may want a request to time out after 1 minute, but spend no more than 150ms on the TCP connection portion or spend no more than 10 seconds idle on a read or write operation.

An alternative interpretation is what HTTPX does, which is to treat read as a timeout reading the response, write as a timeout writing the request and global as a overall timeout.

In my experience, except for streaming, HTTP clients care about total request timeout and maybe connect timeout, but definitely not per-operation timeouts. Streaming typically cares about operation timeouts and only optionally cares about a total timeout.

Often clients measure time to first byte from the server as well as overall latency, so timeouts on both of those could be helpful.

I am happy to help implement this type of behavior for 6.X if that is the target version.

Quoting my thoughts from https://github.com/httprb/http/pull/754#issuecomment-1853966126 here.

misalcedo commented 11 months ago

A couple of questions we probably want to answer with a design:

misalcedo commented 11 months ago

The useful stages of the request lifecycle to me are:

I don't see a benefit to mutually exclusive timeouts as timeouts can just be a min-heap (or array sorted in descending order) of deadlines before the client gives up.

As for guarantees, this depends on how often we check the current time against the next deadline. If we spend most of our time in socket operations (likely), then checking only on socket operations may be a tight enough bound.

I don't think requests should override the connect timeout as requests don't map one-to-one to connect operations. The rest of the timeouts make sense for a request to want to override. The reason I think this is fine is because a total timeout covers the case where the connection takes too long. If the client is slow to connect, having a tighter bound on just the connect operation is an optimization to fail fast since the client is willing to wait until the total timeout if it was the request or response portion that was slow.

ixti commented 11 months ago

One thing to keep in mind during the design (something that we overlooked in the current implementation) is that read timeout should correctly handle timeouts during both stages:

tarcieri commented 2 months ago

@ClearlyClaire @misalcedo we're looking at shipping v6.0.0 in #790. It's the last chance to make breaking changes to the timeout subsystem. Would either of you like to open a PR, or find someone else who would?