Move to ASGI middleware implementation?
twcurrie opened this issue · 8 comments
There's been an uptick in issues (1, 2, 3, etc.) around Starlette's BaseHTTPMiddleware class, so much so that some are proposing to deprecate it.
Should we shift the implementation to be a completely ASGI-based implementation?
If help wanted, let me know.
While I don't have any strong opinion either way, the proposal you linked to seems to still be under heavy discussion (last comment there was after this ticket was opened).
Is it worth pausing for a moment to see which way they decide to go?
I don't really have much time to dig into this, but happy for others to contribute and accept a PR around this. I would just suggest trying to follow whatever the folks at starlette do, so that users of slowapi have a coherent set of changes to make when switching (if anything is needed).
There are generally no downsides to using pure ASGI middleware and there are some small upsides like better performance (BaseHTTPMiddleware creates queues, tasks, etc.).
Are you concerned about backwards compatibility? I think it would only be an issue if you had users subclassing SlowAPIMiddleware right?
Hi, for my particular use-case, I need the middleware to be a pure ASGI middleware, since I want to use background tasks as well. According to my tests, and starlette's documentation.
Currently, the BaseHTTPMiddleware has some known limitations:
- It's not possible to use BackgroundTasks with BaseHTTPMiddleware. Check #1438 for more details.
I'm currently writing it in my application and I'm almost done. It would be a pleasure to contribute to this project with my ASGI implementation of the SlowAPIMiddleware.
However, I just have a question before I could open a PR, would you prefer that I add a second middleware, keeping the current one untouched, or should I replace it with mine ? (cc @laurentS )
It's possible to use BackgroundTasks with BaseHTTPMiddleware on Starlette 0.21.0.
PR that removes that note from the docs: encode/starlette#1874.
IMHO, shifting from the BaseHTTPMiddleware to pure ASGI would be a major revision and would need more review before merge. I think contributing it as a second middleware would be the easiest path forward at this point.
@Kludex good catch, anyway I'm using fastAPI which have a strict dependency on 0.20.4 for now 😄
Anyway, I may open a PR in the coming days with an alternative middleware making sure the current BaseHTTPMiddleware stays the same while giving the ability to use a pure ASGI one instead.
For those interested, here is the associated PR for the ASGI middleware I wrote: #113
Don't hesitate to comment and review it if you feel so 😄