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

BaseHTTPMiddleware vs BackgroundTasks #20

Closed
Kludex opened this issue Dec 4, 2020 · 9 comments
Closed

BaseHTTPMiddleware vs BackgroundTasks #20

Kludex opened this issue Dec 4, 2020 · 9 comments

Comments

@Kludex
Copy link
Contributor

Kludex commented Dec 4, 2020

@trallnag I've just noticed that we use @app.middleware('http') here, I should have been able to catch this earlier... Anyway, that decorator is implemented on top of BaseHTTPMiddleware, which has a problem: encode/starlette#919

Solution: change the implementation to a pure ASGI app/middleware.

PS.: I can open a PR with it, jfyk.

@trallnag
Copy link
Owner

trallnag commented Dec 4, 2020

Async Python is a complete black box for me I have zero experience with so far (except the little bit I interact with it in FastAPI). So if you know a way to refactor the code, please go on. Would be awesome

@Kludex
Copy link
Contributor Author

Kludex commented Dec 4, 2020

I've found a very insightful article yesterday: https://florimond.dev/blog/articles/2019/08/introduction-to-asgi-async-python-web/

But anyway, thank you. I'll be working on it then (it should not be many changes, only focused on the middleware interface).

@trallnag trallnag linked a pull request Feb 14, 2021 that will close this issue
@pawamoy
Copy link

pawamoy commented Apr 6, 2022

Took me a few hours today to get to this issue 😄
Glad to see it has already been worked on, thanks @Kludex and @trallnag 🙂
Any way I can help moving this forward?

@pawamoy
Copy link

pawamoy commented Apr 6, 2022

Oh, I see @Kludex is also working on resolving the original issue in Starlette.

@Kludex
Copy link
Contributor Author

Kludex commented Apr 6, 2022

Any way I can help moving this forward?

Yes, you can take my PR: #23 .

Oh, I see @Kludex is also working on resolving the original issue in Starlette.

🏃 💨

@pawamoy
Copy link

pawamoy commented Apr 6, 2022

Should I understand the Starlette PR is doomed 😅 ?
I'll see if I can continue your work once @trallnag has given us updates (on this PR or on the CI one) 🙂

@Kludex
Copy link
Contributor Author

Kludex commented Apr 6, 2022

Should I understand the Starlette PR is doomed 😅 ?

I'm bothering ppl to review it... 😅

@tonkolviktor
Copy link

Hi all,
would it be possible to get a status update here?
Does the close on this PR means this will never be implemented? encode/starlette#919 or it's fixed via: encode/starlette#1715?

Does anybody have/tested a workaround, like: encode/starlette#919 (comment) ?

thanks

@Kludex
Copy link
Contributor Author

Kludex commented Oct 10, 2022

This was already fixed here.

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

Successfully merging a pull request may close this issue.

4 participants