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

allow custom initialization of the HandlerType of the LambdaRuntime #310

Merged
merged 7 commits into from
Jan 18, 2024

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Oct 10, 2023

Motivation: Provide the flexibility for custom initialization of the HandlerType as this will often be required by higher level frameworks.

Modifications:

  • Modify the LambdaRuntime type to accept a closure to provide the handler rather than requiring that it is provided by a static method on the Handler type
  • Update downstream code to use HandlerProvider
  • Update upstream code to support passing Hanlder Type of Handler Provider
  • Add and update tests

Originally suggested and coded by @tachyonics in #308

@tomerd
Copy link
Contributor Author

tomerd commented Nov 1, 2023

@tachyonics wdyt?

@tachyonics
Copy link
Contributor

@tachyonics wdyt?

This will work for our use case although not quite as convenient as what I had. My only concern is that handlerProvider returns a ByteBufferLambdaHandler which has a makeHandler function requirement that will not actually get used. In our code we can just not implement that method but conceptually it seems a bit odd at the runtime level.

@tomerd
Copy link
Contributor Author

tomerd commented Dec 1, 2023

@tachyonics wdyt about 792d1c1? I dont love the name CoreByteBufferLambdaHandler but hopefully this address the need better?

Copy link
Contributor

@tachyonics tachyonics left a comment

Choose a reason for hiding this comment

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

@tomerd I am not really a fan of the name either but otherwise the changes look good and are more consistent.

@tomerd
Copy link
Contributor Author

tomerd commented Dec 1, 2023

thanks @tachyonics, so now to the hard part - choosing a name.

cc @fabianfett @sebsto @yim-lee @ktoso

any good ideas?

@tachyonics
Copy link
Contributor

so now to the hard part - choosing a name

@tomerd NonInitializingByteBufferLambdaHandler?

@tomerd
Copy link
Contributor Author

tomerd commented Dec 7, 2023

@tachyonics updated to NonInitializingByteBufferLambdaHandler. wdyt?

@tachyonics
Copy link
Contributor

Just some small comments about comments. Otherwise lgtm.

@tomerd
Copy link
Contributor Author

tomerd commented Dec 8, 2023

@tachyonics new name idea: LambdaRuntimeHandler. wdyt?

Motivation:

Provide the flexibility for custom initialisation of the HandlerType as this will often be required by higher level frameworks.

Modifications:
* Modify the LambdaRuntime type to accept a closure to provide the handler rather than requiring that it is provided by a static method on the Handler type
* Update downstream code to use HandlerProvider
* Update upstream code to support passing Hanlder Type of Handler Provider
* Add and update tests

Originally suggested and coded by @tachyonics in swift-server#308
@tomerd tomerd force-pushed the feature/handler-provider branch 2 times, most recently from 16ce236 to 178ce55 Compare December 8, 2023 21:21
@tachyonics
Copy link
Contributor

I'm good with this!

@tomerd
Copy link
Contributor Author

tomerd commented Dec 9, 2023

@fabianfett any concerns?

@fabianfett
Copy link
Member

Tbh. I think this doesn't solve the actual issue. The lifetime handling in Lambda was designed and implemented before ServiceLifecycle came about. For this reason, using long running clients in Lambda currently feels absolutely weird.

For this reason, I think, we should reconsider what the Lambda API should look like taking into account that it should play nicely with ServiceLifecycle.

General ideas:

  1. LambdaRuntime should implement Service
  2. If a Lambda runs locally it should set up a ServiceGroup to handle signal handling
  3. If a user needs long running Services within their Lambda they should be able to use ServiceGroups to manage their lifetimes.

@tomerd tomerd merged commit 8d9f44b into swift-server:main Jan 18, 2024
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants