Context managed client.send()

[tomchristie]

Reworking of #1355 in order to keep thinking a few things over.

• Client.send() changes from returning an "open" Response to being a context managed interface.
• AsyncClient.send() changes from returning an "open" Response to being a context managed interface.
• The internal StreamContextManager disappears — we don't need it anymore.
• Response(on_close) disappears - we don't need it anymore.
• Response.close() does still exist, but it's no longer an I/O operation, it's purely for state marking. (Hey, we're not going to let you attempt to read the stream anymore)

There's a bunch of stuff for us to discuss around 1.0 and these last bits of "should we be using a context managed interface for (1) the Transport API and (2) the send() method."

For me the important step forward is the core idea in #1515, that the Transport API should be an HTTPX interface, in the HTTPX package. The really nice thing there is that once we do that, the httpcore implementation and API doesn't strictly have to define what we're presenting to the end-user in httpx, which just makes everything easier, and allows use to consider each of these three separate issues in isolation from one another...

1. Should the Transport API be a context managed request/response interface?
2. Should the client.send() API (and httpx internals) be a context managed interface?
3. Should the httpcore API and internals use a context managed request/response interface?

I need to follow up on this one with a bit more docs & chat, but parking it here for now.

[tomchristie]

Potential API alternative... Keep the send interface as a plain ol' return a response, and have a send_and_stream_response for the streaming case.

So, this continues to work the same before and after...

request = client.build_request("GET", "https://www.example.com")
response = client.send(request)

But send(stream=True) is no longer a thing. The streaming case exposed an explicitly different method...

request = client.build_request("GET", "https://www.example.com")
with client.send_and_stream_response(request) as response:
    ...

[florimondmanca]

It seems we still have on_close: Callable = None as part of the Response constructor for now, right? (Probably an oversight given the PR description.)

[florimondmanca]

Thoughts on the _compat.py pattern? Move this bit there and then do from ._compat import asynccontextmanager everywhere. Always found it ends up being worth doing eventually, and keeps compat code in a clear area.

[florimondmanca]

I think this might be worth discussing.

If we give folks client.send() and this returns a response context manager, shouldn't they expect response to be readily-usable, eg response.text returns the text right away?

Why would we want .send() to return a context manager, if we don't actually need that interface except in the streaming case? Having to add response.read() each time as boilerplate seems like a smell, right?

Also, how do we help users understand what the effect of not calling response.read() would be?

Given the pre-existing Requests model of ".request() gives me a response with the body readily available", I wonder about the implications of this.

That's why I ended up re-working #1355 as #1491 — in the end as a user it didn't seem necessary.

Now I'd understand if we define .send() to be the entrypoint for the "HTTPX Transport API". It would be a lower-level thing for advanced use cases that .request() or .stream() don't fulfill. But then there are rather simple cases like "I want a custom/manual Request and send it as usual" which become more unusual, where you get this context manager rather than a Response immediately.

Anyway, curious to see how you'd see the docs evolve. :-)

[tomchristie]

So I'm going to close this off, but a little discussion to illuminate first...

To expand on the motivation of why we might have wanted to do this:

Following in the footsteps of trio's structured concurrency constraints but wrt. resources... making sure that when any OS resource is opened we only do so within a with block, ensuring that we don't give developers a footgun with which to create unclosed resources.

If we wanted to follow that constraint it'd imply that we shouldn't expose any "return an opened response" API, which either means dropping response = client.send(stream=True), either in favour of send always being a context block API, or in favour of a plain send method with no streaming and a client.send_and_stream() method.

But the downsides here are that our call stack gets wrapped in decorators all the way through. Tracebacks are harder to follow, and it's not necessarily true that it's easier for developers to follow the code flow.

There's also the downside, that in some contexts a framework might not provide the constructs that we need in order to be able to work within a context managed fashion. See eg. using starlette to proxy and stream a response, which currently requires a close-callback style. (Which is a pointer to a design failure there, but nonetheless, having to point devs to a "if you really need to do this, here's how to call __(a)enter__/__(a)exit__ directly" section really just isn't graceful.

In any case the .build_request()/.send() pair is already a somewhat lower-level API, with .request() and .stream() providing the nicer "don't give devs an easy footgun" APIs.

So short story. I can't see this as a worthwhile trade off. The "resources MUST be context managed" approach clearly has some nice design properties, but I think it backs us into too much of a corner. (And #998 helps show that we can bridge to that neatly enough for some parts if we really need to.)