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

Improve startup cleanup signals #2096

Closed

Conversation

cecton
Copy link
Contributor

@cecton cecton commented Jul 14, 2017

What do these changes do?

  1. cleanup signal is sent even if startup signal failed
  2. helpers to create and manage background tasks

Are there changes in behavior for the user?

  1. The cleanup hooks will be called more often at the end of the process. You now try to connect to a database in a startup hook and get your client session cleaned automatically in the cleanup even if the database connection failed.
  2. You no longer need to use the dict methods of Application to create tasks to manage background processes, you can use the helper add_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

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the changes folder
    • name it <issue_id>.<type> for example (588.bug)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .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.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@cecton cecton force-pushed the improve-startup-cleanup-signals branch 2 times, most recently from af4283a to db38e46 Compare July 17, 2017 11:05
@cecton
Copy link
Contributor Author

cecton commented Jul 17, 2017

@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?

@cecton
Copy link
Contributor Author

cecton commented Jul 17, 2017

(In 3.4.4 it works already)

@cecton cecton force-pushed the improve-startup-cleanup-signals branch 2 times, most recently from eb976de to 466d286 Compare July 17, 2017 13:33
@codecov-io
Copy link

codecov-io commented Jul 17, 2017

Codecov Report

Merging #2096 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           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
Impacted Files Coverage Δ
aiohttp/web.py 99.65% <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 0d9abc9...ca5fb94. Read the comment docs.

@cecton cecton force-pushed the improve-startup-cleanup-signals branch from 466d286 to 0c1909c Compare July 17, 2017 14:22
@cecton
Copy link
Contributor Author

cecton commented Jul 25, 2017

@asvetlov last one but biggest one. You can maybe cherry pick ca5fb94 separately if the rest doesn't meet your expectations.

@asvetlov
Copy link
Member

Hmm. I definitely don't want app.add_task() and others.
People was reluctant for incorporating aiojobs, your proposal overlaps with the library a little bit.
Could you make a PR for ca5fb94 only?
I can cherry-pick it but prefer well-formed pull requests :)

@cecton cecton force-pushed the improve-startup-cleanup-signals branch from 0c1909c to ca5fb94 Compare July 25, 2017 10:32
@cecton
Copy link
Contributor Author

cecton commented Jul 25, 2017

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

@asvetlov
Copy link
Member

Docs should be updated, sure.
Let's left this task to other PR.

Regarding to this one I think we need mention the behavior in docs too both in Usage and Reference.
After PR applying user should always check if her key is in app dict.

It is a breaking change.
On other hand we definitely don't want to have partially initialized application.
Also it correlates with #2121 a little

Mybe we need a ServerStartUp object with methods for saving all activities: startup signals and created handlers.
The object could be used for proper shutdown procedure.

Now it is a wild idea, let me sleep on it.

@cecton cecton force-pushed the improve-startup-cleanup-signals branch from ca5fb94 to 77d7d57 Compare August 2, 2017 15:02
@cecton
Copy link
Contributor Author

cecton commented Aug 2, 2017

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

@asvetlov
Copy link
Member

asvetlov commented Aug 7, 2017

Better solution proposed

@asvetlov asvetlov closed this Aug 7, 2017
@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants