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

Timeout redesign for 6.x #773

Open
tarcieri opened this issue Dec 13, 2023 · 4 comments
Open

Timeout redesign for 6.x #773

tarcieri opened this issue Dec 13, 2023 · 4 comments

Comments

@tarcieri
Copy link
Member

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

This was referenced Dec 13, 2023
@misalcedo
Copy link

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 #754 (comment) here.

@misalcedo
Copy link

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

  • What aspects of the request-lifecycle do we want to expose timeouts for?
  • Should any of these timeouts be mutually exclusive?
  • What guarantees do we want to make for upper and lower bounds on timeouts, if any (i.e. bounds on configured timeout versus when the client actually gives up)?
  • Can requests override the client's timeouts? Override only a subset? Or override none?

@misalcedo
Copy link

The useful stages of the request lifecycle to me are:

  • connect
  • write (request)
    • write (socket)
  • read (response)
    • first byte
    • parse the response
    • read (socket)
  • total time for a single request-response interaction

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
Copy link
Member

ixti commented Dec 13, 2023

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:

  • read headers
  • read body

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

No branches or pull requests

3 participants