Future of on_startup/on_shutdown and lifespan

[adriangb]

Background:

• Further develop startup and shutdown events fastapi/fastapi#617
• Add support for contextlib-like lifecycle handlers. #785
• Support generator function to manage startup/shutdown. #799

I don't think we've been explicit yet as to what the planned future state is for lifespan and on_startup / on_shutdown. We currently have two mutually exclusive ways to do things, which is confusing for users and adds complexity to our codebase (see #2068 for how much complexity).

Using async context managers is (as far as I can tell) never worse than on_startup / on_shutdown and makes some use cases much easier (if you need to wrap another context manager like httpx.AsyncClient). Also as discussed in fastapi/fastapi#617 using on_startup / on_shutdown has some serious foot guns.

Given that using async context managers are all around better, I propose we get rid of on_startup and on_shutdown for 1.0.

What do other think?

cc @graingert @tomchristie @Kludex

[euri10]

what foot guns are you talking about, I admit I didnt read entirely the thread, but I never had any issue with on_startup and on_shutdown

[Kludex]

I also didn't read the whole thread, but based on what I read, the lifespan state would solve the issue on_startup and on_shutdown have. What are the foot guns? 🤔

I think most of our users use on_startup and on_shutdown instead of lifespan, and I don't feel like there's a huge cost to maintain a feature that everybody uses - maybe the new lifespan state adds a bit of complexity to it since we are introspecting the functions, but we can also deprecate the ()->None and only allow (state)->None as an alternative.

[tiangolo]

Funny story... 😬 It's quite a contradiction from my side, but anyway, here it goes.

• I currently don't have support for lifespan functions in FastAPI, but I should (there might even be a PR for that). I do have startup and shutdown.
• I agree it makes sense to remove on_startup and on_shutdown. And replace that with a single lifespan context manager/function with yield. 😬

Then, the next steps would be that I would have to patch on_startup and on_shutdown in FastAPI for backwards compatibility for a while, add support for lifespan, document it, recommend it, discourage using on_startup and on_shutdown and direct people to lifespan. And I think all that should be done, and I agree with removing these two things here. 😅

---

If anyone wants arguments from my side for my point of view I can expand on it. But as I suspect I would be the main expected "opposition" to this (given my contradictions above) and I'm actually in favor, maybe I can avoid making this message longer.

---

Edit 2023-03-07: I just released FastAPI 0.93.0, with support for lifespan, and marking it as the recommended way going forward. 🎉

(I also scratched above the sections that are no longer true/necessary).

Here are the updated docs: https://fastapi.tiangolo.com/advanced/events/

[adriangb]

Look like you just went ahead and... made this happen! Given that most of Starlette's users are FastAPI users, and that FastAPI now supports and recommends async context managers, I'm even more sure that Starlette should just remove the other options.

[Kludex]

I'm still not convinced it's a good idea. 🤷

But... If we want to proceed with this, we should:

1. Deprecate on_startup and on_shutdown.
2. Accept Alternative lifespan state api #2065, and not support state on on_startup and on_shutdown.
3. Remove on_startup and on_shutdown on 1.0.

With this, FastAPI doesn't need to do anything.

Let's wait for a week or so to see if any other member is strong about not doing this. cc @encode/maintainers

[sm-Fifteen]

The main footgun is forgetting/failing to teardown your ressources. Your code will work no matter how you initialize your resources, but there are plenty of "fun" ways it will break if you don't deinitialize them afterwards. It's the main reason why lifetime is a thing in the first place. Reading the lifetime.state MR, the second footgun is thread-safety of shared ressources†, As far as I can tell, though, this one would involve rewriting all the existing user code relying on lifespan (both events and context managers) to use request.state instead of app.state or the global scope.

Enforcing the CM approach gives the user more ways to properly handle teardown, such as relying on object-provided context managers to have the objects be deinitialized automatically, or a closing wrapper. Having separate functions for each is, IMO, an artifact of the initial design in the case of Starlette (the "event" system has never been extended beyond those two), and FastAPI didn't even have any code for it initially, it merely inherited it by virtue of... well, inheriting from the Starlette class.

Besides compatibility with applications that already use on_startup and on_shutdown, my personnal opinion is that context manager lifetime should be the prefered way. The "two events" are certainly well established now, but I think deprecating (and eventually removing) them in favor of a single CM, especially in preparation of a 1.0 release where breaking stuff won't be an option anymore, would be preferable. Adding the state parameter to them would just reinforce that they're still perfectly valid to continue using.

† From my understanding, lifetime is now meant to run per process and per thread, which actually raises a few questions regarding whether multiple SQLAlchemy connection pools could end up being initialized even though these are thread-safe. But I get how the idea is that thread-unsafe objects could now be safely be initialized and stored, and the un-pickleability of your resources would't cause issues with multi-process applications.

[sm-Fifteen]

In async-land especially, your ressources can't be deinitialized on their own, so it's important that the users get it right. From the Async SQLAlchemy doc:

It’s advisable to invoke the AsyncEngine.dispose() method using await when using the AsyncEngine object in a scope that will go out of context and be garbage collected, as illustrated in the async_main function in the above example. This ensures that any connections held open by the connection pool will be properly disposed within an awaitable context. Unlike when using blocking IO, SQLAlchemy cannot properly dispose of these connections within methods like __del__ or weakref finalizers as there is no opportunity to invoke await. Failing to explicitly dispose of the engine when it falls out of scope may result in warnings emitted to standard out resembling the form RuntimeError: Event loop is closed within garbage collection.

[adriangb]

From my understanding, lifetime is now meant to run per process and per thread, which actually raises a few questions regarding whether multiple SQLAlchemy connection pools could end up being initialized even though these are thread-safe

Good point. I have no idea how SQLAlchemy would handle an async connection pool being shared across threads. I would personally just bite the bullet and use one connection pool per event loop, even if that is theoretically wasteful. At that point one could argue that using Python itself is bad because you're basically forced to spin up more processes and hence can't share connections that in another language could have been shared between threads. Plus, if you really wanted to and fully understood how SQLAlchemy works you could use a thread local or something like that. But at least the default case now is going to be storing things per event loop instad of at the module level.

[sm-Fifteen]

I have no idea how SQLAlchemy would handle an async connection pool being shared across threads. I would personally just bite the bullet and use one connection pool per event loop, even if that is theoretically wasteful.

Ah, right, you apparently can't share an AsyncEngine between event loops. As for the thread-safety of the event loops themselves, I never actually thought about this. Asyncio has always been 1 main event loop thread -> thread pool of n runner threads -> only sync stuff to me, and having multiple loops across many threads never even occured to me, especially in Python where the GIL would likely stop a setup like this from being anymore performant than just one loop.

At that point one could argue that using Python itself is bad because you're basically forced to spin up more processes and hence can't share connections that in another language could have been shared between threads.

Yeah, it does remove a lot of the benefits from multi-threading. I get why the interaction between threads and lifetime wasn't even mentionned in the ASGI spec until recently, since it would be such a strange use case to have multiple threads with their own loops and everything.

if you really wanted to and fully understood how SQLAlchemy works you could use a thread local or something like that. But at least the default case now is going to be storing things per event loop instad of at the module level.

So this really is more about "loop-safety" than thread-safety, in a sense?

[adriangb]

Yeah it is about loop safety. It shows up in unexpected places. For example TestClient creates event loops (if I remember correctly, it may re-use them or something) so you can't run multiple TestClient pointing at the same app object concurrently because you'll end up with a mix of things from different event loops. With the new state management TestClient doesn't need to keen state via the global app object.

But just as much about decoupling the web framework from global state. For example one thing I want to experiment with is something like https://github.com/adriangb/lazgi but with no global state. That effectively lets the application support whatever creation pattern it wants, without having to add options or logic to the server!

[euri10]

The main footgun is forgetting/failing to teardown your ressources

this one i quite real and even if there are multiple good ways to grab your hand and avoid making those mistakes, like when using pytest with the "right" config, if this promotes better default behavior I'm all in favor of the proposed switch

[Booplicate]

I was looking for this issue, using on_startup is much easier and makes more sense for callbacks you want to "fire and forget" rather than manage as a resource to clean up on exit. I was confused when saw a deprecation warning, but I guess it makes sense if it's hard to support.

[sheecegardezi]

I am moving away from fastapi. I am a bit confused. I wanted to check database connection if they are healthy how do I pass request aka app.state.database reference to a lifecycle function in Starlette? In fastapi I do something like:

async def check_connections(pool):
    while True:
        await asyncio.sleep(600)
        await pool.check()

@asynccontextmanager
async def lifespan(app: FastAPI):
    app.pool = AsyncConnectionPool(
        conninfo=DATABASE_CONNECTION_URL,
        kwargs={
            "row_factory": psycopg.rows.dict_row,
            # "autocommit": True
        },
        open=True
    )
    yield
    await app.pool.close()

@app.on_event("startup")
async def startup_event(request: Request):
    asyncio.create_task(check_connections(request.app.pool))

@app.on_event("shutdown")
async def shutdown_event():
    scheduler.shutdown()

in Starlette instead of app.pool i have app.state.pool

[sheecegardezi]

thanks for the help i got it working! is lifespan like a special keyword? if i combine the code like following it works

@asynccontextmanager
async def lifespan(app):
    print('Startup')
    app.state.pool = AsyncConnectionPool(
        conninfo=DATABASE_CONNECTION_URL,
        kwargs={
            "row_factory": psycopg.rows.dict_row,
        },
        open=True
    )
    asyncio.create_task(check_connections(app.state.pool))
    print('yield')
    yield
    await app.state.pool.close()
    print('Shutdown')

but if i break it into two parts it throws errors:

@asynccontextmanager
async def startup_event(app):
    print(app.state.ADMIN_EMAIL)
    print("startup_event")
    asyncio.create_task(check_connections(app.state.pool))

@asynccontextmanager
async def lifespan(app):
    print('Startup')
    app.state.pool = AsyncConnectionPool(
        conninfo=DATABASE_CONNECTION_URL,
        kwargs={
            "row_factory": psycopg.rows.dict_row,
            # "autocommit": True
        },
        open=True
    )
    yield
    await app.state.pool.close()
    print('Shutdown')

[graingert]

lifespan is not a special keyword - Starlette does not do magic auto-discovery of members of your module, you need to pass it explicitly to Starlette.

your code should look like this:

• pass lifespan=lifespan to Starlette
• do not assign to app.state directly - yield a dictionary
• use open=False for AsyncConnectionPool
• use async with where available
• use an anyio TaskGroup for background tasks
• print shutdown in a finally statement

import contextlib
import anyio

...

@contextlib.asynccontextmanager
async def lifespan(app):
    print("Startup")
    async with (
        AsyncConnectionPool(
            conninfo=DATABASE_CONNECTION_URL,
            kwargs={
                "row_factory": psycopg.rows.dict_row,
            },
            open=False,
        ) as pool,
        anyio.create_task_group() as tg,
    ):
        tg.start_soon(check_connections, pool)
        try:
            yield {"pool": pool}
            tg.cancel_scope.cancel()
        finally:
            print("Shutdown")
...
app = Starlette(routes=routes, lifespan=lifespan)

[Booplicate]

@sheecegardezi fyi, don't just do asyncio.create_task(...), you should save a reference to the task you created, see a warning in the docs: https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task

[sheecegardezi]

Thanks for pointing it out, i think ill add a reference in global state.

[graingert]

Thanks for pointing it out, i think ill add a reference in global state.

You don't need to do this, you can just use an anyio.create_task_group