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

Consider removing RawContextMiddleware and keeping only ContextMiddleware after resolving memory usage issue #47

Open
tomwojcik opened this issue Jun 23, 2021 · 8 comments
Labels
question Further information is requested

Comments

@tomwojcik
Copy link
Owner

I like ContextMiddleware because it's very simple to understand and expand, even for minds that are not familiar with async Python.

Some time ago it was advised #18 to add RawContextMiddleware and Starlette maintainers were discouraging the use of the built-in middleware. I was surprised to see 3 ❤️ reactions under this issue so I think a few people had problems with it.

Fast-forward almost a year, the issue encode/starlette#1012 (comment) with memory usage has been closed.

If there's no point in keeping both, I'd be happy to remove the more complicated one but I will wait for some confirmations from the community. If there's a case when it makes sense to use RawContextMiddleware, then I'd keep both.

What's your opinion about that? @hhamana @dmig-alarstudios

@tomwojcik tomwojcik added the question Further information is requested label Jun 23, 2021
@hhamana
Copy link
Collaborator

hhamana commented Jun 24, 2021

I haven't seen much difference with either middleware. If the original concern is now fixed, the regular ContextMiddleware is indeed much easier to work with, so supporting only this one would surely simplify things.
But both are working fine so far, so I don't see a pressing reason to remove it either.
Maybe when there are some differences that can't be abstracted nicely?

@michaeltoohig
Copy link

If the original concern is fixed then the docs need to be updated. I ended up in this issue while trying to figure out what was wrong with ContextMiddleware from reading the warning in the docs here

@adriangb
Copy link

adriangb commented Jun 13, 2022

Hi I am a Starlette maintainer. I highly encourage you to not use use BaseHTTPMiddleware. I would stick with RawContextMiddleware and deprecate ContextMiddleware. cc @Klundex

@tomwojcik
Copy link
Owner Author

Great! I was hoping one of the Starlette maintainers would chime in and help make this decision. Thanks.

@flipbit03
Copy link

@adriangb can you clarify why would it be encouraged to keep using RawContextMiddleware over the other? What are the other's shortcomings? Can we simply phase out the other one, if it's a case of being defective? Thanks in advance

@adriangb
Copy link

adriangb commented Sep 8, 2022

I think it's best to link to discussions already existing in Starlette: encode/starlette#1729. There are really no benefits to starlette-context's users for having both middlewares, there is only potential downsides that they will run into bugs related to BaseHTTPMiddleware.

@tomwojcik
Copy link
Owner Author

tomwojcik commented Sep 8, 2022

In the next minor release, we're getting rid of it. The deprecation warning is already there.

Thanks for linking the related discussion.

@flipbit03
Copy link

Thank you @adriangb for the context link!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants