-
-
Notifications
You must be signed in to change notification settings - Fork 749
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
Move Config#load
out of Server#serve
to enable Eager Asynchronous Construction of App
#942
base: master
Are you sure you want to change the base?
Conversation
@MarioIshac Thanks for this PR. I think this makes sense, but just need a bit of careful thinking. How does this behave with Uvicorn multiprocessing? Eg Edit: okay, answering my own question, it seems we're all good. Here's how we spawn for each case (normal, reload, multiple workers): Lines 377 to 386 in c6b80fc
In each case we target |
@florimondmanca I'll recheck the interaction between this PR and uvicorn multiprocessing to make sure I didn't miss anything. I also will take a look at how this PR behaves with apps imported as the object itself ( |
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 think this is looking good.
@euri10, any opinion by chance?
@MarioIshac Would it be possible to add a test for this, eg by passing an app factory (see associated existing tests) that runs something on the event loop?
config = self.config | ||
config.setup_event_loop() | ||
|
||
if not config.loaded: | ||
config.load() |
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.
Let's add a comment about why we're doing this here, perhaps?
config = self.config | |
config.setup_event_loop() | |
if not config.loaded: | |
config.load() | |
# Setup and load app before entering the async environment, so it's possible to | |
# run any async setup on our side or in the app module. | |
# See: https://github.com/encode/uvicorn/issues/941 | |
config = self.config | |
config.setup_event_loop() | |
if not config.loaded: | |
config.load() |
yep, a test for that is a good idea |
I am working on writing the test, but it looks like |
I have kind of the feeling this could be resurrected for 3.10 |
@euri10 I don't think this makes sense anymore, given |
Fixes #941 by identifying the 3 call sites (test utility, gunicorn worker, uvicorn runner itself)
Server#serve
is used at and movingConfig#load
to those sites.