-
-
Notifications
You must be signed in to change notification settings - Fork 874
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: pathsend handling in middlewares #2616
base: master
Are you sure you want to change the base?
Conversation
d7b0358
to
6916ede
Compare
6916ede
to
a6014c6
Compare
async with recv_stream: | ||
async for message in recv_stream: | ||
if message["type"] == "http.response.pathsend": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
271c3b5
to
2107b5e
Compare
2107b5e
to
5b15ea9
Compare
5b15ea9
to
4f01bf6
Compare
@Kludex any updates on this? |
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