starlette-context icon indicating copy to clipboard operation
starlette-context copied to clipboard

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

Open tomwojcik opened this issue 4 years ago • 8 comments

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 https://github.com/tomwojcik/starlette-context/issues/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 https://github.com/encode/starlette/issues/1012#issuecomment-866622798 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 avatar Jun 23 '21 20:06 tomwojcik

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?

hhamana avatar Jun 24 '21 08:06 hhamana

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

michaeltoohig avatar Mar 04 '22 02:03 michaeltoohig

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

adriangb avatar Jun 13 '22 14:06 adriangb

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

tomwojcik avatar Jun 13 '22 15:06 tomwojcik

@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

flipbit03 avatar Sep 08 '22 17:09 flipbit03

I think it's best to link to discussions already existing in Starlette: https://github.com/encode/starlette/discussions/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.

adriangb avatar Sep 08 '22 18:09 adriangb

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

Thanks for linking the related discussion.

tomwojcik avatar Sep 08 '22 19:09 tomwojcik

Thank you @adriangb for the context link!

flipbit03 avatar Sep 09 '22 15:09 flipbit03