Deprecating BaseHTTPMiddleware

[adriangb]

Based on many previous discussions, I am proposing that we deprecate BaseHTTPMiddleware.

We'll put a deprecation warning in the constructor. In this warning we will give:

• Some background on the issue
• A date at which we will remove BaseHTTPMiddleware
• Options for developers (basically help us fix it or migrate to ASGI middleware).

This will be a part of a multipronged approach consisting of:

• Better documentation. We've already documented the limitations and issues with BaseHTTPMiddleware both in our docs and tests. Document how to create ASGI middlewares #1656 will give us better documentation on creating ASGI middleware. This is now done!
• I (and I think @Kludex as well) are prepared to personally contribute to downstream packages by porting their middlewares to pure ASGI middleware. We'll probably just look for any packages with > X stars or something and make PRs. We've proposed (and merged some) upstream fixes into some of the most popular libraries using BaseHTTPMiddleware we could find on GitHub.
• Providing an alternative implementation. Experiment a high-level HTTPMiddleware #1691 proposes an alternative safer implementation of a simple request/response middleware. I'm not sure this is necessary now that we have better ASGI middleware docs, but it's an option.
• Deprecating and eventually removing BaseHTTPMiddleware

[florimondmanca]

This is a rather bold move to make given the usage of BaseHTTPMiddleware in the wild, so let’s pause and think it through. I’m sure there’s been discussion before (are there any links available?), but jotting down some thoughts and questions…

In general, what’s the migration path going to be like for users of BaseHTTPMiddleware? Should we start by exploring how FastAPI could do without? It’s still relying on @app.middleware extensively. If the hundreds of thousands (maybe?) of FastAPI users get this deprecation warning, what action should they take to keep their systems running? Certainly the action might be on some intermediate dependency maintenance team. Does this mean we should first help folks migrate, then deprecate?

What does addressing the two problems mentioned in the docs (context vars, background tasks) look like with pure ASGI middleware?

What do existing examples in the BaseHTTPMiddleware docs look like using pure ASGI middleware?

While I reckon the limitations of BaseHTTPMiddleware, and the fact that currently users expect certain things to work that don’t, I also fear « pure ASGI middleware all the things » might be really off putting a lot of folks who’d like to do very simple edits to requests or responses.

[adriangb]

This is a rather bold move to make given the usage of BaseHTTPMiddleware in the wild, so let’s pause and think it through.

I agree. I realize this is pretty aggressive proposal and recognize that it's a long shot for it to be accepted, but I hope that it can at least lead to some good discussion.

I’m sure there’s been discussion before (are there any links available?), but jotting down some thoughts and questions…

There are lots of general issues related to BaseHTTPMiddleware, the last/clearest I can think of is #1640 (comment).

In general, what’s the migration path going to be like for users of BaseHTTPMiddleware?
Should we start by exploring how FastAPI could do without? It’s still relying on @app.middleware extensively. If the hundreds of thousands (maybe?) of FastAPI users get this deprecation warning, what action should they take to keep their systems running? Certainly the action might be on some intermediate dependency maintenance team. Does this mean we should first help folks migrate, then deprecate?

I think there are several ways to tackle this:

• Better documentation on writing pure ASGI middleware (Document how to create ASGI middlewares #1656
• Contributing downstream to prominent libraries using BaseHTTPMiddleware (Use the right type on Response for tests trallnag/prometheus-fastapi-instrumentator#139, Replace BaseHTTPMiddleware with pure ASGI middleware python-discord/forms-backend#173, will need more)
• Responding to the inevitable inflow of issues along the lines of "how do I convert this middleware to pure ASGI"
• Maybe providing some sort of simple middleware that does not suffer from the pitfalls of BaseHTTPMiddleware. I've been playing around with an idea here, but maybe other options like a "hook" style API for pure ASGI middleware.

What does addressing the two problems mentioned in the docs (context vars, background tasks) look like with pure ASGI middleware?

Basically: nothing. These problems simple don't exist with pure ASGI middleware.

What do existing examples in the BaseHTTPMiddleware docs look like using pure ASGI middleware?

+1 on this. I think we should replace any examples not specific to BaseHTTPMiddleware with pure ASGI middleware, independently of the outcome of this proposal.

While I reckon the limitations of BaseHTTPMiddleware, and the fact that currently users expect certain things to work that don’t, I also fear « pure ASGI middleware all the things » might be really off putting a lot of folks who’d like to do very simple edits to requests or responses.

I've found that often a pure ASGI middleware isn't necessarily more LOC or that ugly once you know how to write it (I admit there is a learning curve). Things that are ugly with pure ASGI middleware (like intercepting the request or response body byte stream) tend to be impossible or buggy with BaseHTTPMiddleware. Pure ASGI also has the huge ecosystem benefit of not being coupled to Starlette. So maybe pure ASGI all the things isn't that bad.

On the other hand, mentioned above, I think we maybe can come up with a different API for these "simple" middlewares that doesn't suffer from the pitfalls of BaseHTTPMiddleware (single example).

[adriangb]

What do existing examples in the BaseHTTPMiddleware docs look like using pure ASGI middleware?

I just looked and we don't have any in our docs, but FastAPI has several. @tiangolo how would you feel about reworking those examples? I'm happy to help.

[florimondmanca]

Now, if we consider something like #1691, I think a migration path could be:

1. Issue a release with the new generator-based API, and perhaps (best if we want to be able to drop BaseHTTPMiddleware eventually?) a deprecation warning hinting users to switch to that or pure ASGI middleware. Experiment a high-level HTTPMiddleware #1691 makes it so that the transformation from BaseHTTPMiddleware style to HTTPMiddleware style is fairly straightforward -- basically (request, call_next) -> (conn) and response = await call_next(request) -> response = yield. If this falls on intermediate libraries, end-users can always -W ignore this particular deprecation for a while.
2. When Starlette 1.0 lands, drop BaseHTTPMiddleware. (I think this criteria is better than committing to a specific date, even in the form of "First minor bump of 2023". If folks want to prepare, they can't know what minor that's going to be.)

?

[kissgyorgy]

Hello!

We at @onekey-sec have a pretty good understanding of the problems with BaseHTTPMiddleware. I try to explain them line by line, @vlaci or @kukovecz can chime in if I forgot something:

Problems

Synchronization issues in previous version

The previous version had synchrionization issues, for example Exceptions were thrown sooner than the response sent out, so testing middleware was impossible, but by implementing lock stepping with anyio.create_memory_object_stream() solved that problem. 👍🏻

The current implementation has the following problems:

Every coroutine got cancelled

after the line await self.app(scope, receive, send) in every other middleware that comes after a BaseHTTPMiddleware in the stack, because of the task_group cancellation in line 74:

async with anyio.create_task_group() as task_group:
    ...
    task_group.cancel_scope.cancel()

This is why Background tasks doesn't work, they simply got cancelled before the task_group async context manager exits.
It also means you can't use an async context manager in any of the later middlewares, because it's coroutines in exit will also be cancelled.

Here is another concreate example which doesn't work:

from sqlalchemy.async import create_async_engine

class DatabaseMiddleware:
    def __init__(self, app):
        self.app = app

    async def __call__(self, scope: Scope, receive: Receive, send: Send):
        db_engine = create_async_engine(...)
        async with db_engine.begin() as conn:  # __aexit__ will be cancelled, the database connection will not be closed properly
            scope["state"]["conn"] = conn
            await self.app(scope, receive, send)

middlewares = [
     Middleware(SomeBaseHTTPMiddleware, ...),
     Middleware(DatabaseMiddleware),
]
starlette = Starlette(..., middlewares=middlewares)

We figured cancellation might have been put there in case of client disconnections? but it has unfortunate side effects for the whole middleware stack.

Exceptions are swallowed

after the line await self.app(scope, receive, send) in every other middleware all Exceptions are swallowed because of a race condition in line 45-46 and line 61-62. There is no guarantee coro will not be finished before body_stream, so app_exc will still be None by the time body_stream get to line 61.

Here is a very surprising example of an Exception you will never see:

class NameErrorMiddleware:
    def __init__(self, app):
        self.app = app

    async def __call__(self, scope: Scope, receive: Receive, send: Send):
        await self.app(scope, receive, send)
        this_variable_doesnt_exist  # will raise NameError, but you will never see it

middlewares = [
     Middleware(SomeBaseHTTPMiddleware, ...),
     Middleware(NameErrorMiddleware),
]
starlette = Starlette(..., middlewares=middlewares)

Possible solutions

Avoiding lock stepping/synchronization issues

Lock stepping with send_stream, recv_stream = anyio.create_memory_object_stream() was a good idea, middlewares are not something they need to run "in parallel", so running the whole stack step-by-step (ASGI message-by-message) in tandem and stopping the chain when something goes wrong was a good idea.

Avoiding cancellation

A middleware should not cancel everything underneath it. Background tasks are a good example, so the task_group cancellation should be done differently or not done at all?

Providing lower-level tools

for helping parsing HTTP structures like Headers from ASGI messages can be pretty simple if there are good tooling in the framework. We needed to read some incoming Request headers and modify the response headers, and with the already existing APIs, we could completely get rid of BaseHTTPMiddleware by doing something like this:

from starlette.datastructures import Headers, MutableHeaders
from starlette.responses import Response

class RequestLoggingMiddleware:
    def __init__(self, app: ASGIApp):
        self.app = app

    async def __call__(self, scope: Scope, receive: Receive, send: Send):
        if scope["type"] != "http":
            await self.app(scope, receive, send)
            return

        request_headers = Headers(scope=scope)
        try:
            request_id = request_headers["X-Request-Id"]
        except KeyError:
            message = "Missing X-Request-Id header"
            logger.exception(message)
            error_response = Response(message, status_code=status.HTTP_400_BAD_REQUEST)
            await error_response(scope, receive, send)
            return

        logger.debug("Request received")

        def send_wrapper(message: Message):
            if message["type"] == "http.response.start":
                # This modifies the "message" Dict in place, which is used by the "send" function below
                response_headers = MutableHeaders(scope=message)
                response_headers["X-Request-Id"] = request_id
            return send(message)

        await self.app(scope, receive, send_wrapper)
        logger.debug("Request finished")

Similarly, some helpers for reading and writing the streams in a sychronized manner should also help if someone want's to intercept the HTTP body stream, something like the current body_stream when implemented with no race conditions 😃

Better documentation

About the documentation, I completely agree with you, some good examples for pure ASGI-middleware, maybe with the lower-level helpers would be better than a complex middleware like this.
SQLAlchemy has this recipes section and generally a ton of examples in their documentation, which I like a lot.

Hope this helps!

[adriangb]

Thank you for taking the time to dig into the codebase!

It also means you can't use an async context manager in any of the later middlewares, because it's coroutines in exit will also be cancelled.

Exceptions are swallowed

These are interesting edge cases that I don't think I'd seen before. I think we should review them more and if they check out add them to the Limitations section in the docs and/or add a test case documenting the behavior.

Avoiding lock stepping/synchronization issues

I didn't understand what the proposed solution / problem is in this section

Avoiding cancellation

I think we all agree on this, but the questions is how can we avoid cancellation without braking anything? #1715 may be a viable proposal

Providing lower-level tools

These already exist. Is the suggestion to put this into the docs (see below)?

Better documentation

We're working on it! We recently merged #1656 and #1723 is building upon that and adding even more examples (including ones using Request as a helper to parse headers and such).

[jamiecha]

FYI, by converting to pure ASGI style, I observed 50% of speed improvements compared to the previous one which was inheriting from BaseHTTPMiddleware.
What's being done inside middleware is exactly same.
I am sharing my case because I didn't expect any speed improvement.

• BaseHTTPMiddleware version

❯ wrk --latency -t8 -c50 -d10s http://127.0.0.1:7088/healthcheck
Running 10s test @ http://127.0.0.1:7088/healthcheck
  8 threads and 50 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    40.31ms    8.99ms  74.11ms   92.22%
    Req/Sec   149.17     33.28   181.00     96.62%
  Latency Distribution
     50%   37.80ms
     75%   38.60ms
     90%   40.43ms
     99%   73.24ms
  11904 requests in 10.02s, 2.05MB read
Requests/sec:   1188.48
Transfer/sec:    210.07KB

• pure ASGI version

❯ wrk --latency -t8 -c50 -d10s http://127.0.0.1:7088/healthcheck
Running 10s test @ http://127.0.0.1:7088/healthcheck
  8 threads and 50 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    26.59ms    6.40ms  42.11ms   66.21%
    Req/Sec   226.04     44.63   420.00     58.00%
  Latency Distribution
     50%   28.75ms
     75%   30.68ms
     90%   32.86ms
     99%   37.32ms
  18024 requests in 10.01s, 3.11MB read
Requests/sec:   1800.52
Transfer/sec:    318.26KB

[adriangb]

Nice! A 50% performance improvement is a lot more than I was expecting, but I would expect a small (probably unmeasurable unless you're piling on dozens of BaseHTTPMiddlewares on a toy app) performance bump.

[jamiecha]

Nice! A 50% performance improvement is a lot more than I was expecting, but I would expect a small (probably unmeasurable unless you're piling on dozens of BaseHTTPMiddlewares on a toy app) performance bump.

Please find the whole code (simplified version of my production code)
Pure ASGI version is more than 2x faster on my machine (x86_64, starlette 0.19.1, fastapi 0.80.0)

• Commands

  • server: uvicorn main:app
  • bench: wrk --latency -t8 -c50 -d10s http://127.0.0.1:8000/healthcheck

• BaseHTTPMiddleware

from fastapi import FastAPI
from starlette.requests import Request

app = FastAPI()

@app.get("/healthcheck")
async def root():
    return {"message": "Hello World"}

@app.middleware("http")
async def request_logging_middleware(request: Request, call_next):
    response = await call_next(request)
    return response

• Wrk result

❯ wrk --latency -t8 -c50 -d10s http://127.0.0.1:8000/healthcheck
Running 10s test @ http://127.0.0.1:8000/healthcheck
  8 threads and 50 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    22.65ms    3.97ms  51.02ms   91.29%
    Req/Sec   265.66     30.51   303.00     59.38%
  Latency Distribution
     50%   21.71ms
     75%   22.69ms
     90%   24.55ms
     99%   40.03ms
  21183 requests in 10.02s, 3.03MB read
Requests/sec:   2114.39
Transfer/sec:    309.87KB

• Pure ASGI

from fastapi import FastAPI
from starlette.middleware import Middleware
from starlette.types import ASGIApp, Scope, Receive, Send, Message

class RequestLoggingMiddleware:
    def __init__(self, app: ASGIApp):
        self.app = app

    async def __call__(self, scope: Scope, receive: Receive, send: Send):
        if scope["type"] != "http":
            await self.app(scope, receive, send)
            return

        def send_wrapper(message: Message):
            return send(message)

        await self.app(scope, receive, send_wrapper)

app = FastAPI(middleware=[Middleware(RequestLoggingMiddleware)])

@app.get("/healthcheck")
async def root():
    return {"message": "Hello World"}

• Wrk result

❯ wrk --latency -t8 -c50 -d10s http://127.0.0.1:8000/healthcheck
Running 10s test @ http://127.0.0.1:8000/healthcheck
  8 threads and 50 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     9.22ms    2.16ms  28.37ms   83.33%
    Req/Sec   654.42     66.64     1.27k    75.62%
  Latency Distribution
     50%    8.58ms
     75%    9.39ms
     90%   11.80ms
     99%   17.41ms
  52211 requests in 10.04s, 7.47MB read
Requests/sec:   5201.42
Transfer/sec:    761.93KB

Interesting :)

[Kludex]

I believe most of the information here is not up-to-date, since we fixed some stuff.

Which problems does the BaseHTTPMiddleware still have?

[adriangb]

Off the top of my head:

• Accessing the request body is problematic.
• ContextVars don't propagate transparently because of the TaskGroup.

[Kludex]

My concern here is that those limitations may not be a reason for deprecating the feature. Since for a beginner, it's easier to use a BaseHTTPMiddleware instead of a pure ASGI middleware.

Also, you brought a solution for the "Accessing the request body is problematic" on #1692, right?

[kissgyorgy]

I would never sacrifice correctness for beginner friendliness. asyncio by itself is not beginner friendly at all and I would also argue if a beginner hit these problems, there is no way they will figure it out.

[Kludex]

The async argument makes sense, but the BaseHTTPMiddleware limitations are documented.

[kissgyorgy]

These are not "limitations", but serious bugs. We lost so much time to these and were having bugs in our middleware for months didn't even notice. Starlette is pretty good and useful, but this middleware is so buggy it should never have been in the codebase at all.
I don't even understand how could you argue for anything against correctness. Every code should be correct first, only after then can you think about anything else.

[Kludex]

The issues you faced on your previous report do not exist anymore.

If you can tell me the "serious bugs" that the BaseHTTPMiddleware still has, it will make your argument stronger.

[Kludex]

Do other members have an opinion here? @encode/maintainers

[gnat]

• Improve wording and structure of pure ASGI middleware docs #1723 (comment)

IIRC, asyncio.shield() resolves the issue with BaseHTTPMiddleware

Documentation is a bit confusing right now... Goes on an off topic explanation of ContextVar (wtf..?) see: https://www.starlette.io/middleware/#limitations

Regardless, I see this as a "recommended pattern" or "documentation" issue at this point rather than something that must be removed. (Do we really want to break FastAPI with Starlette 1.0? 😑)

The documentation in general is getting too wordy and off topic. Should be short and to the point... It's how Starlette originally got popular.

[Kludex]

What BaseHTTPMiddleware issue?

The background tasks issue was already solved.

[adriangb]

Goes on an off topic explanation of ContextVar (wtf..?)

It’s not off topic, BaseHTTPMiddleware does behave different from pure ASGI middleware w.r.t. context vars. We even have a test to prove it:

starlette/tests/middleware/test_base.py

Lines 180 to 211 in 337ae24

	@pytest.mark.parametrize(
	"middleware_cls",
	[
	CustomMiddlewareWithoutBaseHTTPMiddleware,
	pytest.param(
	CustomMiddlewareUsingBaseHTTPMiddleware,
	marks=pytest.mark.xfail(
	reason=(
	"BaseHTTPMiddleware creates a TaskGroup which copies the context"
	"and erases any changes to it made within the TaskGroup"
	),
	raises=AssertionError,
	),
	),
	],
	)
	def test_contextvars(test_client_factory, middleware_cls: type):
	# this has to be an async endpoint because Starlette calls run_in_threadpool
	# on sync endpoints which has it's own set of peculiarities w.r.t propagating
	# contextvars (it propagates them forwards but not backwards)
	async def homepage(request):
	assert ctxvar.get() == "set by middleware"
	ctxvar.set("set by endpoint")
	return PlainTextResponse("Homepage")
	app = Starlette(
	middleware=[Middleware(middleware_cls)], routes=[Route("/", homepage)]
	)
	client = test_client_factory(app)
	response = client.get("/")
	assert response.status_code == 200, response.content

.

asyncio.shield() resolves the issue with BaseHTTPMiddleware

Like Marcelo said there was never a single issue with BaseHTTPMiddleware, there’s been many. And asyncio.shield() was considered to solve the cancellation of background tasks (one of several issues) but we ended up coming up with a better solution (ref #1715)

[gnat]

Ah. So, issues arise when using BaseHTTPMiddleware with contextvars (...which is becoming a common pattern for accessing request data outside of the normal flow?) This makes sense, but know that not everyone uses contextvars- hence the confusion.

• Globally importable request object #420
• Recent user story: Globally importable request object #420 (comment)

Seems tiangolo of FastAPI is aware of this already: #420 (comment)

I mean if you're going to break it, 1.0 is the time to do it. 🤷

[adriangb]

Although I still think BaseHTTPMiddleware is not great we have fixed a lot of the issues it had. I’m warming up to the idea of not removing it for 1.0. I still think we should discourage its use, but maybe we can keep it around until 2.0 to ease the burden for migrating to 1.0? Does something like this even make sense or if we’re going to discourage it should we just remove it?

[Kludex]

I think the "Limitations" section that we have is already a way to discourage it.

Also, as Florimond said above:

[...] might be really off putting a lot of folks who’d like to do very simple edits to requests or responses.

So, I'm happy to keep it.

[adriangb]

I guess that works for now. And it gives us some more time to think about #1691.

[Kludex]

Converting this to a discussion, since it's what it looks. I've already talked to @adriangb about doing it. 👍