Retries for connection failures

[florimondmanca]

A simpler version of #778.

• Retries are off-by-default.
• They're still only triggered on connection failures only, and for idempotent HTTP verbs only.
• The only supported usage now is…

client = httpx.Client(retries=<int>)
client = httpx.Client(retries=httpx.Retries(<int>[, backoff_factor=<float>])

• That's it!

Still TODO:

• Add retries=... to the top-level API.
• Add per-request retries=...

Note:

• There are no tests for sync client retries, mostly because they'd be duplicated versions of the async case… but ideally we should add these, as not having them makes coverage suffer (because the implementation is duplicated).
• There are no tests for using retries on the top-level API, because we can't switch for a mock dispatcher there, and adding fail-until-some-number-of-retries on the uvicorn server isn't straight-forward at all…

---

You can try this out with…

# app.py
from starlette.responses import PlainTextResponse

app = PlainTextResponse("Hello, world!")

# example.py
import asyncio
import httpx


async def main():
    retries = httpx.Retries(10, backoff_factor=0.5)
    async with httpx.AsyncClient(retries=retries) as client:
        r = await client.get("http://localhost:8000")
        print(r)


asyncio.run(main())

1. $ python example.py
2. Start the server.
3. HTTPX first fails to get a response, but eventually manages to get one when the server is started.

Example log output:

DEBUG [2020-01-20 14:50:01] httpx.client - HTTP Request failed: NetworkError(OSError("Multiple exceptions: [Errno 61] Connect call failed ('::1', 8000, 0, 0), [Errno 61] Connect call failed ('127.0.0.1', 8000)"))
DEBUG [2020-01-20 14:50:01] httpx.client - Retrying in 0 seconds
DEBUG [2020-01-20 14:50:01] httpx.client - HTTP Request failed: NetworkError(OSError("Multiple exceptions: [Errno 61] Connect call failed ('::1', 8000, 0, 0), [Errno 61] Connect call failed ('127.0.0.1', 8000)"))
DEBUG [2020-01-20 14:50:01] httpx.client - Retrying in 0.5 seconds
DEBUG [2020-01-20 14:50:01] httpx.client - HTTP Request failed: NetworkError(OSError("Multiple exceptions: [Errno 61] Connect call failed ('::1', 8000, 0, 0), [Errno 61] Connect call failed ('127.0.0.1', 8000)"))
DEBUG [2020-01-20 14:50:01] httpx.client - Retrying in 1.0 seconds
DEBUG [2020-01-20 14:50:02] httpx.client - HTTP Request failed: NetworkError(OSError("Multiple exceptions: [Errno 61] Connect call failed ('::1', 8000, 0, 0), [Errno 61] Connect call failed ('127.0.0.1', 8000)"))
DEBUG [2020-01-20 14:50:02] httpx.client - Retrying in 2.0 seconds
DEBUG [2020-01-20 14:50:04] httpx.client - HTTP Request: GET http://localhost:8000 "HTTP/1.1 200 OK"
<Response [200 OK]>

For a sync client (had to make a few tweaks to our Urllib3Dispatcher here):

# example.py
import httpx

retries = httpx.Retries(10, backoff_factor=0.5)
with httpx.Client(retries=retries) as client:
    r = client.get("http://localhost:8000")
    print(r)

DEBUG [2020-01-20 14:59:36] httpx.client - HTTP Request failed: NetworkError(NewConnectionError('<urllib3.connection.HTTPConnection object at 0x102186310>: Failed to establish a new connection: [Errno 61] Connection refused'))
DEBUG [2020-01-20 14:59:36] httpx.client - Retrying in 0 seconds
DEBUG [2020-01-20 14:59:36] httpx.client - HTTP Request failed: NetworkError(NewConnectionError('<urllib3.connection.HTTPConnection object at 0x1032163d0>: Failed to establish a new connection: [Errno 61] Connection refused'))
DEBUG [2020-01-20 14:59:36] httpx.client - Retrying in 0.5 seconds
DEBUG [2020-01-20 14:59:37] httpx.client - HTTP Request failed: NetworkError(NewConnectionError('<urllib3.connection.HTTPConnection object at 0x103216990>: Failed to establish a new connection: [Errno 61] Connection refused'))
DEBUG [2020-01-20 14:59:37] httpx.client - Retrying in 1.0 seconds
DEBUG [2020-01-20 14:59:38] httpx.client - HTTP Request failed: NetworkError(NewConnectionError('<urllib3.connection.HTTPConnection object at 0x103216f90>: Failed to establish a new connection: [Errno 61] Connection refused'))
DEBUG [2020-01-20 14:59:38] httpx.client - Retrying in 2.0 seconds
DEBUG [2020-01-20 14:59:40] httpx.client - HTTP Request: GET http://localhost:8000 "HTTP/1.1 200 OK"
<Response [200 OK]>

[yeraydiazdiaz]

This is great, thanks @florimondmanca 🌟

Just a minor code style issue but otherwise LGTM

[florimondmanca]

Thanks for the reviews, @yeraydiazdiaz and @tomchristie! Addressed in the latest commits. Let me know what you think about these. :-)

[yeraydiazdiaz]

LGTM, nice job!

[tomchristie]

Seems pretty fantastic, yup!

One question: should we consider using private method names on the class?
(Given that we might have a public API there at some point, it aren’t ready to commit to one yet)

[florimondmanca]

should we consider using private method names on the class?

Well, we could, but then we'd need to have the client call into those private methods… I suppose not documenting Retries as an extensible component (similar to how Timeout isn't advertised as extensible) is enough for now?

[florimondmanca]

Stuff to consider as follow-ups:

• Add on-by-default support for Retry-After headers. (We'll definitely need to have some design discussion there, as we probably don't want to let users shoot themselves in the foot and DoS their entire program because HTTPX decided to wait 30s before re-issuing a request — worse even when that's done in a sync context.)
• [Edit] Add support for retrying on some very specific status codes, such as 503 Service Unavailable.

[florimondmanca]

@tomchristie I marked #782 and #785 as pending for v0.12. This one seems ready to be merged, so should we have it in for v0.12 as well?

[StephenBrown2]

How is this looking for 0.12.0? Looks like some conflicts now because of the private name switch, but I tested a merge and it goes cleanly aside from that.

If it gets merged and I have time this weekend, I may attempt to build on it for status code retries.

[florimondmanca]

@StephenBrown2 Personally I’d be happy to consider it for 0.12. The features so far are restricted in scope which is definitely a good thing... Though I’m still undecided on whether this should eventually be handled by a third-party package, if/when a middleware API such as in #783 is added. That’s why I might be tempted to mark this as « provisional », at least.

[tomchristie]

So, this is looking great!

There's one implementation-level bit that we might want to consider?...

Adding .sleep() to the backends, and using it from the client means that we've got some bit of coupling from the client to the backend implementations which doesn't currently exist.

That conflicts slightly with #768 which drops backends into being a strictly dispatcher-only bit of implementation, and with the intent of #782, which removes backend from the API at the client level.

Something that we could consider here is treating sleep not as part of the backend API, but instead just implementing a plain old async def sleep() function. (which uses sniffio and defers to either trio.sleep or asyncio.sleep).

We could use that in the async case, and use the regular time.sleep in the sync case. (Rather than calling lookup_backend, and calling backend.sleep().)

[dimaqq]

Possibly OT question: does this handle server sending a GOAWAY frame in response to starting a new stream?
Explanation: https://serverfault.com/a/962761
Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1050329#c14
Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=681477

[florimondmanca]

Closing this off as this is super stale now - lots of conflicts I'm not sure are easily resolvable.

If we figure connection retries are something we'd want to have built in, it should be fairly easy to start off from this diff to create a fresh branch from master.