Skip to content

[DNM][RFC] alternative api #18

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[DNM][RFC] alternative api #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ let package = Package(
.package(url: "https://github.com/swift-server/swift-backtrace.git", from: "1.1.0"),
],
targets: [
.target(name: "SwiftAwsLambda", dependencies: ["Logging", "Backtrace", "NIOHTTP1"]),
.target(name: "SwiftAwsLambda", dependencies: ["Logging", "Backtrace", "NIOHTTP1", "NIOFoundationCompat"]),
.testTarget(name: "SwiftAwsLambdaTests", dependencies: ["SwiftAwsLambda"]),
// samples
.target(name: "SwiftAwsLambdaSample", dependencies: ["SwiftAwsLambda"]),
Expand Down
45 changes: 26 additions & 19 deletions Sources/SwiftAwsLambda/Lambda+Codable.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
//===----------------------------------------------------------------------===//

import Foundation // for JSON
import NIO
import NIOFoundationCompat

/// Extension to the `Lambda` companion to enable execution of Lambdas that take and return `Codable` payloads.
/// This is the most common way to use this library in AWS Lambda, since its JSON based.
Expand All @@ -32,21 +34,18 @@ extension Lambda {
}

// for testing
internal static func run<In: Decodable, Out: Encodable>(configuration: Configuration = .init(), closure: @escaping LambdaCodableClosure<In, Out>) -> LambdaLifecycleResult {
internal static func run<In: Decodable, Out: Encodable>(configuration: Configuration = .init(), closure: @escaping LambdaCodableClosure<In, Out>) -> Result<Int, Error> {
return self.run(handler: LambdaClosureWrapper(closure), configuration: configuration)
}

// for testing
internal static func run<Handler>(handler: Handler, configuration: Configuration = .init()) -> LambdaLifecycleResult where Handler: LambdaCodableHandler {
internal static func run<Handler>(handler: Handler, configuration: Configuration = .init()) -> Result<Int, Error> where Handler: LambdaCodableHandler {
return self.run(handler: handler as LambdaHandler, configuration: configuration)
}
}

/// A result type for a Lambda that returns a generic `Out`, having `Out` extend `Encodable`.
public typealias LambdaCodableResult<Out> = Result<Out, Error>

/// A callback for a Lambda that returns a `LambdaCodableResult<Out>` result type, having `Out` extend `Encodable`.
public typealias LambdaCodableCallback<Out> = (LambdaCodableResult<Out>) -> Void
/// A callback for a Lambda that returns a `Result<Out, Error>` result type, having `Out` extend `Encodable`.
public typealias LambdaCodableCallback<Out> = (Result<Out, Error>) -> Void

/// A processing closure for a Lambda that takes an `In` and returns an `Out` via `LambdaCodableCallback<Out>` asynchronously,
/// having `In` and `Out` extending `Decodable` and `Encodable` respectively.
Expand All @@ -73,27 +72,27 @@ public extension LambdaCodableHandler {
/// LambdaCodableCodec is an abstract/empty implementation for codec which does `Encodable` -> `[UInt8]` encoding and `[UInt8]` -> `Decodable' decoding.
// TODO: would be nicer to use a protocol instead of this "abstract class", but generics get in the way
public class LambdaCodableCodec<In: Decodable, Out: Encodable> {
func encode(_: Out) -> Result<[UInt8], Error> { fatalError("not implmented") }
func decode(_: [UInt8]) -> Result<In, Error> { fatalError("not implmented") }
func encode(_: Out) -> Result<ByteBuffer, Error> { fatalError("not implmented") }
func decode(_: ByteBuffer) -> Result<In, Error> { fatalError("not implmented") }
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity why do we use Result here and not plain old throws?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mostly my personal dislike of try/catch blocks

}

/// Default implementation of `Encodable` -> `[UInt8]` encoding and `[UInt8]` -> `Decodable' decoding
public extension LambdaCodableHandler {
func handle(context: LambdaContext, payload: [UInt8], callback: @escaping (LambdaResult) -> Void) {
func handle(context: LambdaContext, payload: ByteBuffer, promise: EventLoopPromise<ByteBuffer>) {
switch self.codec.decode(payload) {
case .failure(let error):
return callback(.failure(Errors.requestDecoding(error)))
return promise.fail(Errors.requestDecoding(error))
case .success(let payloadAsCodable):
self.handle(context: context, payload: payloadAsCodable) { result in
Copy link
Member

Choose a reason for hiding this comment

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

For every non EventLoopPromise based callback I'd build an offloaded sync/async version.

switch result {
case .failure(let error):
return callback(.failure(error))
return promise.fail(error)
case .success(let encodable):
switch self.codec.encode(encodable) {
case .failure(let error):
return callback(.failure(Errors.responseEncoding(error)))
case .success(let codableAsBytes):
return callback(.success(codableAsBytes))
return promise.fail(Errors.responseEncoding(error))
case .success(let buffer):
return promise.succeed(buffer)
}
}
}
Expand All @@ -107,18 +106,25 @@ public extension LambdaCodableHandler {
private final class LambdaCodableJsonCodec<In: Decodable, Out: Encodable>: LambdaCodableCodec<In, Out> {
private let encoder = JSONEncoder()
private let decoder = JSONDecoder()
private let allocator = ByteBufferAllocator()

public override func encode(_ value: Out) -> Result<[UInt8], Error> {
public override func encode(_ value: Out) -> Result<ByteBuffer, Error> {
do {
return .success(try [UInt8](self.encoder.encode(value)))
let data = try self.encoder.encode(value)
var buffer = self.allocator.buffer(capacity: data.count)
buffer.writeBytes(data)
return .success(buffer)
} catch {
return .failure(error)
}
}

public override func decode(_ data: [UInt8]) -> Result<In, Error> {
public override func decode(_ buffer: ByteBuffer) -> Result<In, Error> {
do {
return .success(try self.decoder.decode(In.self, from: Data(data)))
guard let data = buffer.getData(at: buffer.readerIndex, length: buffer.readableBytes) else {
throw Errors.invalidBuffer
}
return .success(try self.decoder.decode(In.self, from: data))
Copy link
Member

Choose a reason for hiding this comment

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

You import NIOFoundationCompat that's why you should just be able to:

return .success(try self.decoder.decode(In.self, from: buffer))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iirc that returns an optional, so we would end up with same # loc

Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid if something is invalid it throws, otherwise returns a nonoptional.

https://github.com/apple/swift-nio/blob/ffb196f5441b2720c847a2170677f7ba468e030b/Sources/NIOFoundationCompat/Codable%2BByteBuffer.swift#L108

} catch {
return .failure(error)
}
Expand All @@ -141,4 +147,5 @@ private struct LambdaClosureWrapper<In: Decodable, Out: Encodable>: LambdaCodabl
private enum Errors: Error {
case responseEncoding(Error)
case requestDecoding(Error)
case invalidBuffer
}
30 changes: 19 additions & 11 deletions Sources/SwiftAwsLambda/Lambda+String.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
//
//===----------------------------------------------------------------------===//

import NIO
Copy link
Member

Choose a reason for hiding this comment

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

Do we maybe want to remove this file for now? When playing around with the handler syntax, this is just a shore to fix. Could be added back later, without any problems.


/// Extension to the `Lambda` companion to enable execution of Lambdas that take and return `String` payloads.
extension Lambda {
/// Run a Lambda defined by implementing the `LambdaStringClosure` protocol.
Expand All @@ -29,21 +31,18 @@ extension Lambda {
}

// for testing
internal static func run(configuration: Configuration = .init(), _ closure: @escaping LambdaStringClosure) -> LambdaLifecycleResult {
internal static func run(configuration: Configuration = .init(), _ closure: @escaping LambdaStringClosure) -> Result<Int, Error> {
return self.run(handler: LambdaClosureWrapper(closure), configuration: configuration)
}

// for testing
internal static func run(handler: LambdaStringHandler, configuration: Configuration = .init()) -> LambdaLifecycleResult {
internal static func run(handler: LambdaStringHandler, configuration: Configuration = .init()) -> Result<Int, Error> {
return self.run(handler: handler as LambdaHandler, configuration: configuration)
}
}

/// A result type for a Lambda that returns a `String`.
public typealias LambdaStringResult = Result<String, Error>

/// A callback for a Lambda that returns a `LambdaStringResult` result type.
public typealias LambdaStringCallback = (LambdaStringResult) -> Void
/// A callback for a Lambda that returns a `Result<String, Error>` result type.
public typealias LambdaStringCallback = (Result<String, Error>) -> Void

/// A processing closure for a Lambda that takes a `String` and returns a `LambdaStringResult` via `LambdaStringCallback` asynchronously.
public typealias LambdaStringClosure = (LambdaContext, String, LambdaStringCallback) -> Void
Expand All @@ -55,13 +54,18 @@ public protocol LambdaStringHandler: LambdaHandler {

/// Default implementation of `String` -> `[UInt8]` encoding and `[UInt8]` -> `String' decoding
public extension LambdaStringHandler {
func handle(context: LambdaContext, payload: [UInt8], callback: @escaping LambdaCallback) {
self.handle(context: context, payload: String(decoding: payload, as: UTF8.self)) { result in
func handle(context: LambdaContext, payload: ByteBuffer, promise: EventLoopPromise<ByteBuffer>) {
guard let payload = payload.getString(at: payload.readerIndex, length: payload.readableBytes) else {
return promise.fail(Errors.invalidBuffer)
}
self.handle(context: context, payload: payload) { result in
switch result {
case .success(let string):
return callback(.success([UInt8](string.utf8)))
var buffer = context.allocator.buffer(capacity: string.utf8.count)
buffer.writeString(string)
return promise.succeed(buffer)
case .failure(let error):
return callback(.failure(error))
return promise.fail(error)
}
}
}
Expand All @@ -77,3 +81,7 @@ private struct LambdaClosureWrapper: LambdaStringHandler {
self.closure(context, payload, callback)
}
}

private enum Errors: Error {
case invalidBuffer
}
50 changes: 27 additions & 23 deletions Sources/SwiftAwsLambda/Lambda.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,14 @@ public enum Lambda {
// for testing and internal use
@usableFromInline
@discardableResult
internal static func run(configuration: Configuration = .init(), closure: @escaping LambdaClosure) -> LambdaLifecycleResult {
internal static func run(configuration: Configuration = .init(), closure: @escaping LambdaClosure) -> Result<Int, Error> {
return self.run(handler: LambdaClosureWrapper(closure), configuration: configuration)
}

// for testing and internal use
@usableFromInline
@discardableResult
internal static func run(handler: LambdaHandler, configuration: Configuration = .init()) -> LambdaLifecycleResult {
internal static func run(handler: LambdaHandler, configuration: Configuration = .init()) -> Result<Int, Error> {
do {
let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1) // only need one thread, will improve performance
defer { try! eventLoopGroup.syncShutdownGracefully() }
Expand Down Expand Up @@ -252,32 +252,20 @@ public enum Lambda {
}
}

/// A result type for a Lambda that returns a `[UInt8]`.
public typealias LambdaResult = Result<[UInt8], Error>

public typealias LambdaCallback = (LambdaResult) -> Void
/// A callback for a Lambda that returns a `Result<[UInt8], Error>`.
public typealias LambdaCallback = (Result<[UInt8], Error>) -> Void

/// A processing closure for a Lambda that takes a `[UInt8]` and returns a `LambdaResult` result type asynchronously.
public typealias LambdaClosure = (LambdaContext, [UInt8], LambdaCallback) -> Void

/// A result type for a Lambda initialization.
public typealias LambdaInitResult = Result<Void, Error>

/// A callback to provide the result of Lambda initialization.
public typealias LambdaInitCallBack = (LambdaInitResult) -> Void

/// A processing protocol for a Lambda that takes a `[UInt8]` and returns a `LambdaResult` result type asynchronously.
public protocol LambdaHandler {
/// Initializes the `LambdaHandler`.
func initialize(callback: @escaping LambdaInitCallBack)
func handle(context: LambdaContext, payload: [UInt8], callback: @escaping LambdaCallback)
func handle(context: LambdaContext, payload: ByteBuffer, promise: EventLoopPromise<ByteBuffer>)
Copy link
Member

@fabianfett fabianfett Mar 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

As stated in #11 (comment) the returning ByteBuffer should be optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

}

extension LambdaHandler {
@inlinable
public func initialize(callback: @escaping LambdaInitCallBack) {
callback(.success(()))
}
public protocol InitializableLambdaHandler {
/// Initializes the `LambdaHandler`.
func initialize(promise: EventLoopPromise<Void>)
Copy link
Member

Choose a reason for hiding this comment

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

I can totally see the idea of using the initialize(promise: EventLoopPromise<Void>) method to report errors during startup as discussed in #11 and I think the intention is very good.

I'm not sure though if it would be better to go all in and make the initialize a real init?

For example we could set it up like this

public protocol InitializableLambdaHandler {
     @inlinable		     /// Initializes the `LambdaHandler`.
     func init(promise: EventLoopPromise<Void>, eventLoopGroup: EventLoopGroup) throws
}

I can see that this looks kind of ugly injecting the EventLoopPromise for reporting async errors and having throws for immediate errors that prevent one from setting up the Handler object/struct. But thinking about usecases I can imagine having clean mains.

Lambda.run(MyLambdaHandler.self)

struct MyLambdaHandler: InitializableLambdaHandler, LambdaHandler {
  let httpClient: AsyncHTTPClient
  init(promise: EventLoopPromise<Void>, eventLoopGroup: EventLoopGroup) throws {
    self.httpClient = AsyncHTTPClient(.shared(eventLoopGroup))
    // other setup with eventLoopGroup: database clients, aws sdk, whatevar.

    // in context of aws especially important is getting keys from secret store, 
    // which is why the promise might be important
    promise.succeed(Void())
  }

  func handle() {
    ...
  }
}

Getting access to the eventLoopGroup created by the Lambda could be a huge benefit, especially for creating clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on one hand I like the idea of init instead of a separate initialize, on the other hand this would not be a normal ctor as you would be expected to compete the promise, so would be strange API either way.

I also dont think we need to make it a throwable. if you want to return an error catch it and fail the promise

Copy link
Member

@fabianfett fabianfett Mar 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know if it would compile though. ;)

struct MyLambdaHandler: InitializableLambdaHandler, LambdaHandler {
  let httpClient: AsyncHTTPClient
  init(promise: EventLoopPromise<Void>, eventLoopGroup: EventLoopGroup) {
    if somethingUnexpected {
      return // not everything is initialized here
    }
    self.httpClient = AsyncHTTPClient()
}

If we want to report an error during bootup, we could also offer a static function. Then we don't have to do the init dance.

Lambda.reportBootupError(_: error)

Copy link
Contributor Author

@tomerd tomerd Mar 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

another alternative is to define an optional blocking initializer and that's it, so users won't be able to to any significant work in it but the signature can be simplified to:

init(eventLoop: EventLoop) throws

but this closes the window on doing things like getting one-time-initialization-type-data from some database or upstream service, which could be handy

or another option is to have both: aninit(eventLoop: EventLoop) throws where you can do things like setup you http-client, and also abootstrap(eventLoop: EventLoop, promise: Promise<Void>) for the getting one-time-initialization-type-data-from-some-database-or-upstream-service use case.

both could/should be optional. maybe that's the best?

Copy link
Member

Choose a reason for hiding this comment

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

We should insert the complete EventLoopGroup. In Lambdas with more than 1500mb ram, one gets access to two cores. Developers should be allowed to use them ;)

I don't know if we can make an init(eventLoop: EventLoop) throws optional.

}

public struct LambdaContext {
Expand All @@ -289,6 +277,8 @@ public struct LambdaContext {
public let clientContext: String?
public let deadline: String?
// utliity
public let eventLoop: EventLoop
public let allocator: ByteBufferAllocator
Copy link
Member

Choose a reason for hiding this comment

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

+1 for having the ByteBufferAllocator in the context as well.

public let logger: Logger

public init(requestId: String,
Expand All @@ -297,13 +287,17 @@ public struct LambdaContext {
cognitoIdentity: String? = nil,
clientContext: String? = nil,
deadline: String? = nil,
eventLoop: EventLoop,
logger: Logger) {
self.requestId = requestId
self.traceId = traceId
self.invokedFunctionArn = invokedFunctionArn
self.cognitoIdentity = cognitoIdentity
self.clientContext = clientContext
self.deadline = deadline
// utility
self.eventLoop = eventLoop
self.allocator = ByteBufferAllocator()
// mutate logger with context
var logger = logger
logger[metadataKey: "awsRequestId"] = .string(requestId)
Expand All @@ -314,9 +308,6 @@ public struct LambdaContext {
}
}

@usableFromInline
internal typealias LambdaLifecycleResult = Result<Int, Error>

private struct LambdaClosureWrapper: LambdaHandler {
private let closure: LambdaClosure
init(_ closure: @escaping LambdaClosure) {
Expand All @@ -326,4 +317,17 @@ private struct LambdaClosureWrapper: LambdaHandler {
func handle(context: LambdaContext, payload: [UInt8], callback: @escaping LambdaCallback) {
self.closure(context, payload, callback)
}

func handle(context: LambdaContext, payload: ByteBuffer, promise: EventLoopPromise<ByteBuffer>) {
self.closure(context, payload.getBytes(at: payload.readerIndex, length: payload.readableBytes) ?? []) { result in
switch result {
case .success(let bytes):
var buffer = context.allocator.buffer(capacity: bytes.count)
buffer.writeBytes(bytes)
promise.succeed(buffer)
case .failure(let error):
promise.fail(error)
}
}
}
}
Loading