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

Support streaming response in asgi transport with asyncio #994

Closed
wants to merge 1 commit into from
Closed

Support streaming response in asgi transport with asyncio #994

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 24, 2020

I haven't written full tests for this yet and have mainly done manual testing, but I want to submit this to discussion first in order not to do unnecessary work, because there are some uncertainties in this approach.

Basically I have started to use the httpx client to be able to do HTTP requests and direct ASGI requests using the same API and it works great for that! However, when I started doing streaming requests I found out that the ASGI transport needs to read the full response before returning it, which naturally doesn't work for streaming at all.

I wrote an implementation using asyncio and tasks, but the problem is that this is not trio compatible at all. Bigger problem is that I can't figure out how to make it trio compatible without refactoring the whole AsyncHTTPTransport interface.

Basically what we need to do for the streaming to work for the ASGI interface is that we first need to run the ASGI application in the background up to (but not any further than) receiving all the headers before the body is received. Then we need to construct a stream out of the following messages that contain the body and return that with the header information.

The way this needs to be done in trio is that we should have a higher level nursery that would create two background tasks, one for running the app and another one for waiting for the response headers to be finished. Once the response headers are fetched we would start reading the body inside the same nursery context, without closing the stream. To be honest I'm not quite sure how to do this, so I simply gave up and write only asyncio support.

Let me know what you think and if any of this makes any sense to you?

@tomchristie
Copy link
Member

Thanks for having an initial dig into this.

In a sense this is actually quite a useful point of reference for making sure we've finalised everything we'd need for 1.0, because supporting streaming responses in ASGI with trio requires nurseries, which means a transport implementing __aenter__ and __aexit__. We're not currently calling into those from the client, but we will want to, or else do something similar.

A trio compatible approach would need to...

  • Add __aenter__ and __aexit__ methods to ASGITransport.
  • Have a nursery on the class, that is responsible for running ASGI apps, have the scope on that nursery managed from the class __aenter__ and __aexit__.

There's more to dig into here, and I don't have time to flesh this out more, but just jotting some of this down to start.

@ghost
Copy link
Author

ghost commented May 25, 2020

Thanks @tomchristie for having a look and for your thoughts on the trio compatibility.

I must say I like the context manager approach, but I also read through #998 and I'm happy to close this PR in favour of that one. While it feels quite hacky it does seem to do the job for now, so maybe go with that approach first for the next release and consider adding context manager support in the next version bump?

@ghost
Copy link
Author

ghost commented Jun 1, 2020

Closing this in favor of #998, I still hope we can get a fix to the next release since we have very long running HTTP requests and not having ASGI streaming support makes the ASGI transport quite useless in those cases.

@ghost ghost closed this Jun 1, 2020
This pull request was closed.
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.

1 participant