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

set_loop for app check, allow app_factory() #96

Merged
merged 3 commits into from
Jul 1, 2017
Merged

Conversation

samuelcolvin
Copy link
Member

fixes #88.

@asvetlov
Copy link
Member

Would you accept coroutine as app_factory?

@samuelcolvin
Copy link
Member Author

samuelcolvin commented Jun 28, 2017

Easy enough to do.

I'm not sure how that would work when running the code in production. Eg. Gunicorn doesn't accept a coroutine, and if you're using web.run_app(), you'd have to do something ugly with loop.run_until_complete.

Why would a coroutine be useful? surely anything requiring a running loop can be done in startup coroutines?

Perhaps having app_factory as a coroutine could encourage people to setup their app in a way which will be hard to deploy?

Very happy to add the functionality if it's useful.

@codecov
Copy link

codecov bot commented Jun 28, 2017

Codecov Report

Merging #96 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #96      +/-   ##
=========================================
+ Coverage   95.06%   95.1%   +0.03%     
=========================================
  Files          12      12              
  Lines         770     776       +6     
  Branches       92      92              
=========================================
+ Hits          732     738       +6     
  Misses         26      26              
  Partials       12      12
Impacted Files Coverage Δ
aiohttp_devtools/cli.py 100% <ø> (ø) ⬆️
aiohttp_devtools/runserver/serve.py 93.27% <100%> (-0.06%) ⬇️
aiohttp_devtools/runserver/config.py 88.28% <100%> (+0.78%) ⬆️
aiohttp_devtools/version.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70aef40...ac7bb7d. Read the comment docs.

@asvetlov
Copy link
Member

Well, let's discuss.
My main motivation is creating every async object (the object that should cooperate with asyncio loop) inside a coroutine.
It allows to skip explicit event loop passing but get implicit running loop used for coro execution.
Maybe we end up with something like run_forever() from python/asyncio#465
I still don't know how to better integrate this approach with gunicorn.
We could add support for coroutine factory to aiohttp.worker easy though.
And after that got smoothly to dropping ._init_loop() trick at all.

@samuelcolvin
Copy link
Member Author

Let's leave the coroutine question for now. Happy to add it later if it's needed.

@samuelcolvin samuelcolvin merged commit 7ce8a6e into master Jul 1, 2017
@samuelcolvin samuelcolvin deleted the check-loop branch July 1, 2017 10:13
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.

app.loop is not defined during pre-check
2 participants