-
-
Notifications
You must be signed in to change notification settings - Fork 857
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
Add connect retries #1196
Add connect retries #1196
Conversation
Fab. 👍✨ Thoughts...
|
The
However, it looks like if Which I think means you'll end up with So I think the actual behaviour that
? |
423b738
to
61b148a
Compare
Based on #1141 (comment), I updated this PR to solely focus on adding support for @tomchristie Should be ready for a new round of reviews… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nicely done.
I think this probably strikes the right balance for us at the moment.
One comment for us to think about around the parameter naming.
Plus I guess we'd want the docs in before we merge it?
But yeah I think this is pretty fab. 👍👍👍
@tomchristie Changed the |
@tomchristie Addressed latest feedback — we should be good to go on this now? 😄 |
Okay, so I was leaving this on pause for a bit to try to figure out if connection retries really make sense on the client on at the transport layer, and I think we really do want this on the transport layer, wrapping as close to the connection opening as possible. The reason here is that if we pull connection retries out to the client layer, then we can end up in a gnarly situation when there's high contention on the connection pool. With connection retries out at the client layer an incoming request with a retry will:
We'd really prefer a behaviour where once the semaphore is acquired, the connection will perform any connect+retry within that scope, so...
This means much less thrashing when there's high contention. Anyways, upshot of this is that I think? I'm keen on us pushing the implementation details of this into the transport layer. Possibly by passing |
@tomchristie The motivations wrt timeouts and pooling are strong, indeed. Thoughts on my reservations related to testing here? #1141 (comment) On HTTPCore we can't just "swap out a transport", so I'm not sure how we'd test this w/o monkeypatch since it would be right into the socket opening logic… |
Ah really great point yup. This is kinda the point at which we need to be digging into exposing |
Opened encode/httpcore#221 against HTTPCore, closing this one for now! |
Closes #1141
Adds off-by-default "retry for connect errors" behavior, used as
httpx.Client(..., retries=<int>)
.ConnectError
/ConnectTimeout
are retried on, and that we only retry on idempotent HTTP verbs (i.e. exclude those that by definition should not be retried on because it could duplicate state changes on the server side).httpx.request(<method>, connect_retries=<int>)
is not included, since I'm not sure we want this knob in the high-level API.httpx.Client(retries=<int>)
API is a good start to any future enhancements to retry functionality, such as anhttpx.Retry
class for encapsulating retries configuration, or providing a custom retries API, while preserving "retries=<int>
retries on connect errors" for the most basic use cases.Rendered docs preview: