Dispatcher middlewares (refactored)

[florimondmanca]

Based off #267
Fixes #295

This builds on the idea of a callable-based middleware interface:

class TheMiddleware(BaseMiddleware):
    async def __call__(self, request, get_response):
        # Process request (or return an early response, e.g. from a cache)
        response = await get_response(request)
        # Process response
        return response

The initial get_response callable is built from the client's dispatch.send, and then from the inner middleware.

@yeraydiazdiaz Feel free to cherry-pick the commit into #267, or we can take the PR from here (you should have commit rights if you check out the branch from the httpx origin). :-)

[sethmlarson]

Mental note here to make sure whoever merges this PR use the GitHub co-commiter magic string in the Squash commit. :) Thank you two for tackling this issue!!

[yeraydiazdiaz]

Thanks for this @florimondmanca! I'm fine with merging this PR 🙂

I left some comments mainly for discussion but TL;DR I prefer the class-based approach with some reservations. 👍

[yeraydiazdiaz]

This looks nice! I like the new __call__ approach feels more like Middleware is a solid abstraction.

Thanks for taking the time fine tune this! Keen on hearing @sethmlarson and @tomchristie's thoughts on this but it's a 👍 from me.

[florimondmanca]

Thanks @yeraydiazdiaz!

[florimondmanca]

About the co-authoring thingy, I found here that we'll need to add this line to the squash merge message:

Co-authored-by: Yeray Diaz Diaz <yeray's email>

@yeraydiazdiaz Is your GH email address the one listed on your website? If it's configured as private, I think you'll need to send us your GitHub no-reply address (instructions here) (but perhaps that will work in all cases?).

[yeraydiazdiaz]

Yes, it's the same one. Thanks :)

[florimondmanca]

@sethmlarson Could you lend us a quick review to confirm this is on the right track as far as the middleware API is concerned? Having went over it when refactoring, I think the rest of the work originally commited by @yeraydiazdiaz is solid.

[sethmlarson]

Big asks from me:

• How do we plan on implementing middlewares that are capable of making more than one request (ie caching, digest auth)? Since the middleware doesn't have access to the calling client it can't make requests on it's behalf.
• We need to document how users can implement their own middlewares in advanced features.

[tomchristie]

Something that we could perfectly well consider would be adding a middleware interface to the client, but not forcing ourselves to factor redirects etc. out. I think that might end up being the best balance of having a comprehensible codebase, plus an extensible API.

Might well be worth drafting up a pull request along those lines, to see how it would look.

I'm also tentatively wondering if taking that approach might lead us to reassessing what the auth interface should look like. (Eg. maybe we'd be able to drop the auth classe interface completely, in favor of middleware, while still just keeping redirect behavior as part of the client itself.)

[florimondmanca]

I’m starting to think — it would really help to define the scope, goals, API and associated requirements (in a word: the design) of the middleware interface, to clear up any misunderstandings and have a shared vision of where this should be going?

I don’t think there was an issue initially but perhaps we could open one and discuss the details there.

[sethmlarson]

Yeah if we're going to have a 4th PR on this concept we should have a design discussion first to align our thoughts.

[florimondmanca]

@sethmlarson

How do we plan on implementing middlewares that are capable of making more than one request (ie caching, digest auth)? Since the middleware doesn't have access to the calling client it can't make requests on it's behalf.

As I explained in #295, the middleware can make extra requests by merely calling itself recursively, with a different or modified request. I put a Digest Auth PoC at the end of the issue description in #295.

Another PoC off the top of my head: a middleware that would retry sending a failed request within a given maximum of retries:

class RetryMiddleware(BaseMiddleware):
    def __init__(self, max_retries: int):
        self.max_retries = max_retries
        self.retries = 0

    async def __call__(self, request, get_response):
        if self.retries > self.max_retries:
            raise MaxRetriesExceeded

        response = await get_response(request)
        if response.status_code < 500:
            return response

        # Try again later…
        self.retries += 1
        await asyncio.sleep(1)
        return await self(request, get_response)

(This example shows that we may want to figure out how to allow middleware to make async I/O operations via the concurrency backend. I think a simple dependency injection mechanism where the client would automatically pass its backend to middleware that have it in their constructor would be a good compromise.)

This could be easily packaged in a separate library and mounted onto the HTTPX client, e.g.

client = httpx.Client()
client.add_middleware(RetryMiddleware(max_retries=3))

(Then we'd ask: how do we control where exactly the middleware is inserted? Do we even want/need to give that kind of control? I don't think it's in the scope of this PR though — this is very much a refactoring operation for an internal middleware API. Once we have this, we can easily design a public-facing middleware API on top.)

[sethmlarson]

This is a good first step towards middlewares :)

[florimondmanca]

Thanks all!