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

Stream interface #1550

Merged
merged 9 commits into from Apr 13, 2021
Merged

Stream interface #1550

merged 9 commits into from Apr 13, 2021

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Apr 1, 2021

This pull request allows us to compare the following two alternatives...

  • The HTTPX Transport API, using stream as a plain primitive Iterable[bytes]/AsyncIterable[bytes], with close/aclose extensions for handling close callbacks.
  • The HTTPX Transport API, using simple httpx.SyncByteStream/httpx.AsyncByteStream interfaces for streams.

Note that the request.stream and response.stream interfaces also change as a result of this.

The pros of this approach are that the Transport API presents a more natural interface...

Before:

with httpx.HTTPTransport() as transport:
    status_code, headers, stream, extensions = transport.handle_request(
        method=b'GET',
        url=(b'https', b'www.example.com', 443, b'/'),
        headers=[(b'Host', b'www.example.com')],
        stream=[],
        extensions={}
    )
    try:
        body = [part for part in stream]
    finally:
        if "close" in extensions:
            extensions["close"]()

After:

Standard case:

with httpx.HTTPTransport() as transport:
    status_code, headers, stream, extensions = transport.handle_request(
        method=b'GET',
        url=(b'https', b'www.example.com', 443, b'/'),
        headers=[(b'Host', b'www.example.com')],
        stream=httpx.ByteStream(b""),
        extensions={}
    )
    body = stream.read()

Streaming case:

with httpx.HTTPTransport() as transport:
    status_code, headers, stream, extensions = transport.handle_request(
        method=b'GET',
        url=(b'https', b'www.example.com', 443, b'/'),
        headers=[(b'Host', b'www.example.com')],
        stream=httpx.ByteStream(b""),
        extensions={}
    )
    try:
        body = [part for part in stream]
    finally:
        stream.close()

The cons of this approach are that we're adding httpx.SyncByteStream and httpx.AsyncByteStream instead of the "handle_request is just plain datatypes everywhere". We're also adding the httpx.ByteStream() class for the simple concrete bytes-only case.

Having had a bunch of time to think over the three possible cases...

  • A stream interface
  • A plain bytes interface, with close/aclose extensions to handle resource closing.
  • A plain bytes interface, with context management to handle resource closing.

My feel is that this one probably hits the sweet spot of "nice low-level interface" while still being neatly usable and obvious from a usability perspective.

It's also lower impact if we want to update httpcore with an equivalent interface, since encode/httpcore#296 in it's current state already matches up with this. (But gets more complex if we start adding in close/aclose extensions.)

@tomchristie
Copy link
Member Author

NB.This change also allows us to trivially drop the kinda-implementation-detail Response(..., on_close=...) argument, in favour of just calling stream.close(), but I've omitted that on this pass. Because, ya'know.

@tomchristie tomchristie mentioned this pull request Apr 8, 2021
10 tasks
Copy link
Member

@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.

So, I take this as essentially moving from Iterable/AsyncIterable which are somewhat too low-level / not super user-friendly, to well-defined HTTPX interfaces. I think I quite like that. At least no strong "no", and I agree the resulting close-handling is more intuitive as a result.

httpx/_transports/asgi.py Outdated Show resolved Hide resolved
httpx/_transports/base.py Outdated Show resolved Hide resolved
httpx/_transports/default.py Outdated Show resolved Hide resolved
httpx/_transports/default.py Outdated Show resolved Hide resolved
httpx/_transports/wsgi.py Outdated Show resolved Hide resolved
tomchristie and others added 3 commits April 12, 2021 12:03
Co-authored-by: Florimond Manca <florimond.manca@gmail.com>
Co-authored-by: Florimond Manca <florimond.manca@gmail.com>
@tomchristie
Copy link
Member Author

tomchristie commented Apr 12, 2021

@florimondmanca That's it, yup. Just a nicer more usable interface at the Transport API level.

We don't expect the vast majority of our users to be working with that directly, but it's probably nicer for cases where folks are really digging in.

It would also potentially allow us to address Seth's comment here at a future date...

I'd prefer a file-like API for response body for better control of the data that is pulled.

...because we could also add support for read(max_bytes=...) if we chose to.

Although there's also also reasons we might not really want to go that route, which I'm not going to go into unless/until that convo actually gets raised as a prospect. Either ways around, it's clearly a more file-like API, even if chunk size is declared at a transport-init level, rather than per-read.

@tomchristie
Copy link
Member Author

Righty, let's push on with this...

@tomchristie tomchristie merged commit 110ce85 into master Apr 13, 2021
@tomchristie tomchristie deleted the stream-interface branch April 13, 2021 12:14
@johtso
Copy link
Contributor

johtso commented Apr 13, 2021

Looks good, is this basically the same as the previous functionality, but pulled into httpx? Wasn't over the moon about the close function in extensions, so this seems better.

@tomchristie
Copy link
Member Author

It does, doesn't it?

@tomchristie tomchristie mentioned this pull request Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants