-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: pathsend handling in middlewares #2616
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are the changes here just because the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. A Better implementation would probably be to refactor
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not making changes to the rest of the code source to make My recommendation for Granian users, and/or any server supporting
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand your position. I think there are 2 major themes here though:
The rationale behind this PR is not to address the first point, but rather the 2nd; as today experience with 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
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we can revert that one. It will be easier.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I'll open a different PR later today and close this one |
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
pathsendidea (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?
pathsendmessages 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.