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

Fix infinite loop when using StreamingResponse with ASGI client #912

Closed
wants to merge 2 commits into from

Conversation

JayH5
Copy link
Member

@JayH5 JayH5 commented Apr 23, 2020

Fixes #908

Without this, the run_until_first_complete function never returns when using a StreamingResponse with a client like httpx directly connected to the ASGI app. This is because the listen_for_disconnect method will loop forever receiving the same message like:

{'type': 'http.request', 'body': b'', 'more_body': False}

...without control ever going back to the event loop.

I'm not sure of a better solution to this as it seems like this is necessary when working with a combination of a while True loop and asyncio.wait().

starlette/concurrency.py Outdated Show resolved Hide resolved
@JayH5 JayH5 changed the title Fix infinite loop when using StreamingResponse with directly connected client (e.g. httpx) Fix infinite loop when using StreamingResponse with ASGI client Apr 23, 2020
@JayH5 JayH5 force-pushed the gh-908/stream-infinite-loop branch from 525dd4d to 57db734 Compare April 25, 2020 19:45
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.

Just got hit by this bug myself as soon as I plug a middleware built with BaseHTTPMiddleware into my middleware chain, as those middleware classes return StreamingResponse instances.

Tried this fix locally and it works like a charm. Arguably not the most elegant but it makes sense that we should give the event loop some air in this infinite loop.

@tomchristie
Copy link
Member

tomchristie commented May 1, 2020

Doesn't make any sense to me - we are already yielding control to the event loop message = await receive().

@JayH5
Copy link
Member Author

JayH5 commented May 1, 2020

@tomchristie as far as I understand, the await in that await receive() call only allows control to be yielded to the event loop, it doesn't actually do that yielding.

If the receive() call is able to do its work without any blocking operations--as is the case when using the httpx client that always has ASGI messages readily available--then control is never yielded.


@florimondmanca I'm curious about what client you were using? Also httpx?


I'm not super-confident in this PR as it's not clear to me whether it should be the job of the client (httpx) to be doing this yielding in its ASGI backend (hard to do because of async abstractions there). But this is the best I have so far.

@florimondmanca
Copy link
Member

@JayH5 Yes, HTTPX with the AsyncClient.

@tomchristie As Jay said - the await doesn’t actually guarantee that the event loop does task switching somewhere.

Trio has an explicit notion of “checkpoints” for this and I assume it wouldn’t be a problem there, but asyncio is more tricky to get right sometimes because of this.

FWIW I remember we already had to resort to sleep(0) for some asyncio fixes in the past. We have one in HTTPX for a Python 3.6 compatibility issue somewhere that was exactly caused by this problem (asyncio not having the chance to do a round of its event loop).

@florimondmanca
Copy link
Member

@JayH5 I agree that this might be more of a client / HTTPX concern.

In normal conditions (eg when uvicorn is calling the app) the receive() call waits on a socket, which triggers event loop checkpoints.

I assume HTTPX should emulate that and make sure that it doesn’t provide a secretly synchronous receive() function - eg because the body iterator actually draws its chunks from a list or similar.

@JayH5
Copy link
Member Author

JayH5 commented May 12, 2020

Fixed on the client side in encode/httpx#919

@JayH5 JayH5 closed this May 12, 2020
@JayH5 JayH5 deleted the gh-908/stream-infinite-loop branch May 12, 2020 14:00
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.

Starlette 0.13.3 hangs with StreamingResponse & httpx client
3 participants