RuntimeError with custom middleware when client drops connection before request finished
#1527
-
The issueWhen using two or more custom middleware that subclass How to reproduceCreate the following file # example.py:
import asyncio
from starlette.applications import Starlette
from starlette.middleware import Middleware
from starlette.middleware.base import BaseHTTPMiddleware
from starlette.responses import JSONResponse
from starlette.routing import Route
async def homepage(request):
await asyncio.sleep(5)
return JSONResponse({"hello": "world"})
routes = [
Route("/", homepage, name="homepage"),
]
class CustomHeaderMiddleware1(BaseHTTPMiddleware):
async def dispatch(self, request, call_next):
return await call_next(request)
class CustomHeaderMiddleware2(BaseHTTPMiddleware):
async def dispatch(self, request, call_next):
return await call_next(request)
all_middleware = [
Middleware(CustomHeaderMiddleware1),
Middleware(CustomHeaderMiddleware2),
]
app = Starlette(debug=True, routes=routes, middleware=all_middleware)Run server: Send HTTP request and disconnect before response is received: You then get the following error on the server: Is there something we are doing wrong or is this an issue? |
Beta Was this translation helpful? Give feedback.
Replies: 9 comments 14 replies
-
|
I have noticed exactly the same problem with I am currently using following middleware as a quickfix (this middleware needs to be registered first). The middleware basically just catches the # -*- encoding: utf-8 -*-
# ! python3
from __future__ import annotations
import logging
from http import HTTPStatus
from typing import Final
from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint
from starlette.requests import Request
from starlette.responses import Response
logger: Final = logging.getLogger(__name__)
class SuppressNoResponseReturnedMiddleware(BaseHTTPMiddleware):
async def dispatch(self, request: Request, call_next: RequestResponseEndpoint) -> Response:
try:
response = await call_next(request)
except RuntimeError as e:
if await request.is_disconnected() and str(e) == "No response returned.":
logger.warning(
"Error `No response returned` detected. "
"At the same time we know that the client is disconnected "
"and not waiting for a response."
)
return Response(status_code=HTTPStatus.NO_CONTENT)
else:
raise
return response |
Beta Was this translation helpful? Give feedback.
-
|
Me and my team stumbled upon the same issue couple of days ago and implemented the same kind of middleware as a workaround (it has to be the last middleware that was installed by the call I even got a pull request ready with the correction and was about to start discussion on this but you beat me to it. Anyway, I think that the main issue here is that the error is misleading and below correction in the diff --git a/starlette/middleware/base.py b/starlette/middleware/base.py
index bfb4a54..4da49aa 100644
--- a/starlette/middleware/base.py
+++ b/starlette/middleware/base.py
@@ -2,7 +2,7 @@ import typing
import anyio
-from starlette.requests import Request
+from starlette.requests import Request, ClientDisconnect
from starlette.responses import Response, StreamingResponse
from starlette.types import ASGIApp, Receive, Scope, Send
@@ -42,6 +42,8 @@ class BaseHTTPMiddleware:
except anyio.EndOfStream:
if app_exc is not None:
raise app_exc
+ if await request.is_disconnected():
+ raise ClientDisconnect("Client disconnect while processing the request")
raise RuntimeError("No response returned.")
assert message["type"] == "http.response.start" |
Beta Was this translation helpful? Give feedback.
-
|
We have the same issue with The workaround seems to work. |
Beta Was this translation helpful? Give feedback.
-
|
didn't worked for me... steps to replicate:
|
Beta Was this translation helpful? Give feedback.
-
|
I got the same here: Requirements: Exception: |
Beta Was this translation helpful? Give feedback.
-
|
Quite a few people complaining about exactly this problem in different issue reports around Starlette, FastAPI and other frameworks using the underlying toolset. See for example: For many peeps it's really hard to relate what you see in this particular stack trace to this discussion, but indeed I also spotted this issue when client disconnects abruptly. I used @Kludex IMHO, instead of closing comments on resolved issues, it would be nice to let community to leave pointers to the correct remedy. Kudos to @kprzybyla for finding the workaround! 🙇 However, there seems to be a large number of cases, when this error will be hitting the screen, because provided workarounds doesn't handle all situations. For example: if you hit Just in case, my Also maybe important to note, it doesn't matter whether you add a custom middleware or not. I tried to run I'd love to contribute to the resolution of the issue, but I'm afraid I'm quite useless here, except nagging to open an issue so others would be aware of the problem 😊 NB: I would never look into Discussions if I wouldn't see @Kludex point out to do so in this comment: #1255 (comment) |
Beta Was this translation helpful? Give feedback.
-
We have numerous issues that come into light after years of inactivity, which are, in general, new issues - even if the issue is similar. |
Beta Was this translation helpful? Give feedback.
-
|
No worries, it was just a humble suggestion from not too much educated user of the tool 😇 |
Beta Was this translation helpful? Give feedback.
-
You added the |
Beta Was this translation helpful? Give feedback.
-
|
Adding Here is my middleware just in case: from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint
from starlette.requests import Request
from starlette.responses import Response
from starlette.types import ASGIApp
from prometheus_client import Gauge, CONTENT_TYPE_LATEST, REGISTRY, generate_latest
METRICS_HEADERS = {'Content-Type': CONTENT_TYPE_LATEST}
REQUESTS_IN_PROGRESS = Gauge(
'starlette_requests_in_progress',
'Gauge of requests by method and path currently being processed',
)
class PrometheusMiddleware(BaseHTTPMiddleware):
def __init__(self, app: ASGIApp) -> None:
super().__init__(app)
async def dispatch(self, request: Request, call_next: RequestResponseEndpoint) -> Response:
REQUESTS_IN_PROGRESS.inc()
try:
return await call_next(request)
except RuntimeError as e:
if await request.is_disconnected():
return Response(status_code=204)
else:
raise
finally:
REQUESTS_IN_PROGRESS.dec()
def metrics(request: Request) -> Response:
return Response(generate_latest(REGISTRY), headers=METRICS_HEADERS)and then I jus add it to the FastAPI stack with: app = FastAPI(title=PROJECT_NAME, debug=DEBUG, version=VERSION, openapi_url='/openapi.json')
from app.core.metrics import PrometheusMiddleware, metrics
app.add_middleware(PrometheusMiddleware)
app.add_route('/metrics', metrics) |
Beta Was this translation helpful? Give feedback.
-
|
Just to clarify on In other words - in my case |
Beta Was this translation helpful? Give feedback.
-
|
To help here, you can try to see if the problem is solved using the If it doesn't help, see if the problem is solved using #1441. |
Beta Was this translation helpful? Give feedback.
-
|
Tried both suggestions - didn't help. This is what I observed in my use case (which seems to be pretty generic, except the use of
|
Beta Was this translation helpful? Give feedback.
-
|
@Kludex Here is another interesting observation:
The errors amount is almost equal to the concurrency level I used (>95). So I suspect there might be not properly closed responses (couldn't find better words to explain a hypothesis) that are preventing the server from being shutdown successfully without draining them first or having If I apply #1441 and reproduce these 3 steps, then |
Beta Was this translation helpful? Give feedback.
-
|
I'm running the example provided above: import asyncio
from starlette.applications import Starlette
from starlette.middleware import Middleware
from starlette.middleware.base import BaseHTTPMiddleware
from starlette.responses import JSONResponse
from starlette.routing import Route
async def homepage(request):
await asyncio.sleep(5)
return JSONResponse({"hello": "world"})
routes = [
Route("/", homepage, name="homepage"),
]
class CustomHeaderMiddleware1(BaseHTTPMiddleware):
async def dispatch(self, request, call_next):
return await call_next(request)
class CustomHeaderMiddleware2(BaseHTTPMiddleware):
async def dispatch(self, request, call_next):
return await call_next(request)
all_middleware = [
Middleware(CustomHeaderMiddleware1),
Middleware(CustomHeaderMiddleware2),
]
app = Starlette(debug=True, routes=routes, middleware=all_middleware)with: uvicorn main:app --reload --port 8001And then I'm running: wrk -c 100 -d 1 http://0.0.0.0:8001/To be sure, I've added a counter, to see how many failures because of disconnection. I've run this against Starlette from today, 30 days ago, and 120 days ago (master branch wise). It looks like there's no regression, the issue is the same. We never stop having this issue, it was always here. I don't see anything on FastAPI that could have caused this issue to come into light more easily. If someone has some more insights, please feel free to share. |
Beta Was this translation helpful? Give feedback.
-
|
I don't know to be honest if it is a regression or not, but using a custom middleware doesn't yield but it does on this one: |
Beta Was this translation helpful? Give feedback.
-
|
That's the version we introduced It will be really appreciated if we can debug things here with pure Starlette. FastAPI pins Starlette, and that makes things hard here. |
Beta Was this translation helpful? Give feedback.
-
|
I understand - for me using |
Beta Was this translation helpful? Give feedback.
-
|
I've been tracking this issue on my application as well (also fastapi with custom middleware). I ran the script above as @bob-rove suggested, using starlette |
Beta Was this translation helpful? Give feedback.
-
Yeah, at some point that exception was obfuscated. |
Beta Was this translation helpful? Give feedback.
-
|
This error is raised when the stream is closed due to I managed to make this error reproducible in 0.14.2 by partially emulating 0.15.0 logic: acjh@37dd8ac I have submitted PR #1706 to shield send "http.response.start" from cancellation. |
Beta Was this translation helpful? Give feedback.
-
|
I saw that you have the separate implementation in #1710, but that there was a discussion about potentially changing the method to an exception. Have you thought more about which way you might go? And is there anything you need from those of us watching the discussion? Extra testing, etc? |
Beta Was this translation helpful? Give feedback.
-
|
This is fixed in #1715 |
Beta Was this translation helpful? Give feedback.
This is fixed in #1715