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

terminator handler #251

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 added this to the 1.0.0-alpha.1 milestone Mar 18, 2022
@tomerd tomerd added 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 ⚠️ needs-major-version-bump For PRs that when merged cause a bump of the major version, ie. x.0.0 -> (x+1).0.0 and removed 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 labels Mar 18, 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 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
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.

Sources/AWSLambdaRuntimeCore/Terminator.swift Outdated Show resolved Hide resolved
Tests/AWSLambdaRuntimeCoreTests/LambdaRuntimeTest.swift Outdated Show resolved Hide resolved
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.

Sources/AWSLambdaRuntimeCore/Terminator.swift Outdated Show resolved Hide resolved
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.

Sources/AWSLambdaRuntimeCore/Terminator.swift Outdated Show resolved Hide resolved
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?

Sources/AWSLambdaRuntimeCore/Terminator.swift Outdated Show resolved Hide resolved
@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?

Sources/AWSLambdaRuntimeCore/Terminator.swift Show resolved Hide resolved
@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
⚠️ needs-major-version-bump For PRs that when merged cause a bump of the major version, ie. x.0.0 -> (x+1).0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants