-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Improve startup cleanup signals #2096
Conversation
af4283a
to
db38e46
Compare
@asvetlov I tested on 3.4.6, 3.5.3, 3.6.0 and 3.6.1 and it works. Unfortunately asyncio is a bit different in 3.4.3 and 3.4.2. Do you really want to support those specific versions? |
(In 3.4.4 it works already) |
eb976de
to
466d286
Compare
Codecov Report
@@ Coverage Diff @@
## master #2096 +/- ##
=======================================
Coverage 97.07% 97.07%
=======================================
Files 38 38
Lines 7719 7719
Branches 1346 1346
=======================================
Hits 7493 7493
Misses 102 102
Partials 124 124
Continue to review full report at Codecov.
|
466d286
to
0c1909c
Compare
Hmm. I definitely don't want |
0c1909c
to
ca5fb94
Compare
@asvetlov done! I will check aiojobs later to see if I can replace some code in my projects. Besides I think it's still confusing, the documentation says that you can spawn background processes in that fashion: http://aiohttp.readthedocs.io/en/stable/web.html#background-tasks It mostly works but it is not perfect, there is a little bit more to do to get a background job working, logging and terminating properly. If people find that aiohttp should not handle background jobs at all, then it would make more sense to remove this example of the documentation. If background jobs are okay, then we should either give an example that works perfectly or implement the helpers like I did (my helpers are still 37 lines of code in Application (still smaller than aiojobs). |
Docs should be updated, sure. Regarding to this one I think we need mention the behavior in docs too both in Usage and Reference. It is a breaking change. Mybe we need a Now it is a wild idea, let me sleep on it. |
ca5fb94
to
77d7d57
Compare
@asvetlov I haven't give much thought about this PR yet but I suppose we can conclude it by adding in the documentation that the startup/cleanup behavior is changed. I've just added it to the doc, I think we are good. |
Better solution proposed |
What do these changes do?
helpers to create and manage background tasksAre there changes in behavior for the user?
You no longer need to use the dict methods of Application to create tasks to manage background processes, you can use the helperadd_task
and provide a coroutine. The coroutine will be called with the app argument. When the application finishes, the tasks will be cancelled and awaited properly (except if a task raise SystemExit or any not Exception based exception).Related issue number
#2092
Checklist
CONTRIBUTORS.txt
changes
folder<issue_id>.<type>
for example (588.bug)issue_id
change it to the pr id after creating the pr.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.