-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: use pure ASGI middlewares #145
Conversation
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.
Very nice! 🙌
✅ Did a final smoke test with ER, with both sessions & read/writes 🚀 |
FWIW, when I started this lib, I initially wrote a fastapi dependency to handle the sqla session. But there was a bug/flaw in fastapi (fastapi/fastapi#869). Therefore, @vicrep and I went with a middleware instead. Today, that fastapi bug/flaw has been fixed: I strongly recommend ditching the middleware in favour of a simpler fastapi dependency. (FWIW, I wrote an async only fastapi sqla lib which illustrates my thought-> https://github.com/hadrien/fastapi-async-sqla ) |
Thanks for flagging, we'll look into this! |
Problem
When using multiple sessions, we've seen sporadic issues coming from the injected middlewares where the response body is missing, causing errors due to a mismatch with the promised
Content-Length
. We've also observed a significant increase in endpoint latency.Besides, the middleware class we're using,
BaseHTTPMiddleware
, is no longer the recommended way of writing middlewares for Starlette. It's known to have functional issues and does not scale well (see discussion on this FastAPI issue, discussion on this Starlette issue, and this performance benchmark).Solution
Refactor the middleware used to inject sessions into the request, to be pure ASGI middleware.
Related
Validation
✅ Tests are passing
✅ ER smoke tests with 2 sessions - calls are successful, both middlewares are present and executed (sample request - only the default session is used, sample request - both sessions are used)