support an asynccontextmanager create_app factory#1152
support an asynccontextmanager create_app factory#1152graingert wants to merge 10 commits intoKludex:mainfrom
Conversation
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
I tested this locally (with a simple example), and it seems to work great!
There is a lot going on in the PR, and this is my first time looking at the internals of the codebase, so I don't fully understand all of the changes. Perhaps one of the Uvicorn maintainers can understand it better, but it might help overall to split this up (if possible).
| "h11>=0.8", | ||
| "typing-extensions;" + env_marker_below_38, | ||
| # contextlib2.nullcontext().__aenter__/__aexit__ | ||
| "contextlib2 >= 21.6.0;" + env_marker_below_310, |
There was a problem hiding this comment.
Is this really required for 3.9? Is the comment highlighting an issue with the implementation in 3.7, 3.8 & 3.9?
There was a problem hiding this comment.
on python3.9 the contextlib.nullcontext class is missing __aenter__ and __aexit__
| if not config.loaded: | ||
| config.load() | ||
|
|
There was a problem hiding this comment.
Could you explain why config loading changes are needed? I'd think all we need is to add a flag, or possibly not even that (since it is easy to inspect a function and check if it is an async generator context manager)
There was a problem hiding this comment.
This is because config's app can't be loaded without a context manager and so now it's always loaded by the time it reaches one of these classes
* handle shutdown if startup fails * exit app_context correctly in run_server test * wait for main_loop task to finish after cancellation
|
Hi, Looking at the broadness of the changes here compared to the "additive" nature of #1151, I think we need to be much more wary about the approach taken here, so as to minimize breaking change potential (in theory, that should be zero for a feature addition). Some example pointers:
I've got more general comments and questions about the problem described in #1151, I'll post those there instead. :-) |
4 participants
fixes #1151