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 retries #778

Closed
wants to merge 5 commits into from
Closed

Add retries #778

wants to merge 5 commits into from

Conversation

florimondmanca
Copy link
Member

@florimondmanca florimondmanca commented Jan 18, 2020

I slept some more on #765, and I think I've come up with something quite nice.

Main changes compared to #765:

  • We always use an exponential backoff, so no more Schedule API. Just a backoff_factor know on the retries config class.
  • The default behavior is to retry on connection failures. No more RetryableError base class — which of our exceptions are retryable is hard-coded in the default behavior.
  • Being able to override .retry_flow() isn't really useful in itself, because extending a retry flow (even by copy-pasting our code) really isn't easy, due to many generator gotchas to watch for. So this PR implements some extension capability, mainly via the RetryLimits interface and being able to pass . I guess in an incremental approach we could restrict this PR to retries=<int> and retries=Retries(<int>[, backoff_factor=<float>]), and move the extension mechanism to a separate/future PR. I'm putting everything in here first to get some more feedback on the direction.

Things still to do:

  • Per-request retries.
  • Sync retries.
  • Add tests. (This won't be easy, as there are many code paths to account for! Hence probably the motivation for splitting this in two steps...)

Notes to reviewers:

  • Read the new docs for an overview of the functionality.
  • Use this script to try things out... Usage:
    • HTTPX_LOG_LEVEL=debug python example.py without starting the server, to try the behavior on connection failures.
    • Start the dummy server: uvicorn example:app
    • Re-run the script, to try the custom behavior implemented by RetryOnStatusCodes.
    • While retries are ongoing, you can turn off the server, and see that it switches to retrying on connection failures again. If you get the server back up, it will retry on status codes, etc. This continues until either policy runs out of retries. Which I think is quite nice/resilient. :-)
import asyncio
import typing

import httpx
from starlette.responses import PlainTextResponse


class RetryOnStatusCodes(httpx.RetryLimits):
    def __init__(self, limit: int, status_codes: typing.Container[int]) -> None:
        self.limit = limit
        self.status_codes = status_codes

    def retry_flow(
        self, request: httpx.Request
    ) -> typing.Generator[httpx.Request, httpx.Response, None]:
        retries_left = self.limit

        while True:
            response = yield request

            if response.status_code not in self.status_codes:
                return

            if retries_left == 0:
                try:
                    response.raise_for_status()
                except httpx.HTTPError as exc:
                    raise httpx.TooManyRetries(exc, response=response)
                else:
                    raise httpx.TooManyRetries(response=response)

            retries_left -= 1


async def main() -> None:
    url = "http://localhost:8000"
    retries = httpx.Retries(
        4, RetryOnStatusCodes(limit=4, status_codes={413, 429, 503}), backoff_factor=1,
    )
    async with httpx.AsyncClient(retries=retries) as client:
        r = await client.get(url)
        print(r)


async def app(scope, receive, send):
    response = PlainTextResponse(status_code=503)
    await response(scope, receive, send)


if __name__ == "__main__":
    asyncio.run(main())

@florimondmanca florimondmanca added the enhancement New feature or request label Jan 18, 2020
@florimondmanca florimondmanca mentioned this pull request Jan 18, 2020
@florimondmanca
Copy link
Member Author

I guess in an incremental approach we could restrict this PR

Yes, definitely going to split this into a more manageable first chunk of work that can actually be considered for merging… 😅

Copy link
Contributor

@yeraydiazdiaz yeraydiazdiaz left a comment

Choose a reason for hiding this comment

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

This is excellent, thanks @florimondmanca!

I left some suggestions, questions and implementation comments.

I didn't take part of the discussions leading to this PR so apologies if this was brought up before, but I feel adding built-in retries to httpx, while immediately useful, might be a source of pain for maintainers as users want more customization options, particularly when retrying and its successor tenacity offer so much in those terms. I can forsee having to push back on these or extend our implementation into a not-so-generic version of tenacity.

That's my 2c anyway, it's a really neat implementation 💯

docs/advanced.md Show resolved Hide resolved
httpx/config.py Show resolved Hide resolved
httpx/config.py Show resolved Hide resolved
httpx/config.py Show resolved Hide resolved
httpx/config.py Show resolved Hide resolved
@StephenBrown2
Copy link
Contributor

@yeraydiazdiaz In my opinion, I think that httpx could provide a basic retry functionality out of the box (including respecting the retry-after header, which is fairly important in http requests), and if more complex retry behaviour is desired, we can/should refer to more advanced modules like tenacity or others.

As is, I still like the progress on this so far. Definitely on board with a minimal implementation, and leaving it at that. Since other retry libraries do exist, we don't have to be too comprehensive with this one, even perhaps leaving off the composition (though I think that's quite neat and I haven't seen it before). Go go @florimondmanca! 👍

@florimondmanca
Copy link
Member Author

florimondmanca commented Jan 20, 2020

Thanks for this first round of reviews!

I'm preparing a PR with just the retries=<int> OR retries=Retries(<int>[, backoff_factor=<float>]) API, so will soon drop this one. There are a few items raised by @yeraydiazdiaz that will naturally resolve at that point.

@florimondmanca
Copy link
Member Author

florimondmanca commented Jan 20, 2020

Also worth mentioning that a middleware API such as the one outlined in #783 would 100% make it possible to provide tenacity-based retries as a third-party package, in the form of a middleware. Which means we could keep our own implementation to an absolute minimum, not even having to fuss about with RetryLimits.

@StephenBrown2
Copy link
Contributor

I'm preparing a PR with just the retries= OR retries=Retries([, backoff_factor=]) API, so will soon drop this one.

Is it necessary to do multiple PRs? Why not modify this one?

@florimondmanca
Copy link
Member Author

Closing in favor of #784.

@StephenBrown2

Is it necessary to do multiple PRs? Why not modify this one?

Allows keeping older PRs for future reference, and not have the cruft of comments/feedback from changes that were largely overridden. :-)

@florimondmanca florimondmanca deleted the retries-impl branch January 20, 2020 19:45
@StephenBrown2 StephenBrown2 mentioned this pull request Jul 31, 2020
11 tasks
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.

None yet

3 participants