-
Notifications
You must be signed in to change notification settings - Fork 113
async/await support #186
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
async/await support #186
Conversation
Node.js developers will feel at home :-) But we would need Seems to be in the pipeline :-) |
This might be interesting to us. Potentially replacing the |
d6b3b48
to
3f849a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, this looks great. Left a few comments, mostly minor around overloading / @asyncHandler
etc
/// - closure: `CodableVoidAsyncClosure` based Lambda. | ||
/// | ||
/// - note: This is a blocking operation that will run forever, as its lifecycle is managed by the AWS Lambda Runtime Engine. | ||
public static func run<In: Decodable>(_ closure: @escaping CodableVoidAsyncClosure<In>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overloading on closure arguments is probably not a good idea
|
||
#if compiler(>=5.5) && $AsyncAwait | ||
/// Strongly typed, processing protocol for a Lambda that takes a user defined `In` and returns a user defined `Out` async. | ||
public protocol AsyncLambdaHandler: EventLoopLambdaHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, an AsyncLambdaHandler
isn't running the async code on the EventLoop
at the moment (that's impossible without custom executors), is this proto conformance still right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is. Those protocols build up on top of each other:
- ByteBufferLambdaHandler (just receive ByteBuffer on the EnventLoop)
- EventLoopLambdaHandler (implements encoding and decoding to other types) - still on EventLoop
- AsyncLambdaHandler - implements
EventLoopLambdaHandler
by dispatching of theEventLoop
.
2b6292d
to
b9e39fe
Compare
Package.swift
Outdated
|
||
#if compiler(>=5.5) && $AsyncAwait | ||
package.targets.first(where: { $0.name == "AWSLambdaRuntimeCore" })? | ||
.dependencies.append(.product(name: "_NIOConcurrency", package: "swift-nio")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can always depend on it, _NIOConcurrency
just doesn't export anything if A/A isn't on. But up to you
@@ -78,6 +78,64 @@ internal struct CodableVoidClosureWrapper<In: Decodable>: LambdaHandler { | |||
} | |||
} | |||
|
|||
// MARK: - Async | |||
|
|||
#if compiler(>=5.5) && $AsyncAwait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is good stuff. should we also mark the closure ones deprecated >= 5.5? I would argue we should.
Package.swift
Outdated
@@ -15,7 +15,7 @@ let package = Package( | |||
.library(name: "AWSLambdaTesting", targets: ["AWSLambdaTesting"]), | |||
], | |||
dependencies: [ | |||
.package(url: "https://github.com/apple/swift-nio.git", .upToNextMajor(from: "2.17.0")), | |||
.package(url: "https://github.com/apple/swift-nio.git", .branch("main")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to wait for the next NIO release before we can merge. @weissi ;)
66b8fcc
to
9e32a33
Compare
Co-authored-by: tomer doron <tomerd@apple.com>
@swift-server-bot test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice one! lgtm!
awesome! |
async
/await
is awesome! Let's support this in Lambda.Modifications:
AsyncLambdaHandler
. Will be renamed toLambdaHandler
as soon as we drop the current callback basedLambdaHandler
.AsyncLambdaHandler
is to use@main
to execute it. Don't useLambda.run
for it. We wan't to removeLambda.run
for 1.0.