Skip to content

terminator handler #251

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

Merged
merged 5 commits into from
Apr 13, 2022
Merged

terminator handler #251

merged 5 commits into from
Apr 13, 2022

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Mar 17, 2022

motivation: make it simpler to register shutdown hooks

changes:

  • introduce Terminator helper that allow registering and de-registaring shutdown handlers
  • expose the new terminator handler on the InitializationContext and deprecate ShutdownContext
  • deprecate the Handler::shutdown protocol requirement
  • update the runtime code to use the new terminator instead of calling shutdown on the handler
  • add and adjust tests

@tomerd tomerd changed the title terminator handler [wip] terminator handler Mar 17, 2022
@tomerd
Copy link
Contributor Author

tomerd commented Mar 17, 2022

@fabianfett this is still wip but wanted to get an initial feedback on the direction / approach before investing more serious time into this

@tomerd tomerd force-pushed the refactor/terminator branch from 720911b to dee3570 Compare March 18, 2022 01:07
@tomerd tomerd added this to the 1.0.0-alpha.1 milestone Mar 18, 2022
@tomerd tomerd added 🆕 semver/minor Adds new public API. ⚠️ semver/major Breaks existing public API. and removed 🆕 semver/minor Adds new public API. labels Mar 18, 2022
@tomerd tomerd force-pushed the refactor/terminator branch from dee3570 to 0819fa3 Compare March 19, 2022 20:17
motivation: make it simpler to register shutdown hooks

changes:
* introduce Terminator helper that allow registering and de-registaring shutdown handlers
* expose the new terminator hanler on the InitializationContext and deprecate ShutdownContext
* deprecate the Handler::shutdown protocol requirment
* update the runtime code to use the new terminator instead of calling shutdown on the handler
* add and adjust tests
@tomerd tomerd force-pushed the refactor/terminator branch from 0819fa3 to 8f1c4ff Compare March 19, 2022 20:41
@tomerd tomerd requested review from fabianfett and yim-lee March 19, 2022 20:48
Copy link
Contributor

@yim-lee yim-lee left a comment

Choose a reason for hiding this comment

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

Overall looks reasonable. Got one question about the tests.

Co-authored-by: Yim Lee <yim_lee@apple.com>
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

This is much better, than the ShutdownHandler method. Couple of questions/nits.

Comment on lines 18 to 21
extension Lambda {
/// Lambda terminator.
/// Utility to manage the lambda shutdown sequence.
public final class Terminator {
Copy link
Member

Choose a reason for hiding this comment

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

I think the Terminator should be Sendable for 5.6 and onwards.

private var index: [String]
private var map: [String: (name: String, handler: Terminator.Handler)]

public init() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public init() {
init() {

Copy link
Member

Choose a reason for hiding this comment

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

Was this missed?

@tomerd tomerd changed the title [wip] terminator handler terminator handler Apr 12, 2022
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Two small nits, left, that shouldn't stop us from merging.

private var index: [String]
private var map: [String: (name: String, handler: Terminator.Handler)]

public init() {
Copy link
Member

Choose a reason for hiding this comment

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

Was this missed?

@tomerd tomerd merged commit 4d0bba4 into swift-server:main Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ semver/major Breaks existing public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants