New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add startup/shutdown dependencies and dependency caching lifespan control #3516
Conversation
@Faylixe @sm-Fifteen, based on your input in the linked issues, I'd appreciate if you could glance over this and give any feedback. Thanks! |
Implementing this requires:
(1) is pretty straightforward.
And if you set any |
Co-authored-by: Elliana May <me@mause.me>
Co-authored-by: Elliana May <me@mause.me>
Co-authored-by: Elliana May <me@mause.me>
I'm not personally sold on having the dependency's lifespan be specified on the caller's end instead of the dependency's end. On one hand, it means any dependency marked as a lifetime dep anywhere causes all of its transitive dependencies to be run and cached as lifetime deps at the same time, which may not be what the user intended. It's also unclear to me what happens if the same function ends up called both as a request-scoped and a lifetime-scoped dependency at different places throughout the application. Would the request-scoped dep short-circuit to using the already available lifetime dep or would it create a separate instance of it? What if the lifetime dep hasn't been initialized yet?
More generally, I'm not sure of the benefit of lazily-loaded shared resources compared to having dependencies context-managed by the lifetime protocol (the main reason I can think of being parametrized dependencies, which weren't the sort of thing I was considering using for lifetime deps in the first place).
I can't speak for most FastAPI users, but the sort of setup I'm using (just a plain reverse proxy for a small API server connected to a bunch of heterogeneous databases) wouldn't fit under that scenario. Really, my main use case for lifetime deps when I opened #617 was to not have active object handles littering the global namespace outside of functions (see #726 (comment), and imagine something like that, but minus the underscored global variables). I figured those could just be wrapped in CM-like functions and annotated for FastAPI to know that those functions, when used as dependencies, are not supposed to be re-run, but to have the eagerly singleton'd result (hopefully a thread-safe result, I don't think that can reliably be checked by FastAPI, so it's up to the user to make sure) returned instead.
It also generally makes sense to me that your server should fail to startup if the database connection your application requires to run is unreachable, or if your core config file can't be found, which is why I would have imagined these as not being lazy, but we might just have different use cases for these. |
Firstly, thank you for the extensive and detailed feedback!
This is very possible, I may have been shortsighted with my use cases, but I'll try to answer some of good questions you bring up
I think this is an important question. To me it boils down to "is an app lifespan dep == to the same dep but with a request lifespan?". Let me know if this interpretation is incorrect. class Config:
...
def get_config() -> Config:
return Config()
@app.get("/")
def root(cfg1 = Depends(get_config, lifetime="app"), cfg2 = Depends(get_config)):
# note that this would only apply to the first call
# subsequent calls would depend on the resolution of the below discussion regarding cache keys
assert cfg1 is cfg2 # option 1, current implementation
assert cfg1 is not cfg2 # option 2, adding the `lifetime` parameter as a cache key element If we add the
The intention is that only the top level (let's call it root) dependency gets marked as a lifetime dependency and cached as such. If a transitive dependency ( class Config:
...
def get_config() -> Config:
return Config()
def lifetime_dep(cfg: Config = Depends(get_config)):
return cfg
def request_dep(cfg: Config = Depends(get_config)):
return cfg
calls = 0
@app.get("/")
def root(cfg1 = Depends(lifetime_dep, lifetime="app"), cfg2 = Depends(request_dep)):
global calls
calls +=1
if calls == 1:
assert cfg1 is cfg2
else:
assert cfg1 is not cfg2 In this case,
I agree, it would be nice to have the opposite (DI for the startup/shutdown system) as well. Like you say, it often makes sense to not even start the application unless the database, etc. are able to connect. But maybe there is a use case for both?
As far as I know, even simple reverse proxies like NGINX have healthcheck functionality. In your use case, how do you know a deploy was successful if you don't have some sort of healthcheck / rediness?
This is not necessarily a solution, but maybe something like the following can be used to clean up things: # in db.py or something
def get_db_connection(cfg: Config = Depends(get_config)) -> Connection:
# do some stuff with cfg to get a database connection
def DBConnection() -> Connection:
return Depends(get_db_connection, lifetime="app")
# in main.py
@app.get("/")
def root(conn: Connection = DBConnection()):
... Now the caller doesn't have to specify |
+1 on this, not a big fan as well.
In that case, if dependency behavior should be defined by the user I would prefer having a factory function handling this: def AppDepends(*args, **kwargs) -> Any:
return Depends(*args, **kwargs, lifespan="app") Which then can be use like a traditional one: @app.get("/")
def root(cfg1 = AppDepends(get_config), cfg2 = Depends(get_config)): Otherwise thanks a lot for working on this, not an easy one and I am really looking forward for this feature <3 |
Yeah, I totally understand on:
The main reason I'm not doing it like that is that:
And so doing it this way allowed me to do it in relatively few lines of code, with no major refactors, while still satisfying all of my needs. I do think there's some use for both, I can see scenarios where it is desirable to lazily initialize lifetime dependencies.
That does seem like a cleaner short term solution. We could also do this in the FastAPI codebase, something similar is already being done for def Depends( # unchanged
dependency: Optional[Callable[..., Any]] = None, *, use_cache: bool = True
) -> Any:
return params.Depends(dependency=dependency, use_cache=use_cache)
def AppDepends(
dependency: Optional[Callable[..., Any]] = None, *, use_cache: bool = True
) -> Any:
return params.Depends(dependency=dependency, use_cache=use_cache, lifetime="app") |
hello! I'd highly recommend using the To support this, Another change about this is that the @contextlib.asynccontextmanager
async def TaskGroup() -> AsyncGenerator[anyio.TaskGroup, None, None]:
async with anyio.create_task_group() as tg:
yield tg # <- this might throw asyncio.CancelledError or trio.CancelledError! |
100% agreed, thank you for the suggestion. Unfortunately there are a couple blockers to this: The first is that FastAPI does not support that yet (in particular, FastAPI's router class doesn't have a public The second bigger problem is that Statlette can't have both |
The startup and shutdown hooks are merely shimmed by the default starlette lifespan context manager (see encode/starlette#799), now. There's nothing stopping FastAPI from adding a custom lifespan handler to take care of lifespan dependencies that also shims the old startup and shutdown events. |
@graingert I pushed a version using Lines 454 to 479 in 629e465
FastAPI(lifespan=...) we'll have to handle adding the user's lifespan into this glue lifespan (I suppose checking if it is a context manager or generator, etc.).
|
Co-authored-by: Thomas Grainger <tagrain@gmail.com>
I think it's worth waiting for the Starlette 0.16.0 upgrade in this pr because so much changed about how lifespan works |
Are there more changes beyond having to wrap the lifespan func in Do you know if FastAPI has a plan / upgrade path for Starlette versions? |
Yeah, that's a good point: there's a decent amount of flexibility built into the combination of Of course, the challenge is making sure that it's clear/explicit how to accomplish a task given the API. I'm just some random guy on the internet--no need to give me too much attention--but, I tend to find, e.g., |
I think you're making a very good point in that I think the nice thing is that because of how generator dependencies and the cache are implemented here (in particular, that the dependency need not be the same object as the value it yields), we have full control over both aspects independently. This got me thinking about all of the options we have that users might want. I'm going to leave aside creation of dependencies since that is governed by when they are first needed (which simplifies understanding of startup/shutdown events: they just move the dependency creation to happen before the first request). Dependency LifespansRequestThis is the current default / only option. AppThe original proposal in this PR. EndpointThe dependency gets destroyed when the endpoint finishes running (i.e. immediately before the request is done processing). Notably, this would allow the dependency to raise None/ BackgroundThe dependency gets destroyed in the background immediately after its value is retrieved. CachingTrue / RequestThis is the current default behavior. FalseThis is current optional behavior AppProposed in this PR. CombinationsI think that by combining these, all major use cases can be covered. Some of these (like ImplementationI did a quick draft implementation and it's not hard to do, it just means keeping around 3 AsyncExitStacks and 2 caches instead of just 1 of each. |
I pushed a series of commits implementing the options proposed in #3516 (comment). I am thinking that startup/shutdown dependencies should also be split out into another PR, which would greatly reduce the diff size, but I'll keep it in here for now since I think it's a strong use case for the other changes. The only one I didn't implement is I'd like to acknowledge @zoliknemet for the excellent implementation in #2697, which I partially borrowed to implement Additionally:
|
fastapi/dependencies/utils.py
Outdated
dependant.request_param_name, | ||
dependant.query_params, | ||
dependant.header_params, | ||
dependant.cookie_params, | ||
dependant.body_params, | ||
dependant.request_param_name, | ||
dependant.websocket_param_name, | ||
dependant.http_connection_param_name, | ||
dependant.response_param_name, | ||
dependant.path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this is a sensible set of exclusions. I dunno what to do with the security dependencies.
Some more doc edits. I've gone with editing the existing examples referenced in #3516 (comment) instead of duplicating them. |
In case any of your are interested, I made a toy repo where I've been prototyping what it would look like to generalize the DI system to be less request/response coupled and to support the features proposed in this PR. Here's an example of how it might integrate into FastAPI internally (the goal is near zero user facing changes): https://github.com/adriangb/di/ I was also able to get some other nice features working:
|
I'm going to close this in favor of discussion in #3641 Also, the library referenced in #3516 (comment) is now available on PyPi: https://pypi.org/project/di/ |
Issues this addresses
To address these issues, this PR provides for more advanced dependency injection concepts.
In particular:
Finally, this PR gives lifespan events (startup/shutdown) the dependency injection.
These changes are presented together for cohesiveness (they aren't very useful individually), but they can easily be split into multiple smaller PRs for easier review.
Here's a small motivating example: