Skip to content

How about provide non-contextmanager api for stream? #830

Closed

Description

@MisLink

I'm tring to get a stream using httpx and forward it using starlette, Here is the code I expect:

async with httpx.AsyncClient() as client:
    async with client.stream("GET", url) as r:
            return StreamingResponse(r.aiter_raw())

But, because the call of r.aiter_raw is after the call of stream context manager's __aexit__, the code above will raise a ResponseClosed exception.
I don't learn more about the design of httpx and starlette, and I think the simple way is providing non-contextmanager api for stream, just like AsyncClient. It can prevent we use dunder method directly:

async with httpx.AsyncClient() as client:
    ctxm = client.stream("GET", wos_url)
    response = await ctxm.__aenter__()
    return StreamingResponse(
        response.aiter_raw(),
        background=BackgroundTasks([BackgroundTask(ctxm.__aexit__)]),
    )

Activity

  1. lovelydinosaur commented on Feb 27, 2020

    @lovelydinosaur
    Member

    If anything I’d probably say that it indicates Starlette request/response views are awkward for some streaming cases, rather than that we should change httpx.

    You can use httpx.stream and starlette’s StreamingResponse inside a plain ASGI app, which might be useful, eg... https://gist.github.com/tomchristie/5765e10a90a41c7e57470e2dc700f9db

  2. florimondmanca commented on Feb 28, 2020

    @florimondmanca
    Contributor

    One other approach would be to skip the .stream() helper in your case and enter manual mode with .send():

    async with httpx.AsyncClient() as client:
        request = client.build_request("GET", wos_url)
        response = await client.send(request, stream=True)
        return StreamingResponse(
            response.aiter_raw(),
            background=BackgroundTasks([BackgroundTask(response.aclose)]),
        )

    The behavior is the same than with the __aenter__()/__aexit__() pair, but arguably easier on the eyes. :-)

    (.send(stream=...) is public API — see the Client developer interface.)

  3. florimondmanca commented on Feb 28, 2020

    @florimondmanca
    Contributor

    ^ It might be a good idea to document that pattern (which is also valid in the sync case). I think we already mentioned in the past expanding our docs to give .send() some light anyway, so…

  4. added
    questionFurther information is requested
    user-experienceEnsuring that users have a good experience using the library
    on Feb 28, 2020
  5. triztian commented on Feb 28, 2020

    @triztian

    I think I'm running into the same issue, essentially I'm trying to replicate this usage pattern from requests, this works successfully:

    import flask
    import requests
    
    @app.route('/large_file', method=['GET'])
    def large_file():
        return response_with_requests('https://some.url/with/redirect')
    
    
    def response_with_requests(url):
        def gen_stream(r):
            for b in r.iter_content(chunk_size=1024):
                yield b
    
        resp = requests.get(url, stream=True)
        return flask.Response(gen_stream(resp), content_type='application/octet-stream')

    And this is my httpx version, it always fails with httpx.exception.ResponseClosed

    import flask
    import httpx
    
    @app.route('/large_file', method=['GET'])
    def large_file():
        return response_with_httpx('https://some.url/with/redirect')
    
    
    def response_with_httpx(url):
        with httpx.stream('GET', url, allow_redirects=True) as r:
            def gen_stream(r):
                for b in r.iter_bytes():
                    yield b
    
            return flask.Response(gen_stream(r), content_type='application/octet-stream')

    What I can infer from @MisLink is that the dunder __exit__ method is called as soon as I return my response object; this marks the stream as is_closed (checked in models.py, in theiter_raw(self)) which raises a httpx.exception.ResponseClose and causes for the request to fail. For my use case I'm not using any of the async APIs.

    I agree that maybe there should be a non-context manager API for streaming.

  6. florimondmanca commented on Feb 28, 2020

    @florimondmanca
    Contributor

    @triztian Essentially the « manual » mode with .send() is the non-context managed streaming usage.

    You can adapt my async snippet from above to your sync use case: use a Client, use .send(), drop any async/await annotations, make sure to plug into Flask’s callback system to register response.close() (otherwise you’ll end up with dangling connections! This is the original motivation for encouraging context managed usage) and you should be good.

    Let me know if you’re able to make it work. :)

  7. triztian commented on Feb 28, 2020

    @triztian

    I was able to create a working solution by using the client API, thanks to @florimondmanca for mentioning it, however I think had he not mentioned it I would've spent a lot of time looking for an answer:

    def response_with_httpx_client(url):
        client = httpx.Client()
        req = client.build_request('GET', url)
        resp = client.send(req, stream=True, allow_redirects=True)
    
        return Response(resp.iter_bytes(), content_type='application/octet-stream')

    I think it would be really helpful to include this solution as part of the documentation in the Streaming Responses section, considering that most people that move from requests to httpx will find my first initial working solution and then land on the following page: Streaming Responses

    I also struggled a bit with the Developer Interface: Client documentation, it includes the following example:

    >>> client = httpx.Client()
    >>> response = client.get('https://example.org')

    However the client.get method does not support the stream argument, yet requests.get does support it, while 100% equal APIs are not to be expected it can be a little confusing for newcomers to httpx (like myself)


    Happy to contribute these changes, a special section under the README documentation that aids in requests migration from common patterns would be helpful.

  8. MisLink commented on Feb 28, 2020

    @MisLink
    Author

    @tomchristie @florimondmanca thanks for your advices! send() api is good enough, using plain asgi protocol is also cute. And I think maybe somewhere could remind people content in StreamingResponse is a awaitable object even in sync call.
    I confused httpx and starlette... don't mind.

  9. florimondmanca commented on Mar 1, 2020

    @florimondmanca
    Contributor

    @triztian

    However the client.get method does not support the stream argument, yet requests.get does support it

    This is a deliberate design decision on our end -- see #588 for background. :-)

  10. florimondmanca commented on Mar 1, 2020

    @florimondmanca
    Contributor

    Opened #840 with a docs update. It uses a different style (based on ExitStack/AsyncExitStack) than reaching into the .send() API so that .stream() remains the sole recommended entrypoint to streaming responses. Free to leave comments there!

  11. lovelydinosaur commented on Mar 2, 2020

    @lovelydinosaur
    Member

    It uses a different style (based on ExitStack/AsyncExitStack) than reaching into the .send()

    I think documenting out build_request and send ar eprobably the right option here. I still want to stear folks away from this wherever possible, because it's a really good way of failing to close streaming response (vs. the context managed case, which protects you from that.)

  12. florimondmanca commented on Jun 16, 2020

    @florimondmanca
    Contributor

    So, reading over this again, what do we think of this? …

    async with httpx.AsyncClient() as client:
        async with await client.stream("GET", url) as response:
            ...

    Note the extra await there, similar to trio's open_file() API.

    The idea would be to drop the "get the response on enter idea", and instead do this:

    • Add context manager + async context manager methods to Response. __enter__ and __aenter__ are blank, and __exit__/__aexit__ call .close() / .aclose().
    • Make Client.stream() / await AsyncClient.stream() return a Response instance (instead of a context manager that we'd have to __enter__() to get the response object).

    Non-context managed usage would then look like this:

    async with httpx.AsyncClient() as client:
        response = await client.stream("GET", url)
        return StreamingResponse(
            response.aiter_raw(),
            background=BackgroundTasks([BackgroundTask(response.aclose)]),
        )

    Which seems like a rather natural transition form the context-managed usage (drop the async with and make sure to aclose the response).

    Obviously this would be a breaking change (users need to add the extra await), but IMO one we might want to make before 1.0 lands.

    For background, originally we discussed "responses as context managers" in #393. This was kept open as relevant for a while, and then considered "obsoleted" by the .stream() API (#600).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    questionFurther information is requesteduser-experienceEnsuring that users have a good experience using the library

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions