encode / uvicorn

An ASGI web server, for Python. 🦄
https://www.uvicorn.org/
BSD 3-Clause "New" or "Revised" License
8.41k stars 728 forks source link

await request.is_disconnected() brings up large ClientDisconnected error in Uvicorn v0.28.0 #2271

Closed Kludex closed 6 months ago

Kludex commented 6 months ago

Discussed in https://github.com/encode/uvicorn/discussions/2270

Originally posted by **bdashore3** March 10, 2024 Hi there, I'm currently trying to serve an SSE streaming response using the latest versions of Uvicorn (0.28.0) and FastAPI (0.110.0). But, with the latest version of Uvicorn, I am getting a large error message regarding `ClientDisconnected` that's being logged whenever I call `await request.is_disconnected()` or have an exception handler on `CancelledError`. I believe this gets raised due to the following PR: https://github.com/encode/uvicorn/pull/2220 In addition, I'm using the starlette version that FastAPI installs automatically (v0.26.3) How would I suppress the `ClientDisconnected` exception since I already know the request is disconnected when this traceback is logged? I wrote up a couple of MREs explaining this issue (both require FastAPI, and Uvicorn at their latest versions): This MRE fires the error when a CancelledError exception handler is triggered: ```py import asyncio import uvicorn import json from fastapi import FastAPI, Request from fastapi.responses import StreamingResponse app = FastAPI() def get_sse_packet(json_data: str): """Get an SSE packet.""" return f"data: {json_data}\n\n" # Completions endpoint @app.get("/stream") async def stream_output(request: Request): async def generator(): try: for i in range(10): if await request.is_disconnected(): print("Disconnect triggered!") break await asyncio.sleep(1) yield get_sse_packet(json.dumps({'testnum': i})) except asyncio.CancelledError: # Here, the print gets logged, then the exception gets sent as well print("Cancelled exception triggered!") except Exception: print( "Exception triggered!" ) return StreamingResponse( generator(), media_type="text/event-stream", ) if __name__ == "__main__": uvicorn.run( app, host="0.0.0.0", port=5000, ) ``` This MRE fires the error on the next iteration of the generator when `await request.is_disconnected()` is run: ```py import uvicorn import json import time from fastapi import FastAPI, Request from fastapi.responses import StreamingResponse app = FastAPI() def test_generator(): for i in range(10): time.sleep(1) yield i def get_sse_packet(json_data: str): """Get an SSE packet.""" return f"data: {json_data}\n\n" # Completions endpoint @app.get("/stream") async def stream_output(request: Request): async def generator(): try: for i in test_generator(): if await request.is_disconnected(): # Here, the print gets logged, but the error gets logged too... print("Disconnect triggered!") break yield get_sse_packet(json.dumps({'testnum': i})) except Exception: print( "Exception triggered!" ) return StreamingResponse( generator(), media_type="text/event-stream", ) if __name__ == "__main__": uvicorn.run( app, host="0.0.0.0", port=5000, ) ``` Traceback (gets logged after the disconnect or the CancelledError): ``` ERROR: Exception in ASGI application + Exception Group Traceback (most recent call last): | File "D:\python-test\venv\Lib\site-packages\uvicorn\protocols\http\h11_impl.py", line 408, in run_asgi | result = await app( # type: ignore[func-returns-value] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | File "D:\python-test\venv\Lib\site-packages\uvicorn\middleware\proxy_headers.py", line 69, in __call__ | return await self.app(scope, receive, send) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | File "D:\python-test\venv\Lib\site-packages\fastapi\applications.py", line 1054, in __call__ | await super().__call__(scope, receive, send) | File "D:\python-test\venv\Lib\site-packages\starlette\applications.py", line 123, in __call__ | await self.middleware_stack(scope, receive, send) | File "D:\python-test\venv\Lib\site-packages\starlette\middleware\errors.py", line 186, in __call__ | raise exc | File "D:\python-test\venv\Lib\site-packages\starlette\middleware\errors.py", line 164, in __call__ | await self.app(scope, receive, _send) | File "D:\python-test\venv\Lib\site-packages\starlette\middleware\exceptions.py", line 62, in __call__ | await wrap_app_handling_exceptions(self.app, conn)(scope, receive, send) | File "D:\python-test\venv\Lib\site-packages\starlette\_exception_handler.py", line 64, in wrapped_app | raise exc | File "D:\python-test\venv\Lib\site-packages\starlette\_exception_handler.py", line 53, in wrapped_app | await app(scope, receive, sender) | File "D:\python-test\venv\Lib\site-packages\starlette\routing.py", line 758, in __call__ | await self.middleware_stack(scope, receive, send) | File "D:\python-test\venv\Lib\site-packages\starlette\routing.py", line 778, in app | await route.handle(scope, receive, send) | File "D:\python-test\venv\Lib\site-packages\starlette\routing.py", line 299, in handle | await self.app(scope, receive, send) | File "D:\python-test\venv\Lib\site-packages\starlette\routing.py", line 79, in app | await wrap_app_handling_exceptions(app, request)(scope, receive, send) | File "D:\python-test\venv\Lib\site-packages\starlette\_exception_handler.py", line 64, in wrapped_app | raise exc | File "D:\python-test\venv\Lib\site-packages\starlette\_exception_handler.py", line 53, in wrapped_app | await app(scope, receive, sender) | File "D:\python-test\venv\Lib\site-packages\starlette\routing.py", line 77, in app | await response(scope, receive, send) | File "D:\python-test\venv\Lib\site-packages\starlette\responses.py", line 257, in __call__ | async with anyio.create_task_group() as task_group: | File "D:\python-test\venv\Lib\site-packages\anyio\_backends\_asyncio.py", line 678, in __aexit__ | raise BaseExceptionGroup( | ExceptionGroup: unhandled errors in a TaskGroup (1 sub-exception) +-+---------------- 1 ---------------- | Traceback (most recent call last): | File "D:\python-test\venv\Lib\site-packages\starlette\responses.py", line 260, in wrap | await func() | File "D:\python-test\venv\Lib\site-packages\starlette\responses.py", line 254, in stream_response | await send({"type": "http.response.body", "body": b"", "more_body": False}) | File "D:\python-test\venv\Lib\site-packages\starlette\_exception_handler.py", line 50, in sender | await send(message) | File "D:\python-test\venv\Lib\site-packages\starlette\_exception_handler.py", line 50, in sender | await send(message) | File "D:\python-test\venv\Lib\site-packages\starlette\middleware\errors.py", line 161, in _send | await send(message) | File "D:\python-test\venv\Lib\site-packages\uvicorn\protocols\http\h11_impl.py", line 461, in send | raise ClientDisconnected | uvicorn.protocols.utils.ClientDisconnected +------------------------------------ ```

[!IMPORTANT]

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.

Fund with Polar

bdashore3 commented 6 months ago

So, I did a little digging into Starlette and the problematic line of code is Here. Maybe an issue should be opened there as well?

The problem stems from sending responses to an already disconnected request. Previously, Uvicorn just had a pass if the request is disconnected, but now it raises a ClientDisconnected error which isn't handled in Starlette for that case.

While this should be fixed (or handled appropriately) in Starlette, FastAPI doesn't support starlette > v0.36.x.

For a workaround, I'm using sse-starlette which doesn't have this issue and won't bring dependency conflicts with FastAPI.

The main takeaway is to not yield after a request is disconnected (or check if it's disconnected before yielding). Hopefully this helps someone with the same problem!

Kludex commented 6 months ago

Since a lot of people are watching this, can someone create a PR reverting the PR that caused this?

We need a backwards-compatible strategy here before adding this feature again.

EwertonDCSilv commented 6 months ago

Estou com o mesmo problema !

EwertonDCSilv commented 6 months ago

Então, investiguei um pouco o Starlette e a linha de código problemática está Here . Talvez um problema deva ser aberto lá também?

O problema decorre do envio de respostas a uma solicitação já desconectada. Anteriormente, o Uvicorn só tinha uma passsolicitação se a solicitação fosse desconectada, mas agora gera um ClientDisconnectederro que não é tratado no Starlette para esse caso.

Embora isso deva ser corrigido (ou tratado adequadamente) no Starlette, FastAPI não oferece suporte a starlette > v0.36.x.

Para uma solução alternativa, estou usando o sse-starlette que não apresenta esse problema e não trará conflitos de dependência com FastAPI.

A principal lição é não ceder depois que uma solicitação for desconectada (ou verificar se ela está desconectada antes de ceder). Espero que isso ajude alguém com o mesmo problema!

Tem alguma previsão de suporte a isso @tiangolo ?

EwertonDCSilv commented 6 months ago

Um caminho que encontrei foi voltar para o uvicorn[standard]==0.27.0 que é anterior a PR que adiciona o comportamento indesejado image

image

jonathan-s commented 6 months ago

Wanted to fill in that I'm seeing this when using the wsgimiddleware provided by fastapi while using unicorn 0.28.0

Kludex commented 6 months ago

The fix will be released on 0.28.1 in a couple of hours.