-
-
Notifications
You must be signed in to change notification settings - Fork 833
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
Sans-I/O client middleware stack #783
Conversation
0af9cb7
to
1a5fe2b
Compare
I think we should get rid of the |
So, probably no surprise that I'm a bit cautious about us introducing something here. Last time around adding a middleware stack felt like an abstraction too far, that masked the actual workings of the the Client, when a little more flatness was actually sufficient, and more readable. Something I'd suggest for any exploratory work here would be to add a middleware API without also moving around our existing implementation into middleware itself. That'd be a more minimal footprint to look over, and would let us focus more on "is this the right API to expose to users" vs. "is this feasible, and how does this affect our implementation". At this point it's not even clear to me that we neccessarily need a middleware API. For example, one keep-it-simple alternative here would be to make sure that we're documenting the Granted, those wouldn't be modular drop-me-in-for-reuse classes, but that might be okay. Alternately, it could be that a more constrained "event hooks" API would be sufficient (again along with adding good documentation for I guess my rough take at the moment on this would be to push back on it for as long as possible while gathering example use-cases. Documenting |
@tomchristie Cool, thanks for the feedback. Obviously I would have been very surprised that this would go through without push back or caution. :) I can try to isolate the addition of the middleware API in a separate PR. Not using it for our internal layers might end up making it seem like it’s not thorough enough, when I actually think it’s a pretty meaningful API that’s just at the right level of abstraction this time... Your point about « let’s just have users subclass and wrap .send() » is sensible, I just have the feeling we might be able to do better if this proves to be a right way to go. But right, let’s do that and take it from there. |
Closing, followed up with #800. |
So…
Implementing retries naturally involved adding a
.send_handling_retries()
method to clients. This has resulted in creating a chain of 3 method calls:With #778, both retries and auth would be using a generator-based API. We don't use one for redirects, but we very much could.
So all of this looked an awful lot like a middleware stack to me, so…
This PR moves the redirects and auth logic to a dedicated
httpx.middleware
, which defines a generator-based middleware API. As a result,Client
andAsyncClient
are really just pretty type-hinted shells around a dispatcher and a stack of middleware.A no-op middleware looks like this:
The question obviously is how can this API work both in the sync and async cases? For example, the redirect middleware needs to call
response.read()
/await response.aread()
...The solution is to defer the
await
ing of yielded value right to the edge... This is what these two helper functions allow to do:consume_generator()
consume_generator_of_awaitables()
They allow writing generators based on functions that may or may not return awaitables. For example, calling
dispatcher.send()
will return a coroutine in the async case (because we have anAsyncDispatcher
), but a plain response in the sync case (because we have aSyncDispatcher
). So if weyield dispatcher.send()
, and make sure the generator is passed throughconsume_generator()
in the sync case, andawait consume_generator_of_awaitables()
in the async case, then we'll have the coroutine awaited in the async case, and the plain value left as-is in the sync case. (Makes sense?)For cases when the API isn't exactly the same in the sync and async cases, we introduce a special
SyncOrAsync
container that defines what we should call in both situations, and then the two helper functions do the unwrapping when the stumble upon an instance of this container. So this allows doing the switch between e.g.response.read()
andresponse.aread()
.And… tada. ✨
Yes, this is generator black magic pushed to the limit.
But, I think this middleware API could actually be a viable one, which is a pretty big deal.
The real killer thing IMO is that it allows all of the following:
client.send()
[^0]. This is done with acontext
that works a bit likescope
in ASGI.([^0] This bit would just require adding a
**kwargs
everywhere on the client API…)Eventually, we could aim for something like this…