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

Switch to request context manager interface #1355

Closed
wants to merge 5 commits into from

Conversation

florimondmanca
Copy link
Member

@florimondmanca florimondmanca commented Oct 8, 2020

Alternative to #1341, taking into account the POC here: https://gist.github.com/tomchristie/e6e5e06b6c7b77b9c726b74d1ba0acbc

The change footprint in this PR is much larger than #1341, because this also changes the following, introducing breaking API changes:

  • (Breaking)Response drops .close() and .aclose(), and .is_closed. In fact, we don't do any tracking of "open/closed" stream state on responses anymore, because we don't need it anymore. We also drop the ResponseClosed exception as a result.
  • (Breaking) Client.send() changes from returning an "open" Response to a ContextManager[Response].
  • (Breaking) AsyncClient.send() changes from returning an "open" Response to a AsyncContextManager[Response].
  • The internal StreamContextManager disappears — we don't need it anymore.

Docs also updated to reflect these items.

Obviously the scope is larger than what #1341 does, but it's because this PR includes very-likely follow-ups to #1341 that we'd have had to do after #1341 anyway, to keep things most consitent.

Still worth noting that, just like #1341, this PR keeps HTTPX working against HTTPCore 0.12, or HTTPCore once encode/httpcore#206 is merged… (Thanks to the ensure_(async_)context_manager helpers.)

@florimondmanca florimondmanca added api change PRs that contain breaking public API changes httpcore Issues related to HTTPCore (core HTTP networking layer) - https://github.com/encode/httpcore labels Oct 8, 2020
@florimondmanca florimondmanca requested a review from a team October 8, 2020 18:00
@tomchristie
Copy link
Member

tomchristie commented Oct 9, 2020

Fantastic! So, things to note here... at some point I mentioned that we wouldn't need Response.close(). Well, it's not actually clear exactly what we want there, but it might make sense to keep it.

Here's the deal...

with httpx.stream("GET", "https://www.example.com") as response:
    pass
response.read()

Currently that'll raise httpx.ResponseClosed. Which we'd want it to do.

We could just drop .close() and leave it up to the stream iterator to raise whatever exception it chooses if you read it outside of the transport request scope, but that's clearly not what we want for the httpx API, as we do want to make sure we're only ever raising httpx exceptions at this level.

So, I think we do end up wanting to keep response.close(), it's just that it's a bit different because it will no longer perform any I/O or call into an underlying stream.close()...

  • _send_single_request would call response.close() as soon as it exits the request with block, indicating that the stream is no longer usable.
  • response.close() would mark the response as closed, but we'd drop the on_close, and we wouldn't call into anything else.
  • We wouldn't need response.aclose(), because it's not actually an I/O operation at all.

?...

@florimondmanca
Copy link
Member Author

@tomchristie Yup, I got to the point of just removing on_close and keeping .close() as a shim around switching the .is_closed flag, but then couldn't think of a case where we'd actually want to call that method, either internally or by the user. Your example makes sense, though.

@florimondmanca
Copy link
Member Author

The main thing I don't like about this anymore is the breaking change "Client.send() changes from returning an "open" Response to a ContextManager[Response]". I think it's perfectly fine to keep returning a maybe-open Response in all cases. I'm actually not sure why we'd need so many changes in HTTPX anymore. I'll close this and open a fresh one. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change PRs that contain breaking public API changes httpcore Issues related to HTTPCore (core HTTP networking layer) - https://github.com/encode/httpcore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants