Document interaction of BaseHTTPMiddleware and contextvars

[adriangb]

Using BaseHTTPMiddleware changes the behavior of the application because creating a TaskGroup erases changes to the context made within tasks spawned from the TaskGroup.

This PR is merely documenting the behavior, not necessarily proposing a fix or change.

As I understand it, the implications of this behavior were direct cause for a recent FastAPI minor version release with breaking changes (https://github.com/tiangolo/fastapi/releases/tag/0.74.0). I think it is important to at least document this behavior so that it is well understood going forward and so that we can know if it changes.

That said, one possible fix, if we decide we want to "fix" this, would be something along the lines of #1258

[peku33]

Got into the same issue.
When settings contextvars in app with multiple BaseHTTPMiddlewares they are not visible to each other,

[adriangb]

Would you be able to share your concrete use case @peku33 ? As is right now this is a very "theoretical" issue, but it would help to get some actual use cases 😃

[peku33]

I am using one middleware to populate request context (contextvar) with some details and another middleware to provide logging based on that data.

Data added to contextvar is only visible to "inner" middlewares, like if the context was cloned and frozen before running it.

example:

def debug_middleware(index: int) -> Middleware:
    async def _dispatch(
        request: Request,
        call_next: RequestResponseEndpoint,
    ) -> Response:
        if index == 2:
            # stub populator
            user = User("a", "b", True)  # FIXME
            request_context = RequestContext(
                user=user,
            )
            REQUEST_CONTEXT_VAR.set(request_context)
            print("populated with", request_context)

        print("debug pre", index, REQUEST_CONTEXT_VAR.get())
        r = await call_next(request)
        print("debug post", index, REQUEST_CONTEXT_VAR.get())

        return r

    return Middleware(BaseHTTPMiddleware, dispatch=_dispatch)


app = Starlette(
    middleware=[
        request_context.debug_middleware(1),
        request_context.debug_middleware(2),
        request_context.debug_middleware(3),
    ],
)

the result is:

debug pre 1 None
populated with RequestContext(user=...)
debug pre 2 RequestContext(user=...)
debug pre 3 RequestContext(user=...)
debug post 3 RequestContext(user=...)
debug post 2 RequestContext(user=...)
debug post 1 None

while I would rather expect it to be:

debug pre 1 None
populated with RequestContext(user=...)
debug pre 2 RequestContext(user=...)
debug pre 3 RequestContext(user=...)
debug post 3 RequestContext(user=...)
debug post 2 RequestContext(user=...)
debug post 1 RequestContext(user=...) << different here

What is even funnier - it looks like the last middleware shares the context with the worker (with unicorn), because if the contextvar is set in the first (outermost) middleware - it is visible from inside of the worker.

[Kludex]

What "same problem"? You mean the same as caused by the TaskGroup?

[adriangb]

Yes. Although I think this has been fixed in the meantime. Let me re-confirm.

[Kludex]

this?

[adriangb]

Yep I think that should fix the issue. It doesn't change the rest of this PR, but we can just remove my comment.

[adriangb]

Ah okay, no the issue is different: to_thread.run_sync() now propagates the context _ forwards_ but not backwards:

from contextvars import ContextVar
import anyio

ctx = ContextVar[str]("ctx")

async def async_func() -> None:
    assert ctx.set("bar")

def sync_func() -> None:
    assert ctx.set("foo")

async def main() -> None:
    await async_func()
    assert ctx.get() == "bar"
    await anyio.to_thread.run_sync(sync_func)
    assert ctx.get() == "foo"  # fails

anyio.run(main)

So there is still a subtle difference between a async and async endpoint. Side note: I think at some point before a 1.0 support for sync endpoints should be dropped, I think @tomchristie suggested this as well in the past via Gitter (Tom if you recall any of this conversation, or don't think it happened, please correct me if I am wrong).

I'm also happy to add a similar test in another PR documenting that edge case.

[Kludex]

Let's go with this. It just adds a confirmation of the behavior we are aware.

[Kludex]

Thanks @adriangb :)