fix: pathsend handling in middlewares #2616
Conversation
gi0baro
force-pushed
the
2613-fix-pathsend-middlewares
branch
3 times, most recently
from
d7b0358 to
6916ede
Compare
gi0baro
force-pushed
the
2613-fix-pathsend-middlewares
branch
from
6916ede to
a6014c6
Compare
| async def body_stream() -> BodyStreamGenerator: | ||
| async with recv_stream: | ||
| async for message in recv_stream: | ||
| if message["type"] == "http.response.pathsend": |
There was a problem hiding this comment.
Hi there! wouldn't it make sense to just read the file and stream it through here? Would be better for compatibility, no? Or maybe have some flag like "supports_message_pass_through"?
There was a problem hiding this comment.
Reading the file from Python would be just the standard ASGI behaviour and defeat the whole pathsend idea (see for ref https://asgi.readthedocs.io/en/latest/extensions.html#path-send).
About the flag: I'm not sure I get the point around it. How should we evaluate such flag? pathsend messages will be returned by Starlette only if the relevant extension is present in the ASGI scope as the server supports it.
There was a problem hiding this comment.
I was thinking about random middlewares out there, deriving from this BaseMiddleware that do not know pathsend. To keep those running, returning the message instead of bytes could be opt-in via a flag/class property
There was a problem hiding this comment.
@aersam but even if you subclass BaseMiddleware, there's no way you can interact with that code. If the middleware completely overrides __call__ then it should be compliant with ASGI messages IMHO.
gi0baro
force-pushed
the
2613-fix-pathsend-middlewares
branch
2 times, most recently
from
271c3b5 to
2107b5e
Compare
gi0baro
changed the title
gi0baro
force-pushed
the
2613-fix-pathsend-middlewares
branch
from
2107b5e to
5b15ea9
Compare
gi0baro
force-pushed
the
2613-fix-pathsend-middlewares
branch
from
5b15ea9 to
4f01bf6
Compare
|
@Kludex any updates on this? |
There was a problem hiding this comment.
Are the changes here just because the StreamingResponse is used in the BaseHTTPMiddleware?
There was a problem hiding this comment.
Yes. A Better implementation would probably be to refactor BaseHTTPMiddleware, but it is something I am not confident to do by myself given my knowledge of the codebase is still limited.
There was a problem hiding this comment.
We are not making changes to the rest of the code source to make BaseHTTPMiddleware happy.
My recommendation for Granian users, and/or any server supporting http.response.pathsend is to replace BaseHTTPMiddleware for pure ASGI middleware. I don't want us to add more complexity to that code. That piece of code had too many issues over time.
There was a problem hiding this comment.
I understand your position.
I think there are 2 major themes here though:
StreamingResponsedoesn't support any message type other thanhttp.response.body, which make sense given the intent of streaming responses, so unless an additional body message type is added to ASGI this is future-proof. But, sinceBaseHTTPMiddlewaredepends on that, this also means any application using that middleware or a derivate middleware won't support any other ASGI message in responses, which is something you might want to change to be future-proof on ASGI spec changes.- As of today Starlette states (due to the changes I made in Add support for ASGI
pathsendextension #2435) to supporthttp.response.pathsend, which is only partially true: as soon as the route involves aBaseHTTPMiddleware,GZIPMiddlewareor a derivate of those in the middleware chain, then Starlette doesn't actually support pathsend.
The rationale behind this PR is not to address the first point, but rather the 2nd; as today experience with pathsend supportive servers with any Starlette/FastAPI application is practically dependant on which middlewares are implemented on the route returning a file response.
So given all of the above, if we can't find a proper way to address the pathsend issue, I'd rather suggest to revert #2435 and remove pathsend support completely until we find a proper way to support that ASGI extension in all the usages of Starlette and its dependants. This would sacrifice the performance gain of pathsend but at least we'd have the same DX across all the Starlette apps, regardless of the middlewares used, and thus prevent the whole bunch of issues related to this opened in Granian repo.
There was a problem hiding this comment.
Yeah, we can revert that one. It will be easier.
There was a problem hiding this comment.
👍 I'll open a different PR later today and close this one
|
Closing this in favour of #2649, as discussed. |
3 participants
Summary
Closes #2613
It seems that when I introduced support for pathsend in #2435 I didn't check the included middlewares to also support the relevant ASGI messages. Affected Starlette versions 0.36.0 and onwards when running on a server supporting pathsend.
Note: this is still in draft since I'd like to plan some tests for the involved code; also the proposed solution isn't necessarily the best one, probably someone like @Kludex might review this and give his feedbacks before proceeding further.Update: I added tests just to verify the code. In case you're unhappy with the implementation, we can refactor that and keep the tests.
Checklist