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

Turn transport.request() into a context manager #206

Closed
wants to merge 22 commits into from

Conversation

florimondmanca
Copy link
Member

@florimondmanca florimondmanca commented Oct 3, 2020

Closes #145

Note: recommend reviewing with "Hide whitespace changes" turned on.

This PR introduces a breaking change to the transport API:

Before:

async with Transport() as http:
    response = await http.request(...)
    ...

After:

async with Transport() as http:
    async with http.request(...) as response:
        ...

From my point of view, motivation is two-fold:

  • Move towards more strictly observing principles of structured concurrency. One of the main manifestations of this is preferring to rely on a with syntax block, rather than using e.g. wrapper classes that hold a close callback. This should make the transport API more correct to use, meaning less chances to shoot oneself in the foot by forgetting a callback somewhere.
  • Facilitate per-request holding of I/O state, making things like supporting streaming ASGI responses not only possible, but also very easy to implement. (See Add support for streaming responses to ASGITransport httpx#998)

There are trade-offs to consider on the user side of things:

  • Callback-driven clients and APIs will have to interface with HTTPCore's preference for with blocks. This can be done correctly and reliably with the help of exit stacks, which convert the syntactical scope of a with block into a programmatic scope (via .enter_context() and .close()), but I'd reckon this could introduce friction.
  • For HTTPX, embracing the with syntax seems like the most promising option: Switch to request context manager interface httpx#1355. (For the record, keeping its callback-driven nature was also attempted: Add support for context manager responses httpx#1341.)

@florimondmanca florimondmanca added the enhancement New feature or request label Oct 3, 2020
@florimondmanca
Copy link
Member Author

@tomchristie It's a big one, but at least this is where I think we want to go, and we can now find ways to possibly get this in incrementally (not sure how, but you always have better ideas than me on that front :-)).

@florimondmanca
Copy link
Member Author

There's a follow up to this related to point 3/ in encode/httpx#1274 (comment), being that byte streams now don't really need a .close()/.aclose() method, since we're able to call any close callback in a finally: ... block around the yield statement. But I think we should treat it as a follow-up, so that this PR remains focused on the core API change?

Copy link
Member Author

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

@tomchristie I've gone over the PR again and I'm pretty happy with how this looks. At least implementation-wise this is clean.

@@ -1,7 +1,7 @@
from ._async.base import AsyncByteStream, AsyncHTTPTransport
from ._async.base import AsyncHTTPTransport
Copy link
Member Author

@florimondmanca florimondmanca Nov 30, 2020

Choose a reason for hiding this comment

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

I guess we just need to be careful that this drops SyncByteStream and AsyncByteStream.

We rely on these in a few places in HTTPX: https://github.com/encode/httpx/pull/1399/files#diff-f7cafd3bb44d8d834be724482dacf4ba57c93e7faa62bdce50fc05c00557e222R79

Some other implementations out there would also still be relying on these interfaces.

Should we perhaps keep some aliases in place until 1.0…?

from typing import Iterable, AsyncIterable

SyncByteStream = Iterable[bytes]
AsyncByteStream = AsyncIterable[bytes]

… Or just delay merging these "request context manager" PRs until the next minor release cycle? (We're pinning HTTPCore to a minor in HTTPX anyway.) Yeah, thinking about it — I think that's probably the plan? :-)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I don't think we're sufficiently confident yet about exactly where we're going with this interface,
so I reckon rolling the other stuff (where we are confident) and keeping this hanging a little longer.

@tomchristie
Copy link
Member

Closing in line with encode/httpx#1491

Goodness me but it's been a long road.

I think we can be very happy with the Transport API we're actually going to end up with encode/httpx#1522 - going all the way with context managed resources here just feels like too much of a heavy trade off.

We could potentially still choose to use context managed approaches internally within httpcore if we wanted, since we can see how to tackle bridging between the two neatly enough. (Eg. see encode/httpx#998)

@tomchristie tomchristie deleted the fm/request-context-manager branch April 19, 2021 11:09
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.

Should Transport.request() return a context manager?
2 participants