Skip to content
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

rewrite middleware to pure ASGI #18

Closed
dmig-alarstudios opened this issue Aug 17, 2020 · 5 comments · Fixed by #21
Closed

rewrite middleware to pure ASGI #18

dmig-alarstudios opened this issue Aug 17, 2020 · 5 comments · Fixed by #21
Labels
enhancement New feature or request

Comments

@dmig-alarstudios
Copy link

Since there are issues in Starlette caused by BaseHTTPMiddleware class, this package becomes a source of these issues in any project using it:

The simple solution would be to rewrite the middleware to pure ASGI.

@tomwojcik
Copy link
Owner

tomwojcik commented Aug 17, 2020

Hmm, good point. I never heard about this issue before and I'm not affected by this. I will definitely rewrite this to pure ASGI at some point as it seems like there are no downsides of the solution you suggest but I will give it some time until the creators of Starlette decide what to do next. From the tickets you linked I conclude that BaseHTTPMiddleware is just broken in some cases.

@tomwojcik tomwojcik added enhancement New feature or request help wanted Extra attention is needed labels Aug 17, 2020
@dmig-alarstudios
Copy link
Author

Not in some cases. It's broken by design.
Problems are: ReadTimeouts, if using BaseHTTPMiddleware-based middleware along with BackgroundTasks and huge memory usage because BaseHTTPMiddleware spools everything in memory.

@dmig-alarstudios
Copy link
Author

Here is a nice and simple example of ASGI middleware: https://github.com/encode/starlette/blob/master/starlette/middleware/sessions.py

@tomwojcik
Copy link
Owner

Yeah, I see. I will rewrite it over the weekend. Thanks for letting me know.

@tomwojcik tomwojcik linked a pull request Oct 9, 2020 that will close this issue
@tomwojcik
Copy link
Owner

@dmig-alarstudios 0.3.0 published with what should be a functioning "raw" middleware. Let me know if it doesn't work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants