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

Add support for response context manager interface #1491

Closed
wants to merge 4 commits into from

Conversation

florimondmanca
Copy link
Member

@florimondmanca florimondmanca commented Mar 1, 2021

Prompted by #1355 (comment)

This PR is now a plain refactor that adds support for HTTPCore returning context-managed responses, should we choose to move forward with encode/httpcore#206.

This ends up basically being a re-issuing of #1341.

I know we discussed "let's just switch to context managers throughout the stack" (#1341 (comment)), but eventually all the changes in #1355 didn't seem to be worth the hassle to me. If all we want is to make the Transport API more resource-leak-safe, then it shouldn't change anything to HTTPX's API since we already handle resource closure properly here.

We could look at reorganizing things to use the context-managed interface more thoroughly internally (to simplify a bunch of try/except/finally code around close() and aclose()), but all I'm saying is that it doesn't matter as far as compatibility with encode/httpcore#206 is concerned.

@florimondmanca florimondmanca added the refactor Issues and PRs related to code refactoring label Mar 1, 2021
@florimondmanca florimondmanca force-pushed the fm/response-ctx-manager branch from 8c4d082 to 630d5e8 Compare March 1, 2021 14:22
@florimondmanca florimondmanca marked this pull request as draft March 9, 2021 18:51
@florimondmanca florimondmanca marked this pull request as ready for review March 13, 2021 17:17
@tomchristie
Copy link
Member

Okay, after much hair-tearing I think it's time for us to call a close on this.

There's no other existing Python networking libraries that adopts this approach wrt. handling the network close, and while it does have some point to recommend it, it also results in longer tracebacks, more awkward under the hood wrangling, and ultimately a Transport API that'd be less obvious to most of our user base, than a standard ol' file-like-ish stream with .close()/.aclose() methods.

@adriangb
Copy link
Member

adriangb commented Sep 6, 2022

There's no other existing Python networking libraries that adopts this approach wrt. handling the network close

@tomchristie I may be missing something, but doesn't aiohttp exclusively use context managers?

@tomchristie
Copy link
Member

doesn't aiohttp exclusively use context managers?

I'm not sure what the API for that was at the time I originally made that note.

So...

Yes they do. At the response level.

But not within the lower-level stack or at their Connector API layer.

We also exclusively use context managers at the response level. Ie. You need to use a client.stream block if you want to hold the connection open, while other methods return a closed response.

But we don't use them at the transport API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues and PRs related to code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants