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.

Sign up for GitHub

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

Jump to bottom

Add clarification about GracefulExit when using handle_signals=True #7043

Merged
merged 6 commits into from
Sep 11, 2024
Merged

Add clarification about GracefulExit when using handle_signals=True #7043

merged 6 commits into from
Sep 11, 2024

Conversation

Daste745
Copy link
Contributor

@Daste745 Daste745 commented Oct 23, 2022 •
edited by Dreamsorcerer
Loading

What do these changes do?

Clarify that GracefulExit needs to be handled manually when using handle_signals=True in AppRunner and ServerRunner.

Are there changes in behavior for the user?

N/A

Related issue number

Fixes #4414

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.bugfix)
    • 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."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 23, 2022
@Daste745
Copy link
Contributor Author

Daste745 commented Oct 23, 2022 •
edited
Loading

The issue #4414 is quite old, but I think this still needed addressing. I added a short mention about the need to handle GracefulExit signals in the handle_signals param description. If this is not enough, I could expand it to a 'Note' or 'Warning' section.

@Dreamsorcerer
Copy link
Member

I'll let @webknjaz look over it, as it was his issue.

@codecov

This comment was marked as off-topic.

Sign in to view
webknjaz
webknjaz reviewed Oct 26, 2022
View reviewed changes
CHANGES/4414.doc Outdated Show resolved Hide resolved
webknjaz
webknjaz approved these changes Oct 26, 2022
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Other than the necessary change note improvements, this LGTM.

@webknjaz
Copy link
Member

Cool, thanks!

@webknjaz webknjaz enabled auto-merge (squash) October 26, 2022 17:18
@Daste745
Copy link
Contributor Author

We've got some CI errors, probably because GracefulExit doesn't have any section in the docs and the linter can't create a reference. I don't see an obvious place to put more information about GracefulExit, as it's only used for this one purpose.

@Dreamsorcerer
Copy link
Member

Hmm, should it be added to the reference section then? Previously, it was considered a private inner working of the library, and not meant to be used by a user. But, if we are now expecting users to catch the exception, then maybe it needs to be documented as part of the public API now..

@Daste745
Copy link
Contributor Author

I agree. I'll try to find a place to document the GracefulExit API somewhere in the docs.

auto-merge was automatically disabled October 28, 2022 22:21

Head branch was pushed to by a user without write access

@Daste745
Copy link
Contributor Author

I added it under the same section that AppRunner and ServerRunner live. I'm not sure if it's the best spot though.

webknjaz
webknjaz reviewed Oct 28, 2022
View reviewed changes
docs/web_reference.rst Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Oct 28, 2022
View reviewed changes
docs/web_reference.rst Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Oct 28, 2022
View reviewed changes
docs/web_reference.rst Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Oct 28, 2022
View reviewed changes
docs/web_reference.rst Outdated Show resolved Hide resolved
@webknjaz webknjaz added the backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot label Jan 28, 2024
@Dreamsorcerer Dreamsorcerer added the backport-3.11 Trigger automatic backporting to the 3.11 release branch by Patchback robot label Sep 11, 2024
@Dreamsorcerer Dreamsorcerer merged commit bee613d into aio-libs:master Sep 11, 2024
34 of 35 checks passed
@patchback Patchback
Copy link
Contributor

patchback bot commented Sep 11, 2024 •
edited
Loading

Backport to 3.10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.10/bee613d090cab3b7c00a83604668181961b562ff/pr-7043

Backported as #9122

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Sep 11, 2024
@Daste745 @patchback
Add clarification about GracefulExit when using `handle_signals=Tru…
50cc1df 
…e` (#7043)

(cherry picked from commit bee613d)
@patchback patchback bot mentioned this pull request Sep 11, 2024
Merged
5 tasks
@patchback Patchback
Copy link
Contributor

patchback bot commented Sep 11, 2024 •
edited
Loading

Backport to 3.11: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.11/bee613d090cab3b7c00a83604668181961b562ff/pr-7043

Backported as #9123

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Sep 11, 2024
@Daste745 @patchback
Add clarification about GracefulExit when using `handle_signals=Tru…
e213b5f 
…e` (#7043)

(cherry picked from commit bee613d)
@patchback patchback bot mentioned this pull request Sep 11, 2024
Merged
5 tasks
Dreamsorcerer pushed a commit that referenced this pull request Sep 11, 2024
@patchback @Daste745
[PR #7043/bee613d0 backport][3.10] Add clarification about `GracefulE…
fb6726f 
…xit` when using `handle_signals=True` (#9122)

**This is a backport of PR #7043 as merged into master
(bee613d).**

Co-authored-by: Daste <stefankar1000@gmail.com>
Dreamsorcerer pushed a commit that referenced this pull request Sep 11, 2024
@patchback @Daste745
[PR #7043/bee613d0 backport][3.11] Add clarification about `GracefulE…
8596dc2 
…xit` when using `handle_signals=True` (#9123)

**This is a backport of PR #7043 as merged into master
(bee613d).**

Co-authored-by: Daste <stefankar1000@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webknjaz webknjaz webknjaz approved these changes

@Dreamsorcerer Dreamsorcerer Dreamsorcerer approved these changes

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees

@webknjaz webknjaz

@Daste745 Daste745

Labels
backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot backport-3.11 Trigger automatic backporting to the 3.11 release branch by Patchback robot bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document the need to catch GracefulExit if handle_signals=True
4 participants
@Daste745 @Dreamsorcerer @webknjaz @bdraco