Skip to content
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

support an asynccontextmanager create_app factory #1152

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

graingert
Copy link
Member

fixes #1151

@graingert graingert marked this pull request as ready for review August 14, 2021 21:39
@graingert graingert requested review from euri10 and Kludex August 15, 2021 06:29
@graingert graingert mentioned this pull request Aug 15, 2021
Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


minimal_requirements = [
"asgiref>=3.4.0",
"click>=7.0",
"h11>=0.8",
"typing-extensions;" + env_marker_below_38,
# contextlib2.nullcontext().__aenter__/__aexit__
"contextlib2 >= 21.6.0;" + env_marker_below_310,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really required for 3.9? Is the comment highlighting an issue with the implementation in 3.7, 3.8 & 3.9?

Copy link
Member Author

@graingert graingert Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on python3.9 the contextlib.nullcontext class is missing __aenter__ and __aexit__

Comment on lines -46 to -48
if not config.loaded:
config.load()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@graingert graingert Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@florimondmanca
Copy link
Member

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:

  • config.load() disappearing w/o deprecation is probably a no-no: many people might be depending on it, causing breaking changes when upgrading.
  • This PR changes a bunch of tests that don't seem to require changing. These changes are a strong signal that merging this PR would require users to go and change a bunch of their own code to upgrade Uvicorn, whereas they might not care about the new feature. I'd suggest no existing tests should be changed here (they can be later, if we deem it relevant), and only new tests be added for the new feature.
  • Is the new config.app_context() supposed to be public API? If so, it should probably be documented. If documenting it doesn't make much sense, then it probably means that's an implementation detail that should be hidden from public API in some way.
  • Should there be any change not-strictly-relevant to immediately resolving support @asynccontextmanager app_factory functions #1151 (eg refactoring, shuffling things around), ideally that should go in its own PR.

I've got more general comments and questions about the problem described in #1151, I'll post those there instead. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support @asynccontextmanager app_factory functions
3 participants