-
-
Notifications
You must be signed in to change notification settings - Fork 920
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
RuntimeError("No response returned")
in BaseHTTPMiddleware
#2516
Comments
versions: python=3.11
starlette=0.37.1
uvicorn=0.27.1 @jonathanslenders this also happens for me, and seems to also be due to client disconnects. I have modified your example a bit, and interestingly in my example it consistently works when you have 3x middleware, and breaks when you have +4x middlewares. import asyncio
from starlette.applications import Starlette
from starlette.middleware import Middleware
from starlette.middleware.base import BaseHTTPMiddleware
from starlette.requests import Request
from starlette.responses import Response
from starlette.routing import Route
class DummyMiddleware(BaseHTTPMiddleware):
def __init__(self, **kwargs) -> None:
self._version = kwargs.pop("version")
super().__init__(**kwargs)
async def dispatch(self, request: Request, call_next) -> Response:
print(self._version, id(self), "STARTED")
try:
res = await call_next(request)
print(self._version, id(self), "COMPLETED")
return res
except Exception as exc:
print(self._version, id(self), "FAILED", exc)
return Response(b"")
async def sleepy(request: Request):
sleep_ms = int(request.query_params.get("sleep", 200))
await asyncio.sleep(sleep_ms / 1000)
return Response(b"")
app = Starlette(
routes=[Route("/", sleepy)],
middleware=[Middleware(DummyMiddleware, version=_ + 1) for _ in range(4)],
) Serve the app with Uvicorn uvicorn app:app --reload Make a CURL request where you can modify the curl http://127.0.0.1:8000?sleep=250 -m 0.2 OUTPUT 1 4392769744 STARTED
2 4392769616 STARTED
3 4392769232 STARTED
4 4392768208 STARTED
4 4392768208 COMPLETED
3 4392769232 COMPLETED
2 4392769616 COMPLETED
1 4392769744 FAILED No response returned. Changing number of middlewares to 3 1 4368290960 STARTED
2 4368290768 STARTED
3 4368290576 STARTED
3 4368290576 COMPLETED
2 4368290768 COMPLETED
1 4368290960 COMPLETED
After some further debugging, this seems to appear exactly from starlette>=0.28.0 and seems to could be in relation to introduction of the Not surprisingly this change in Starlette would not cause the issue anymore, though I don't imagine this is actually the solution: - message = await wrap(wrapped_receive)
+ message = await wrap(receive) @Kludex would you have any clue why it happens at >=4 middlewares but not <=3? Or does it just seem to be a coincidence that this behaviour happens? |
Hi, seems like we encountered a similar issue here. We are actually using fastapi and at first we suspect that we are facing fastapi/fastapi#8187. However we noticed that the hang can only be reproduced when more than two people keep refreshing the page together. So, that looks more like a race condition, i.e. probably not related to fastapi/fastapi#8187, which is a stream double-consume issue and hence should be reproducible by a single user's refresh, unconditionally, which is not our case. Our starlette version is 0.36, and when two users are refreshing the page at the speed of ~one time per 2s, we can repro the hang issue in ~30 secs. |
👋🏻 We think we're seeing this too, with 5 middlewares, on:
|
We were seeing this as well on py3.12, dep versions:
I'm downgrading us to:
Which seems like it'll resolve the issue, but I'll report back after letting it run for a week. Unfortunately, we can't bump versions of |
I would recommend you all try using pure ASGI middleware instead of BaseHTTPMiddleware. I'm confident it will solve the issues you're seeing. |
On our side at least, we're not explicitly choosing to use Rather, Maybe this is a personal weakness, but I feel more comfortable downgrading to a known-good version of these dependencies and waiting for the issue to be fixed, than I do subclassing Regardless, thank you! It's good to know that's a possible recourse if we want to upgrade |
You don't have to subclass FastAPI. Just follow the Starlette docs here or use FastAPI dependencies instead. |
Hey starlette team! First of all thanks for the great ASGI framework you've created. I wanted to chime in the conversation as we're seeing these issues as well by using >=4 middlewares as @mikkelduif mentioned. I was digging a little bit inside the code and seems to be an issue with the starlette/starlette/middleware/base.py Line 33 in 5a1bec3
however we consume starlette/starlette/middleware/base.py Lines 83 to 84 in 5a1bec3
however as @adriangb suggested we will move to using pure ASGI middlewares and remove all of our |
We're seeing the issue as well, forcing us to use fewer middlewares. Please fix. |
Hi folks please try #2620 and see if it fixes your issue. I do still recommend you avoid |
I checked out #2620, and it solved it! Thanks so much! |
Fix and enable again starlette tests that were disabled. Reason of the bug: - starlette was fixed in 0.38.3 for an issue that was causing trouble in our appsec instrumentation. Tests were upgraded accordingly Fix: encode/starlette#2620 Issue: encode/starlette#2516 Also improve the guards in the asgi middleware appsec instrumentation for request body to ensure that we never break. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
So, how do you check now for a streaming response returned from BaseHttpMiddleware without importing private |
You shouldn't be doing that. What's your use case? |
Before I was able to check what returned from call_next as Streaming response
Now I am unable to check that without importing What would you suggest? |
Well, I suggest you to not to have that check... But I want to understand what is the use case. What were you doing that needed to check the response class? Also... Was it possible to reach the |
encode#2620) * Don't poll for disconnects in BaseHTTPMiddleware via StreamingResponse Fixes encode#2516 * add test * fmt * Update tests/middleware/test_base.py Co-authored-by: Mikkel Duif <[email protected]> * add test for line now missing coverage * more coverage, fix test * add comment * fmt * tweak test * fix * fix coverage * relint --------- Co-authored-by: Mikkel Duif <[email protected]>
Previously, a "No response returned"-error has been discussed in this discussion: #1527, and was supposedly fixed in this PR: #1715
However, the bug is still present. It's a race condition, so quite tough to reproduce, but the following reproducer works.
Reproducer
Use anyio 4.2.0 It does happen also with anyio 3.0.0, but it happens much more regularly on 4.2.
First, apply the following patch to the Starlette codebase, so that we have a breakpoint in case it happens:
Then run this code:
Open either Firefox or Chrome, navigate to http://localhost:8000 and hold down command-r (refresh) to repeatedly have the browser refresh the page. After a couple of seconds, we'll enter the breakpoint.
(Feel free to turn this issue into a discussion if that's more appropriate. I'm also looking for a fix but have a hard time understanding
BaseHTTPMiddleware
.)edit: I updated the above snippet and added the missing imports.
This PR contains a possible fix: #2519
Important
The text was updated successfully, but these errors were encountered: