Skip to content

Add clarification about GracefulExit when using handle_signals=True#7043

Merged
Dreamsorcerer merged 6 commits intoaio-libs:masterfrom
Daste745:handle-signals-graceful-exit-docs
Sep 11, 2024
Merged

Add clarification about GracefulExit when using handle_signals=True#7043
Dreamsorcerer merged 6 commits intoaio-libs:masterfrom
Daste745:handle-signals-graceful-exit-docs

Conversation

@Daste745
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Dreamsorcerer commented Oct 23, 2022

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
webknjaz
webknjaz approved these changes Oct 26, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

webknjaz commented Oct 26, 2022

Cool, thanks!

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

Daste745 commented Oct 27, 2022

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
Copy Markdown
Member

Dreamsorcerer commented Oct 27, 2022

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
Copy Markdown
Contributor Author

Daste745 commented Oct 27, 2022

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
Copy Markdown
Contributor Author

Daste745 commented Oct 28, 2022

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
webknjaz
webknjaz reviewed Oct 28, 2022
View reviewed changes
webknjaz
webknjaz reviewed Oct 28, 2022
View reviewed changes
webknjaz
webknjaz reviewed Oct 28, 2022
View reviewed changes
@Dreamsorcerer Dreamsorcerer merged commit bee613d into aio-libs:master Sep 11, 2024
@patchback
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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 <[email protected]>
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 <[email protected]>
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

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