⬆️ Upgrade Starlette to 0.28.0

[adriangb]

No description provided.

[github-actions]

📝 Docs preview for commit b522322 at: https://6480fa62dfb68a45d49e263b--fastapi.netlify.app

[github-actions]

📝 Docs preview for commit 19a2549 at: https://6480fdf6c17e9e4bc504459a--fastapi.netlify.app

[adriangb]

I'm not 100% sure why this is changing, I think it's worth looking into a bit more, but I did do a test with a real client (httpx) and a background task that sleeps for 10s before completing and it works correctly (the client gets the response immediately, the background task finishes 10s later)

[tiangolo]

I've been banging my head against the wall trying to understand this 😂

I think I got it. As the new AsyncExitStack returns inside the async with block, with the app to be called later, instead of awaiting it inside, the exit code of the async context managers (dependencies) would be executed first...

Nevertheless, I'm not sure how the manual test with HTTPX would work. 🤔

I tried reverting the changes in AsyncExitStackMiddleware and wrapping like this self.app = AsyncExitStackMiddleware(request_response(self.get_route_handler())).

Then these tests seem to pass, but now another test to check that dependencies with yield can catch exceptions is failing, as if the exception was not bubbling up, I'll keep trying to figure that out tomorrow.

[adriangb]

import anyio
from fastapi import BackgroundTasks, FastAPI, Response


app = FastAPI()


@app.get("/")
async def endpoint(background_tasks: BackgroundTasks) -> Response:
    async def slow() -> None:
        await anyio.sleep(10)
        print("slow() done")
    background_tasks.add_task(slow)
    return Response()


if __name__ == "__main__":
    import httpx
    
    with httpx.Client(base_url="http://localhost:8000") as client:
        resp = client.get("/")
        resp.raise_for_status()
        print("resp done")
        resp = client.get("/")
        resp.raise_for_status()
        print("resp done")

If you run that with uvicorn test:app and then python test.py you'll see that the client gets a response immediately but it takes 10s to print slow() done so it's not like the response is waiting for the background tasks, I don't think there's a bug here, it must just have to do with TestClient and/or how the test is set up.

[adriangb]

Ok yeah I think I agree with you. What’s happening is that since the request is awaited inside of the AsyncExitStack the background tasks run before the dependencies exit. I think we’d have to essentially re-implement request_response you change that, but also encode/starlette#2176 just so happens to “fix” this because background task execution would be moved outside of the the entire app 😂

[adriangb]

Currently the scenario this impacts is users that are doing something time sensitive in their dependencies and have long running background tasks.

[adriangb]

Oh my!

I did do a test with a real client (httpx) and a background task that sleeps for 10s before completing and it works correctly (the client gets the response immediately, the background task finishes 10s later)

The thing is I just assumed this was the correct behavior and what FastAPI currently does. It turns out that it's not what FastAPI currently does! If you run #9636 (comment) on 4b31bee it turns out that the dependencies exit doesn't run until the background tasks finish! In fact, you've mentioned this before: #6871 (comment)

I guess the question now is: is this something we're comfortable breaking or not. It seems like the main use case is to use a database session in background tasks. That would get broken by encode/starlette#2176 so I'm glad we caught this. The fundamental issue comes from the definition of what a background task is: I think of them as something that executes at some point but makes no promises about when. But historically Starlette has executed them within the ASGI request/response cycle after the response is sent, and it seems FastAPI depends on that to be able to pass a database session from an endpoint to a background task.

If we're not comfortable breaking this behavior then one of two things needs to happen:

1. Starlette makes wrap_app_handling_exceptions public
2. FastAPI re-implements wrap_app_handling_exceptions and maybe even puts the AsyncExitStack in there

This is because currently catching HTTPException and awaiting the Response object both happen within request_response so if FastAPI wants to insert its AsyncExitStack in between those two it needs to reimplement request_response which requires calling wrap_app_handling_exceptions

[tiangolo]

Nice, thanks for all the investigation and detailed explanation!

TL;DR: I think the best approach is to follow through in Starlette with the change, and change the behavior in FastAPI, a breaking change, but should be done. Nevertheless, not just right now, in a couple of weeks.

Current behavior

Indeed, currently in FastAPI the start of middlewares is run, then the start of dependencies with yield, then the response is created, the middlewares end is executed, background tasks are executed, and the end of dependencies is executed. It's not intuitive, and now I think it doesn't make that much sense.

My original reasoning, and my main argument in some issues before was that a dependency could create a DB session, and the session could be kept and re-used in background tasks. And then if there was a problem there the dependency would be able to rollback or something.

Problems

Now I disagree with my past self. I think it's wrong to keep the same session in background tasks for several reasons.

The response has to be sent back before background tasks start, which means that the DB session (and possibly connection) is held there waiting for the network, preventing other parts from using the connection, etc. I think some of the problems with DB session/connection deadlocks might be just because of this (only a theory).

Also, if a session is rolled back after the response is sent, whatever the response said was done would be potentially false... but by that point, the session would probably have already committed the data (at least in SQLAlchemy-like engines), but if that's the case, then there are not many savings in keeping the same session instead of creating a new one in background tasks.

This also means that because the exit of dependencies is executed only after the response is sent back, dependencies can't access and modify a response (as you mentioned in this issue: #3500).

It's also awkward and counter-intuitive that the onion is not properly layered, if middlewares cover dependencies, it would make more sense they also cover the exit, not just the start, dependencies end up being a weird thing that runs its exit code in a non-intuitive moment, particularly while having in mind middlewares, exception handlers, and request handlers (endpoint/path operation function).

Wanted behavior

My plan is to change that, so that dependencies' exit code is run before the response is returned.

This means dependencies could access and modify the response, or log, etc.

They could close DB connections before the response is returned, instead of holding that while waiting for the network.

The layering would probably be more intuitive.

Background tasks would not be able to use the same session, they would have to create a new one.

Tradeoffs

The main advantage of the current behavior (that I can think of) is using the same DB session in background tasks.

The disadvantages are all listed above (the same advantages of the planned behavior).

Now, most importantly, I don't think there are actually many people taking advantage of or needing the current "feature". And they probably shouldn't either way, it's probably not a great pattern. I don't think there's a lot of usage of the integrated lightweight background tasks, at least for things that would depend on this behavior... and if there are, that's probably not a good idea, it would probably be better to have an external background job system like ARQ, RQ, Celery, etc.

Plan

That was just to give context, and to explain why I agree it makes sense to have the breaking change in FastAPI. There are still a bunch of details to figure out, e.g. how would a dependency receive, modify, and give back a response.

Also, I've been playing with the idea of allowing request handler functions to yield, having the code after that being executed after the response is sent, it's kind of an inlined background task. Although that's more on the side of the current behavior than the proposed one, I haven't properly thought about that yet, just some brain dump.

Now, mi immediate plan (for these next days) was to take this PR and make a release, just with those minor upgrades, including the bump in the lower range of Pydantic and a couple of minor things (not v2 yet).

I wanted to make one or more small releases with small things and not hard breaking changes before the betas with Pydantic v2 (which are almost ready to be done). Just because I would expect more complications with Pydantic v2, so better to have the small things that can be released already done.

...but, now I feel this behavior change, which makes sense to me, has the potential of being an important breaking change. And in that case, I would probably rather do it after the first releases supporting Pydantic v2 (as that's almost done).

So, my plan is to delay this until after that, and first see if there are small things that should go in the next few simple releases, then Pydantic v2, and then come back to this, and some other bigger things.

[adriangb]

I agree with your points, and I do think having a layering of Starlette -> Middleware & Routing -> Send Response -> Handle Exceptions -> Dependencies -> Endpoint would be more intuitive than what we have now. It would also mean that the Starlette -> Middleware & Routing -> Send Response portion would be pure ASGI and the Handle Exceptions -> Dependencies -> Endpoint portion would be pure Request/Response, which solves the issues coming from converting back and forth and ending up with multiple Request / Response objects*.

Background tasks would not be able to use the same session, they would have to create a new one.

My main concern here would be how they do that. If someone has completely bought into FastAPIs dependency injection system, it could be difficult to also access DB sessions and such outside of the dependency injection system. The current setup allows "cheating" by getting the session from the di system and then passing it out of it. As you said, it's probably not a good pattern but it does work. So I think we'd have to cook up some examples of how users can achieve the same thing if it's no longer allowed to pass dependencies to background tasks. We also have to think of what happens to existing code: it would still "work" except that in the case of a DB session when they try to use it from a background task they'd get some error about using a session after it's been closed/committed.

I tested out encode/starlette#2176 with this PR/branch. No further tests break and no more changes are required. It also gets things to exactly the state I think you want (aside from being able to edit the response, that should also be relatively easy to fix once we figure out the right API for it):

from typing import AsyncIterable
from fastapi import FastAPI, Request, HTTPException, Response, Depends, BackgroundTasks
from starlette.types import ASGIApp, Scope, Receive, Send


app = FastAPI()


async def dep() -> AsyncIterable[None]:
    print("dep start")
    try:
        yield
    except HTTPException:
        print("dep exc")
        raise HTTPException(451)
    else:
        print("dep else")


@app.exception_handler(HTTPException)
async def error_handler(request: Request, exc: HTTPException) -> Response:
    print("error handler")
    return Response(content=b"from error handler", status_code=exc.status_code)


async def background_task() -> None:
    print("background task start")
    await anyio.sleep(1)
    print("background task done")


@app.get("/ok")
async def ok(background: BackgroundTasks, dep: None = Depends(dep)) -> Response:
    print("/ok")
    background.add_task(background_task)
    return Response(b"/ok")


@app.get("/err")
async def err(background: BackgroundTasks, dep: None = Depends(dep)) -> Response:
    print("/err")
    background.add_task(background_task)
    raise HTTPException(418)


class Middleware:
    def __init__(self, app: ASGIApp) -> None:
        self.app = app

    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        print("asgi middelware before")
        await self.app(scope, receive, send)
        print("asgi middlware after")


app.add_middleware(Middleware)



if __name__ == "__main__":
    import anyio
    import httpx
    import uvicorn

    async def main() -> None:

        server = uvicorn.Server(uvicorn.Config(app, log_level="critical", lifespan="off"))

        async with anyio.create_task_group() as tg:
            tg.start_soon(server.serve)
            await anyio.sleep(1)

            async with httpx.AsyncClient(base_url="http://localhost:8000") as client:
                resp = await client.get("/ok")
                print((resp.status_code, resp.content))

            print("")

            async with httpx.AsyncClient(base_url="http://localhost:8000") as client:
                resp = await client.get("/err")
                print((resp.status_code, resp.content))

            await anyio.sleep(2)  # let background tasks finish
            tg.cancel_scope.cancel()

    anyio.run(main)
    """
    asgi middelware before
    dep start
    /ok
    dep else
    asgi middlware after
    background task start
    (200, b'/ok')

    asgi middelware before
    dep start
    /err
    dep exc
    error handler
    asgi middlware after
    (451, b'from error handler')
    background task done
    """

So if you're sure this is the right direction then Starlette can move forward with that change and you can just cut one big "Bump Starlette to 0.29.0 and change behaviors" release after you do the other smaller releases you want to do.

* With the exception of BaseHTTPMiddleware but there's nothing FastAPI can do about that other than encouraging users to use either dependencies or pure ASGI middleware.

[antont]

We are using the pattern you have discussed, passing the session from dependency to bg tasks, which run small things like updating counters in the db, and just yesterday I put our redis cache writes to such a lightweight background task as well.

Have found it nice to be return the API response as soon as possible for the UI, and use the objects that have at hand to make small bg ops simply, including the DB session.

However, I think it will be fine for us to change those to get a new session, and it's simple with the session maker we have exposed in a module anyway. Your reasoning about rollbacks etc. makes sense in our case too.

[adriangb]

@tiangolo do we still want to move forward with this change?

[adriangb]

We can get rid of all of this now because the structure is Starlette -> ServerErrorMiddleware -> User middleware -> routing -> HTTPException handling -> AsyncExitStack -> Dependencies -> Endpoint (in other words, Starlette moved what used to be ExceptionMiddleware to be inside of routing)

[wu-clan]

Hi.

Will this solve the problem of unexpected hangs when using await request.body(), json(), and form() methods in middleware?

I just installed Starlette 0.28.0 for testing, after obtaining await request.body(), json(), and form().，await self.app(scope, receive, send) unexpectedly hung up

I'm just using them for logging purposes

[adriangb]

Not sure what you are doing or trying but the example in #5386 works on this branch so yes it does solve the problem.

[tiangolo]

📝 Docs preview for commit 288dd7c at: https://64853da1aab7f44a4d6614c3--fastapi.netlify.app

[michaelgmiller1]

Would be great to get this merged in to pick up this change: encode/starlette#2026

[michaelgmiller1]

Anything in particular blocking this?

[Gribesh]

Can this PR be merged? or even upgrade to a higher version of Starlette. :)

I am having some dependency conflicts. Currently using --no-deps flag to install requirements.

[rogelioLpz]

Any news about this PR ?
Is there an estimated date to merge it?

[a410202049]

Is there an estimated date to merge it?

[michaelgmiller1]

Starlette is now up to 0.31.1 - any reason that FastAPI can't be upgraded? This continues to be a thorn in our side.

[thornewolf]

Would love to get this merged. Starlette has added initializing Jinja2Templates from an existing env (Using custom jinja2.Environment instance), which is only available in later versions.

Current version pinning makes it difficult to use these latest features.

[rafalkrupinski]

@thornewolf merging it could be only one git commit away. It could be your git commit...

[thornewolf]

@rafalkrupinski Perhaps I'm mislead but I think the necessary commits are already finished. We just need merge approval?

[rafalkrupinski]

@rafalkrupinski Perhaps I'm mislead but I think the necessary commits are already finished. We just need merge approval?

From the latest comments I take it isn't finished, and even if it was, the branch is in conflict.

[musicinmybrain]

@rafalkrupinski Perhaps I'm mislead but I think the necessary commits are already finished. We just need merge approval?

From the latest comments I take it isn't finished, and even if it was, the branch is in conflict.

To me it seems clear from #9636 (comment) that we’re waiting for @tiangolo to return to the topic and decide exactly how (and when) to proceed. Nothing gets merged without @tiangolo anyway. I don’t see what a random contributor could offer here.

[rafalkrupinski]

To me it seems clear from #9636 (comment) that we’re waiting for @tiangolo to return to the topic and decide exactly how (and when) to proceed. Nothing gets merged without @tiangolo anyway. I don’t see what a random contributor could offer here.

Admittedly I only skimmed thru the comments. Of course you're right, it's just waiting for @tiangolo

[vfazio]

I had a naive approach that simply hooked the exception chain and reraised exceptions in the context of the AsyncExitStack that worked from starlette 0.27 -> 0.31.1. Looks like this may be a more full fledged/robust change.

[tiangolo]

I needed to finish this dependency refactor first: #10831, and that had to be done after the Pydantic v2 migration. But now that is done, the way is free for the Starlette upgrades! 🎉

[tiangolo]

I just merged master into this branch, and because of the recent changes in the dependencies PR, now this one was a clean pure Starlette upgrade. 😎

Thanks for all the discussion and work here @adriangb! 🚀

This will be available in FastAPI 0.107.0 in the next hours. 🎉

I'll try to make several consecutive releases upgrading each recent version of Starlette (if everything works out well). 🤞

[adriangb]

Awesome stuff @tiangolo !

[CFelipezapata]

Hello team! @tiangolo and @adriangb I'm not sure if anyone noticed, but I started getting this error when using fastapi==0.107.0 and fastapi==0.108.0 and I see this is the PR related to the first of the mentioned releases.

I am using as well uvicorn==0.24.0.post1 but also got the same error with uvicorn==0.25.0

I had to rollback to using fastapi==0.106.0 in the mean time and my issue was resolved.

2023-12-26 17:54:01 Traceback (most recent call last):
2023-12-26 17:54:01   File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 192, in __call__
2023-12-26 17:54:01     await response(scope, wrapped_receive, send)
2023-12-26 17:54:01   File "/usr/local/lib/python3.10/site-packages/starlette/responses.py", line 259, in __call__
2023-12-26 17:54:01     await wrap(partial(self.listen_for_disconnect, receive))
2023-12-26 17:54:01   File "/usr/local/lib/python3.10/site-packages/starlette/responses.py", line 255, in wrap
2023-12-26 17:54:01     await func()
2023-12-26 17:54:01   File "/usr/local/lib/python3.10/site-packages/starlette/responses.py", line 232, in listen_for_disconnect
2023-12-26 17:54:01     message = await receive()
2023-12-26 17:54:01   File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 52, in wrapped_receive
2023-12-26 17:54:01     msg = await self.receive()
2023-12-26 17:54:01   File "/usr/local/lib/python3.10/site-packages/sentry_sdk/integrations/starlette.py", line 136, in _sentry_receive
2023-12-26 17:54:01     return await receive(*args, **kwargs)
2023-12-26 17:54:01   File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 128, in receive_or_disconnect
2023-12-26 17:54:01     message = await wrap(wrapped_receive)
2023-12-26 17:54:01   File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 123, in wrap
2023-12-26 17:54:01     result = await func()
2023-12-26 17:54:01   File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 56, in wrapped_receive
2023-12-26 17:54:01     raise RuntimeError(f"Unexpected message received: {msg['type']}")
2023-12-26 17:54:01 RuntimeError: Unexpected message received: http.request

[adriangb]

I don’t recall seeing that before. Can you post a new issue with a minimal reproducible example and link it here? Thanks

[CFelipezapata]

I don’t recall seeing that before. Can you post a new issue with a minimal reproducible example and link it here? Thanks

I have solved the issue with the help of another user. This happens when using a middleware and trying to awaiting the request.body from it. There used to be a workaround that we were using from this github discussion. But now, the workaround seems to be much simpler with a higher version of Starlette. Here's my comment with an MRE.

[staticdev]

I am still having RuntimeError: Unexpected message received: http.request with latest fastapi 0.108.0 and startlette 0.32.0.post1, with a:

@app.middleware('http')
async def my_middleware(request: Request, call_next: Callable[[Request], Response]) -> Response:
    # do smthing
    response = await call_next(request)  # type: ignore[misc]
    # do smthing
    return typing.cast(Response, response)

Do we have to change our middlewares somehow or it is a bug to be fixed @Kludex @tiangolo ? I did not find an open issue besides this PR and the discussion mentioned.

[Kludex]

Read this: #8187 (reply in thread)

[catarse-natural]

@Kludex also not clear for me this comment, so this is a backward incompatible change? Why existing middlewares stopped working? I also saw another upgrade PR for Starlette 0.35 (https://github.com/tiangolo/fastapi/pull/10798/files), does it maybe keep current middlewares working?