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 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? |
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