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.

Sign up for GitHub

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

Document BaseHTTPMiddleware bugs #1640

Merged
merged 4 commits into from
May 22, 2022
Merged

Document BaseHTTPMiddleware bugs #1640

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
184de53
Document `BaseHTTPMiddleware` bugs
Kludex May 18, 2022
35138da
Apply suggestions from code review
Kludex May 19, 2022
237421e
Merge branch 'master' into document-basehttpmiddleware-bug
Kludex May 19, 2022
8ec35ca
Merge branch 'master' into document-basehttpmiddleware-bug
Kludex May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion docs/middleware.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,6 @@ class CustomHeaderMiddleware(BaseHTTPMiddleware):
return response



middleware = [
Middleware(CustomHeaderMiddleware, header_value='Customized')
]
Expand All @@ -227,6 +226,12 @@ Middleware classes should not modify their state outside of the `__init__` metho
Instead you should keep any state local to the `dispatch` method, or pass it
around explicitly, rather than mutating the middleware instance.

!!! bug
Currently, the `BaseHTTPMiddleware` has two known issues:
Kludex marked this conversation as resolved.
Show resolved Hide resolved

- It's not possible to use multiple `BaseHTTPMiddleware` based middlewares.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is true... I'll need to come back to this.

This BaseHTTPMiddleware is a real challenge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it hurts a lot of people to say that, but I don't think a web framework suitable for large projects should use too low abstraction... I like the design of Starlette so much that I even implemented a similar one for WSGI thing.

But I continue to practice, observe the starlette/fastapi community feedback and communicate with colleagues in the company. Maybe we should use only Request and Response objects inside the framework like Django or flask.

We can see that many of the problems stem from Starlette's direct use of ASGI internally. Like this issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I get what you say exactly. But if I do, you're implying on doing a major refactor on the code source?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no intention of doing a major refactoring of starlette, it's too much of an impact. But maybe we should change our positioning to be more of a toolkit than a framework.

Then transfer part of the work to fastapi.

For example, reimplement a Django-like Application in fastapi, so that only Request and Response are used in the entire framework.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this way we can not only solve the BaseHTTPMiddleware problem, but also improve the other problems like #944.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to fix BaseHTTPMiddleware for the past three or four days, and have looked at tons of other web frameworks. The results have been negative. 😞

Perhaps the above point is cowardly, but we can really think about the overall structure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to fix BaseHTTPMiddleware for a long time as well. No need to feel ashamed about not succeeding... 😅 And I'll probably try again next weekend... 😞

But maybe we should change our positioning to be more of a toolkit than a framework.

You mean like https://github.com/klen/asgi-tools , right? I don't agree with that change of mindset, but you can create a discussion to see how others feel about it, if you want.

Copy link
Contributor

@erewok erewok May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think the problems with BaseHTTPMiddleware are terminal: it's not fixable.

I also disagree about what the problem is: I don't think Starlette is offering too many low-level tools but not encouraging developers to use them enough. For example of this, the other middleware shipped by Starlette works fine, and those all take the scope, receive, send args. Here, Starlette's layers-of-an-onion design where everything is an ASGI app that wraps another ASGI app (until you get to the center where a Request object is created) is both simple and makes a lot of sense in my opinion: even after all this time I can still hold almost the whole Starlette codebase in my head. If other libraries (such as the starlette-prometheus one I linked above) wrote their Middleware using this style, they'd probably be fine. We've written lots of our own Middleware using these patterns at work and our background tasks do run in the background: we don't have this issue.

I actually think we run into problems with the leaky abstractions in Async programming in Python in general when we start to work with the following:

  • Streaming response bodies
  • Background tasks

My suspicion is that to fix these problems you would likely have to spin up channels and separate the processing of requests -> responses, requests -> streaming responses, and background. This is probably where the refactoring effort should be placed (if there is to be one). A Starlette app should spin up a dedicated channel for background tasks and sending responses and then the Response class should route things to the proper channel.

The real problem in my opinion is ever thinking about a "complete" Request, Response, or Background when everything should be treated as streaming by. You can only map or filter over these things or send work to other channels. (My last preference here would be to leverage the kinds of tools that trio provides, so you get much improved channels, nested scoping, and cancellation over standard library async, but that would probably be a disruptive change for Starlette users.)

Again, I offer my opinions humbly as someone who has a lot of respect for the Starlette codebase, someone who sees this project as a very nice implementation of the Python ASGI spec and someone who's built a couple dozen projects at work using it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also believe it's not fixable / not worth fixing and it was the wrong abstraction to begin with.

Something I'm wondering is: can we track down the packages that are using BaseHTTPMiddleware and make PRs to them to switch them over? That might have an even bigger impact on the community than docs and if we focus on just popular projects it wouldn't take that much time (I think).

separate the processing of requests -> responses, requests -> streaming responses, and background

I do think that we should be spinning up a lifespan scoped anyio.TaskGroup and pushing background tasks into there instead of processing them after sending the response but before returning from our ASGI coroutine.

But maybe we should change our positioning to be more of a toolkit than a framework
Then transfer part of the work to fastapi.

I do think there's a lot of things that Starlette offers right now that it could do without / could be transferred upstream or to other libraries. Off the top of my head:

  • Config management
  • Schema / OpenAPI stuff
  • Authentication

- It's not possible to use `BackgroundTasks` with `BaseHTTPMiddleware`.

Kludex marked this conversation as resolved.
Show resolved Hide resolved
## Using middleware in other frameworks

To wrap ASGI middleware around other ASGI applications, you should use the
Expand Down