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

Add connect retries #221

Merged
merged 4 commits into from
Oct 12, 2020
Merged

Add connect retries #221

merged 4 commits into from
Oct 12, 2020

Conversation

florimondmanca
Copy link
Member

@florimondmanca florimondmanca commented Oct 10, 2020

Alternative to encode/httpx#1196, prompted by encode/httpx#1196 (comment)

Closes encode/httpx#1141

This PR adds off-by-default "retry on connection failure" capability to the connection pool, using a retries=<int> option. Also includes an exponential backoff mechanism, with a fixed backoff factor of 0.5 for now (so, 0s, 0.5s, 1.0s, 2.0s, 4.0s, etc).

We add it at the transport layer, as close to opening sockets as possible, so that connect retries don't interact with the connection pooling logic, so that we do "acquire a connection, open a socket, open a socket, …, release the connection".

(We're in a better position now to add this feature now, because we have a backend option on the connection pool transport. This PR tweaks it a bit so we can pass a full-fledged backend instance, rather than only a backend name. Then we build and provide a mock backend that raises exception on-demand, for testing purposes.)

(The diff looks quite big, but everything is unasynced, so duplicated, and test code is a bit large too, so…)

Once merged, we'll be able to document this on the HTTPX side, in a new "Retries" section in "Advanced Usage". For now usage will be a bit involved, but once httpx.HTTPTransport() lands (encode/httpx#1302) things will be easier UX-wise.

@florimondmanca florimondmanca added the enhancement New feature or request label Oct 10, 2020
@florimondmanca florimondmanca requested a review from a team October 10, 2020 06:58
Copy link
Member

@JayH5 JayH5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this as a most basic (but still useful) implementation of retries 🎉

Only question is whether the parameter should be retries or connect_retries? Not sure if that discussion has already been had somewhere?

tests/async_tests/test_retries.py Outdated Show resolved Hide resolved
@cdeler
Copy link
Member

cdeler commented Oct 10, 2020

@florimondmanca

Sorry for the question. You added the retries mechanism only for connection establishing yes?

It's talking about the possibility, if we may be introducing some sort of Retry's dict as it's done for timeouts' dict?

After that we can carefully handle retry for, e.g. "HTTP POST" requests (allow retries for connect, but do not allow after that)

What do you think about that?

PS reference link : urllib3 retry doc (they also separate the retries types)

@florimondmanca
Copy link
Member Author

florimondmanca commented Oct 10, 2020

@JayH5

Only question is whether the parameter should be retries or connect_retries? Not sure if that discussion has already been had somewhere?

I think we've had it already on the HTTPX repo. TLDR was "let's use retries so we can expand its signature if needed w/o having to add an extra option". So eg in top of retries=<int>, we can easily expand to support retries=(int, int, ...) (or a dict) for any kinds of retries we'd like to support. Same if we want to make the exponential back off configurable at some point. And then we wrap it up in a handy httpx.Retries class on the HTTPX side, like we do for httpx.Timeout. :)

@cdeler Yes only connection for now, as an absolute minimum viable feature and implementation. As I explained above we can easily expand later based on feedback and proven needs. There's a lot of background for why this minimum scope was chosen in: encode/httpx#1141 and encode/httpx#1196.

Copy link
Member

@JayH5 JayH5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this 😄

@tomchristie
Copy link
Member

Fantastic, lovely piece of work!

And yup, once we've had a good look at encode/httpx#1302 we ought to be in a better position wrt. usability for this too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retries
4 participants