Remaining bugs/limitations of BaseHTTPMiddleware - and what to do about them

[jhominal]

When #1438 is fixed, the documentation explanation for deprecating BaseHTTPMiddleware should be reviewed, in particular its remaining bugs and limitations.

Personally, I can think of two issues in the implementation of BaseHTTPMiddleware (once #1438 is fixed):

1. Context variables that are set downstream cannot go "upstream" of a BaseHTTPMiddleware - that is the consequence of the downstream ASGI application being run in a task distinct from the upstream app;
2. If there is a request body and BaseHTTPMiddleware.dispatch reads from it, then the request body will not be available for call_next - there will be either a deadlock, the first packet will be missing, etc. - that is the consequence of call_next hooking itself directly to request.receive, and of there being no attempt at buffering the request body (which, in my opinion, would be a bad idea);

Does anyone have any other bugs/limitations in mind?

Now, what to do about the issues I mentioned?

• For the context variables issue:
  • I suspect that it can be "mostly fixed" by running dispatch in a new task, and running the downstream ASGI app in the same task (no guarantee, it came to my mind as I was writing this post);
  • this has likely tradeoffs around the behavior of context variables, e.g. if the middleware itself sets them;
  • but the whole weight of these tradeoffs would be on the people who are using BaseHTTPMiddleware, instead of the current situation where the people who use BaseHTTPMiddleware can be completely unaware of the way that they are breaking context variables for other applications and middlewares;
  • I think such a solution would have the advantage that people who currently have BaseHTTPMiddleware that do not make use of context variables would have nothing to change, and their middlewares would stop breaking other people's context variables;
  • I could spend a few hours trying that, is anyone interested?
• For the read request body issue, I would be in favor of keeping that limitation in BaseHTTPMiddleware, and document it more strongly, and I would even look into raising an exception if dispatch calls both request.receive (directly or indirectly) and call_next;

[jhominal]

I would like to add that it is not my intent to question the deprecation of BaseHTTPMiddleware, but only to limit the damage that it can do for as long as it keeps being used.

[adriangb]

👍 so far I think I am the only one publicly advocating for deprecation, it is certainly not a concensus from the entire team or anything like that. I've also been working both sides of the coin: advocating for deprecation but also trying to fix the issues. Appreciate your work and approach on this.

[adriangb]

For the context variables issue [...] I suspect that it can be "mostly fixed" by running dispatch in a new task

I'm not as good at visualizing streams/tasks in my head as it seems you are, I think I would need to see some code to understand this approach.

There might also be a workaround for 3.11+: #1258 (comment)

For the read request body issue, I would be in favor of keeping that limitation in BaseHTTPMiddleware, and document it more strongly, and I would even look into raising an exception if dispatch calls both request.receive (directly or indirectly) and call_next

I had a proof of concept for this at one point: #1528

that is the consequence of call_next hooking itself directly to request.receive, and of there being no attempt at buffering the request body (which, in my opinion, would be a bad idea)

Have you had a chance to look at #1692? My logic here was that Request.body() already is expected to store the body in memory, so buffering that should be okay. So by subclassing Request I was able to (1) differentiate Request.body() from Requesr.stream(), (2) avoid modifying Request itself / only doing this in BaseHTTPMiddleware and (3) only buffering for the downstream middleware / downstream middleware can still consume the stream, modify/replace it, etc. without having to deal with cache invalidation and such.

[jhominal]

Oh, it does seem that 3.11 will have a feature that has the potential to solve the issue of context variables bubbling to parent tasks. Interesting. Probably too late to save BaseHTTPMiddleware, but interesting!

For the request content, I am of the mind that currently, because of call_next using request.receive directly, I know that it does not work to consume the body in both dispatch and in call_next (unless someone writes a new request.receive function, but then they are already half-way to implementing an ASGI middleware). Right now, I would prefer warding off that limitation in a deterministic way, e.g. by forbidding call of call_next when the body has been consumed by dispatch.

I would be extremely wary of allowing .body() calls by a middleware to trigger loading of the whole body in memory, because I think that the middleware, by its nature, generally does not have the necessary context to take the decision of "is that content loadable or not", and would lead to an epidemic of "I have a middleware, I will call .body() to read the content" completely neutralizing streaming request bodies.

I think that conceptually, HTTP middleware should handle request and response bodies either in a streaming fashion, or not at all. And there is a gap here - while there exists a StreamingResponse in starlette to handle response bodies in a streaming way, no similar tooling exists for handling of Request bodies, as .receive is the authoritative for the request body.

[jhominal]

Here is my starting proposal for fixing contextvars: jhominal#1

It is still a work in progress - in particular, as written, contextvars set in dispatch will not be visible in the endpoint, but I intend to fix that by providing an argument on call_next that takes the list of contextvars that should be visible to the application.

[jhominal]

Now that I reread this merge request, I realize that I am now running the send and receive callables in the context of dispatch... that may be an issue, need to explore that.

[jhominal]

Here is my analysis of the contextvars issue, and of the impact of BaseHTTPMiddleware on them.

Because of the way that BaseHTTPMiddleware spawns tasks in order to bridge ASGI with the Request/Response API that it exposes, that means that the user code that is run in a spawned task can be unable to set context variables that can be read in the parent task.

Here are the various pieces of user code that interact with BaseHTTPMiddleware, where context variables may be used:

• Upstream: the caller of BaseHTTPMiddleware.__call__;
• app: the downstream application, wrapped by BaseHTTPMiddleware;
• dispatch: the middleware user code;
• receive: the receive callable set in __call__
• send: the send callable set in __call__

Here is how the tasks are currently organized in starlette:

• Main task: Upstream -> dispatch/receive/send
• Spawned task: (Copy of dispatch context) -> app

Which leads to this matrix of "who can read context variables set by whom":

Context variable set by	Upstream	app	dispatch	receive	send
Can be read by Upstream	✅	❌	✅	✅	✅
Can be read by app	✅	✅	✅	❌	❌
Can be read by dispatch	✅	❌	✅	✅	✅
Can be read by receive	✅	❌	✅	✅	✅
Can be read by send	✅	❌	✅	✅	✅

My main issue with that matrix, is that app cannot set variables that will be read upstream - that means that, like in bug #1438, the simple existence of a BaseHTTPMiddleware-based middleware in the chain introduces dysfunctions that can only be fixed by the application deployer (by stopping the usage of middlewares that subclass BaseHTTPMiddleware) or by the middleware implementer (stop subclassing BaseHTTPMiddleware), but not by the victim (the application that sets context variables, or the upstream code that needs to read these context variables).

In order to fix my main issue, I propose the following change in the way that user code is run in various tasks:

• Main task: Upstream -> app
• Spawned task: (Copy of Upstream context) -> dispatch/receive/send

Such a change was implemented, as a work in progress, in jhominal#1

That change leads us to this matrix:

Context variable set by	Upstream	app	dispatch	receive	send
Can be read by Upstream	✅	✅	❌	❌	❌
Can be read by app	✅	✅	❌	❌	❌
Can be read by dispatch	✅	❌	✅	✅	✅
Can be read by receive	✅	❌	✅	✅	✅
Can be read by send	✅	❌	✅	✅	✅

Looking at this matrix, there are two regressions brought by the change:

• dispatch cannot set context variables that can be read Upstream, or by the app;
  • In order to handle that case, I would propose adding a new argument to call_next, where dispatch can specify a list of contextvars, the value of which would be flowed to app (and upstream);
• receive and send cannot set context variables that will be readable upstream.
  • I have a hard time imagining that there are many receive and send implementations that set context variables, in the expectation of these context variables being readable everywhere. That is particularly the case for receive, which, in many cases, is going to be called from a new task if the application wishes to handle http.disconnect;
  • Thus, I would be in favor of accepting that change in behavior;

What does everyone else think?

[jhominal]

Here are my off the cuff reactions, just looking at your proposed change:

1. Until I saw your proposed change I had thoughtlessly believed that bpo-46994: Accept explicit contextvars.Context in asyncio create_task() API python/cpython#31837 would allow us to run subtasks in the same context as the parent task, and of course that is not the case (there is no API to get a reference to the current Context object, and a single Context object cannot be used to run two functions at a time);
2. Enumerating context keys to detect which context variables have changed is something that I would prefer to avoid - it goes a bit against part of the promise of contextvars, which is that, no matter how many variables you have, you can pop subtasks without incurring a runtime cost (and the underlying data structure is heavily optimized for that purpose);
3. I suspect that there may be a way to emulate the context argument from create_task on Python<3.11, by using Context.run and hacking at the __await__ level, but that would have to be confirmed in one way or another;
4. I would still prefer to apply reorganize BaseHTTPMiddleware tasks so that app is in the same task as __call__ jhominal/starlette#1 before applying a change like yours to synchronize context variables across contexts, because I believe that gives us more options in tuning the "synchronize context vars" strategy;

[adriangb]

1. Until I saw your proposed change I had thoughtlessly believed that bpo-46994: Accept explicit contextvars.Context in asyncio create_task() API python/cpython#31837 would allow us to run subtasks in the same context as the parent task, and of course that is not the case (there is no API to get a reference to the current Context object, and a single Context object cannot be used to run two functions at a time);

Yes, you're right. @agronholm and I asked about this: https://discuss.python.org/t/back-propagation-of-contextvar-changes-from-worker-threads/.

2. Enumerating context keys to detect which context variables have changed is something that I would prefer to avoid - it goes a bit against part of the promise of contextvars, which is that, no matter how many variables you have, you can pop subtasks without incurring a runtime cost (and the underlying data structure is heavily optimized for that purpose);

Theoretically, I agree. Practically though, Django has had this for years and I haven't heard anyone complain about performance. I also ran a quick benchmark:

from contextvars import ContextVar
from time import time
import anyio

vars = tuple([
    ContextVar[int](str(n))
    for n in range(1_000)
])
for n, var in enumerate(vars):
    var.set(n)

start = time()
for _ in range(1_000):
    for var in vars:
        val = var.get()
        if val % 10 == 0:
            var.set(val)
end = time()
print(end - start)

async def task():
    ...

start = time()
for _ in range(1_000):
    async with anyio.create_task_group() as tg:
        tg.start_soon(task)
end = time()
print(end - start)

I got 0.09 and 0.07 respectively. I chose create_task_group() as an arbitrary unit of work that BaseHTTPMiddleware is already doing (it's also doing a lot more stuff). So I think in practice the performance impact wouldn't be that much greater than just using BaseHTTPMiddleware in the first place unless someone is using 10,000 context variables and setting hundreds of them for every request, in which case that's a super niche use case and the fix would be to not use BaseHTTPMiddleware (they currently wouldn't even be able to use it, so slow would be an improvement over broken even for this niche use case).

4. I would still prefer to apply reorganize BaseHTTPMiddleware tasks so that app is in the same task as __call__ jhominal/starlette#1 before applying a change like yours to synchronize context variables across contexts, because I believe that gives us more options in tuning the "synchronize context vars" strategy;

I'm open to any solution (and I think yours is pretty amazing), I just want us to compare pros and cons.

[jhominal]

So I think in practice the performance impact wouldn't be that much greater than just using BaseHTTPMiddleware in the first place unless someone is using 10,000 context variables and setting hundreds of them for every request, in which case that's a super niche use case and the fix would be to not use BaseHTTPMiddleware (they currently wouldn't even be able to use it, so slow would be an improvement over broken even for this niche use case).

Very fair. You are right, I should not get too caught up on performance in BaseHTTPMiddleware, given how much work it already does in order to be able to provide its interface.

I think that waiting for Python 3.11+ to fix the "app cannot set context variables that can be read upstream of a BaseHTTPMiddleware" seems a bit late to me. I will think of how to get a fix with "copying context variables" that would work with current APIs (I think that calling copy_context() from coro could work? Or maybe it does not work with the workflows as they are, it does not seem that I can get a handle on the context at the right time.)

[jhominal]

I think that, using the principle of "copying context variables", I was able to evolve jhominal#1 so that:

• context variables that are set in dispatch are available both upstream, and downstream in the app;
• it works on Python 3.7+, no need to wait for Python 3.11;

There is still a tradeoff in that, in this version of the fix, context var modifications that happen after call_next is called are not transferred upstream.

In other words, here is the updated matrix of context var visibility, having removed send/receive from the analysis, and adding a distinction for "dispatch before call_next" vs "dispatch after call_next":

Before jhominal#1

Context variable set by	Upstream	app	dispatch before call_next	dispatch after call_next
Can be read by Upstream	✅	❌	✅	✅
Can be read by app	✅	✅	✅	❌
Can be read by dispatch before call_next	✅	N/A	✅	N/A
Can be read by dispatch after call_next	✅	❌	✅	✅

After jhominal#1 (both commits)

Context variable set by	Upstream	app	dispatch before call_next	dispatch after call_next
Can be read by Upstream	✅	✅	✅	❌
Can be read by app	✅	✅	✅	❌
Can be read by dispatch before call_next	✅	N/A	✅	N/A
Can be read by dispatch after call_next	✅	❌	✅	✅

In other words, we exchange the bug "downstream context variables do not flow upstream" with "context variables from middleware, after call_next, do not flow upstream". I think that exchange is more than fair, especially as a middleware can still use mutable objects in the context vars to make changes that will be available to the concurrent tasks.

[adriangb]

I think that exchange is more than fair

💯 agreed. I think that as long as Upstream and app can communicate we are in a much better place. The issue currently is that if you (Jean) are writing an app and use some library that I (Adrian) wrote that is implemented using BaseHTTPMiddleware and that breaks some ContextVar used by your app/Upstream the only thing you can do is come ask me to change my middleware to accommodate some other library or your private code. With this change, what would break (if anything) is my middleware -> you are now asking me to fix a bug in my middleware. I don't know of any middleware that is setting/getting context variables within dispatch and if they exist I think the answer is simple: rewrite it as a pure ASGI middleware.

[jhominal]

After reading #1678 (comment) it appears that the following issue has also been identified in BaseHTTPMiddleware, which seems to be that exceptions can be swallowed, due to race conditions that exist around the app_exc variable.

I think that fixing that race condition should be possible.

[adriangb]

Has anyone looked into this further / worked on it?

[jhominal]

I have also continued to think about the issue of processing of request bodies in a BaseHTTPMiddleware, and I think the best solution for that issue would be to implement these two action items:

1. Completely preclude the possibility of double consumption by checking that dispatch only calls at most one of request.receive and call_next, and raise an exception if that condition is not met.
2. In order to allow people to process the request (and response) bodies in a streaming fashion, I would propose adding the following lines to base.py:

AsyncByteStream = typing.AsyncIterable[bytes]
AsyncByteStreamFunction = typing.Callable[[AsyncByteStream], AsyncByteStream]


class BaseHTTPMiddleware:

    @staticmethod
    def wrap_request_body(request: Request, func: AsyncByteStreamFunction) -> Request:
        wrapped_stream_iterator = func(request.stream()).__aiter__()
        stream_exhausted = False

        async def wrapped_receive() -> Message:
            nonlocal stream_exhausted
            if not stream_exhausted:
                try:
                    chunk = await wrapped_stream_iterator.__anext__()
                    return {"type": "http.request", "body": chunk, "more_body": True}
                except StopAsyncIteration:
                    stream_exhausted = True
                    return {"type": "http.request", "body": b"", "more_body": False}
                except ClientDisconnect:
                    stream_exhausted = True
                    return {"type": "http.disconnect"}
            else:
                return await request.receive()

        return Request(scope=request.scope, receive=wrapped_receive, send=request._send)

    @staticmethod
    def wrap_response_body(response: Response, func: AsyncByteStreamFunction) -> Response:
        if not isinstance(response, StreamingResponse):
            raise ValueError(
                "wrap_response_body can be used only on responses issued by call_next"
            )

        response_with_wrapped_content = StreamingResponse(
            content=func(response.body_iterator),
            status_code=response.status_code,
        )
        response_with_wrapped_content.raw_headers = response.raw_headers
        return response_with_wrapped_content

In other words, we could provide helper functions that help users of BaseHTTPMiddleware to process the content stream in a streaming fashion, whether in the request or in the response.

[antonpirker]

Very interesting discussion!

I built the official Sentry integration for Starlette and a user has now exact the problem of the request freezing because Sentry reads the request and then a BaseHTTPMiddleware-based middleware tries to read the request again.

See: getsentry/sentry-python#1573

Is there any work-around that I could implement to prevent the hanging from happening?
I can not think of anything that I could do in the Sentry SDK, to allow middlewares that run after Sentry to access the request body with BaseHTTPMiddlewares call_next() method...

Any pointers or hints are greatly appreciated.

[adriangb]

Could you rewrite it as a pure ASGI middleware?

[adriangb]

Sorry looking at the code I realized that the Sentry middleware itself is an ASGI middleware but the user's middleware is not. For the record, it looks like the body is consumed here.

[adriangb]

I think you have two options here:

1. Buffer the request body and pass this buffer onto downstream apps.
2. Wrap receive so that as the downstream application streams the request body you keep track of the size.

(1) is easier to implement in principle. The simplest implementation where you just store the body into memory is trivial but is not suitable for production because you can easily run into memory exhaustion. You could use SpooledTemporaryFile or something like that but even then if a user is trying to stream bytes into a database, proxy them to another service, etc. it would be a huge performance hit to write everything to disk as it passes through the app.

(2) has a lot more edge cases from the get go (e.g. what to do if the request body is never consumed by the app or if the app starts consuming it and then crashes) but I think this would scale better to large requests, high concurrency, etc. One issue here is that you won't have the request body size until after the request is done processing (although I assume you batch metadata exports so it's not realtime anyway).

This said, I think trying to get the real bytes from the request body is problematic in general. I don't know if it's possible/useful but I think I would have implemented this metadata in two pieces: (1) the value from the Content-Length header (if it exists) and (2) the bytes read by the app (which often, but not always, will be the entire request body). Then solution (2) would be easy to implement because there aren't really any edge cases.

[antonpirker]

Thanks for this great input @adriangb
So this is nothing I can implement in an hour or two :-)
We have to work on the next release of our SDK (going out this week), but afterwards I will have more time to dive into this.

[adriangb]

Sounds good, feel free to ping if you have any questions or need examples

[adriangb]

Coming back to this. I think the state of the issues and their possible solutions are:

• BackgroundTask cancellation: Replace task cancellation in BaseHTTPMiddleware with http.disconnect+recv_stream.close #1715 and/or Move BackgroundTask execution outside of middleware stack #1700 (I think we should do both)
• Reading request bodies: Reuse Request's body buffer for call_next in BaseHTTPMiddleware #1692 or Cache request 'body' and 'stream_consumed' in the ASGI scope #1519 (I think one or the other)
• ContextVars: reorganize BaseHTTPMiddleware tasks so that app is in the same task as __call__ jhominal/starlette#1 (@jhominal is this ready for a PR to Starlette itself?)
• Deprecating BaseHTTPMiddleware #1678 (comment): not sure about this, I don't know that anyone has looked into it.