Added `shutdown() -> EventLoopFuture<Void>` to the `ByteBufferLambdaHandler` #122

fabianfett · 2020-06-12T08:48:57Z

Motivation

This fixes #121.

Before we also had a small bug in the code:

public func start() -> EventLoopFuture<Void> {
    assert(self.eventLoop.inEventLoop, "Start must be called on the `EventLoop` the `Lifecycle` has been initialized with.")

    logger.info("lambda lifecycle starting with \(self.configuration)")
    self.state = .initializing
    // triggered when the Lambda has finished its last run
    let finishedPromise = self.eventLoop.makePromise(of: Int.self)
    finishedPromise.futureResult.always { _ in
        self.markShutdown()
    }.cascade(to: self.shutdownPromise)
          // ^--- to cascade to the shutdown promise, the `finishedPromise` has to be fulfilled/failed
    var logger = self.logger
    logger[metadataKey: "lifecycleId"] = .string(self.configuration.lifecycle.id)
    let runner = Runner(eventLoop: self.eventLoop, configuration: self.configuration)
    return runner.initialize(logger: logger, factory: self.factory).map { handler in
        self.state = .active(runner, handler)
        self.run(promise: finishedPromise) 
        // ^-- finishedPromise can only be fulfilled, if startup was successful
    }
}

Look out for the // ^-- comments in the code above. If a factory failed before, the Lambda was not shutdown, which ultimately led to a leaked EventLoop and Lifecycle. This pr addresses this issue as well and provides a test for it.

Changes

added syncShutdown() throws to the ByteBufferLambdaHandler protocol and provided a default implementation (empty).
fixed above bug. If the Lambda's factory fails the Lifecycle now will fail the shutdownFuture as well.

ktoso · 2020-06-12T09:10:25Z

Sources/AWSLambdaRuntimeCore/LambdaHandler.swift

@@ -164,6 +164,14 @@ public protocol ByteBufferLambdaHandler {
    /// - Returns: An `EventLoopFuture` to report the result of the Lambda back to the runtime engine.
    ///            The `EventLoopFuture` should be completed with either a response encoded as `ByteBuffer` or an `Error`
    func handle(context: Lambda.Context, event: ByteBuffer) -> EventLoopFuture<ByteBuffer?>
+
+    /// The method to clean up your resources.
+    /// Concrete Lambda handlers implement this method to shutdown their `HTTPClient`s and database connections.


Is it worth adding a mention about if your create/init future failed then this would not be invoked so take special case about any resources that need to be torn down there (as we discussed that case on chat)?

@ktoso fixed below

tomerd · 2020-06-12T17:44:03Z

Sources/AWSLambdaRuntimeCore/LambdaLifecycle.swift

+                        try handler.syncShutdown()
+                    } catch {
+                        logger.error("Error shutting down handler: \(error)")
+                    }


@fabianfett why not surface the handler shutdown error into the finishPromise?

Good idea, we might overwrite a previous error though? Do we want that? wdyt? @tomerd

maybe roll them up to a "ShutdownError" of sorts that contain a collection/array of the underlying errors?

tomerd

looks good, one question

fabianfett · 2020-06-12T17:45:51Z

Sources/AWSLambdaRuntimeCore/LambdaLifecycle.swift

+                    // If the lambda is terminated (e.g. LocalServer shutdown), we make sure
+                    // developers have the chance to cleanup their resources.
+                    do {
+                        try handler.syncShutdown()


@weissi @tomerd We are on the EventLoop at this point. AHCs httpClient.syncShutdown() would be a no go at that point. Where do we want to dispatch to?

I don't think the handler would expect to be called from outside the EventLoop to do the shutdown. I think you need to require the caller to provide an asynchronous shutdown method.

Well, you could also document that it'll be called on another thread but that's maybe a bit weird. AHC's syncShutdown is thread-safe but I don't think we can expect that from everybody.

Also: If shutdown is synchronous, then often the startup is also synchronous, no?

I think you need to require the caller to provide an asynchronous shutdown method.

I tend to agree this fits better with the rest of APIs

@weissi @tomerd @ktoso I addressed that, by making shutdown async. Further I went a little ahead and wrapped the eventLoop as well as the logger into a ShutdownContext. To get in line with #126

lmkwyt

ktoso

Looking good now @fabianfett 👍

weissi · 2020-06-15T17:02:50Z

Sources/AWSLambdaRuntimeCore/LambdaHandler.swift

+    ///
+    /// - Note: In case your Lambda fails while creating your LambdaHandler in the `HandlerFactory`, this method
+    ///         **is not invoked**. In this case you must cleanup the created resources immediately in the `HandlerFactory`.
+    func shutdown(context: Lambda.ShutdownContext) -> EventLoopFuture<Void>


Is this invoked once per lambda invocation?

no. only in debug mode for shutdown. or if the lambda occurs an recoverable error. and the lambda is therefore closed.

@fabianfett ah right. In that case, couldn't we run it synchronously after MTELG.withCallingThreadAsEventLoop returns?

Ah no, doesn't make sense. Shutdown probably needs the EL :)

ignore my comments. This looks good.

tomerd · 2020-06-16T01:27:35Z

Sources/AWSLambdaRuntimeCore/LambdaRunner.swift

@@ -95,7 +95,7 @@ private extension Lambda.Context {
 }

 // TODO: move to nio?
-private extension EventLoopFuture {
+extension EventLoopFuture {


is this change leftover from previous iteration?

no it's needed, since I use mapResult in another file.

Also: Please don't use any access qualifiers (especially not public) in front of extension. It makes code reviews really difficult because a func foo() {} can be public if maybe 100 lines above there's a public extension XYZ. I think all qualifiers should be banned in front of extension

I hope there's some linter for this perhaps? Maybe time to look around for one hm

tomerd · 2020-06-16T01:28:16Z

Sources/AWSLambdaRuntimeCore/LambdaLifecycle.swift

+            .flatMap { (handler, result) -> EventLoopFuture<Int> in
+                let shutdownContext = ShutdownContext(logger: logger, eventLoop: self.eventLoop)
+                return handler.shutdown(context: shutdownContext).recover { error in
+                    logger.error("Error shutting down handler: \(error)")


not a blocker, since this is debug mode only, but ideally the shutdown error would bubble into the shutdownPromise instead of being logged

@tomerd I hope I fixed this with my last push.

weissi

Thanks, LGTM!

fabianfett requested review from tomerd and weissi June 12, 2020 09:02

fabianfett added this to the 0.1.1 milestone Jun 12, 2020

ktoso reviewed Jun 12, 2020

View reviewed changes

fabianfett force-pushed the ff-add-syncShutdown-to-handler branch from 881312b to feb35c7 Compare June 12, 2020 09:27

tomerd reviewed Jun 12, 2020

View reviewed changes

fabianfett commented Jun 12, 2020

View reviewed changes

fabianfett force-pushed the ff-add-syncShutdown-to-handler branch from a2e87b7 to 6488d9a Compare June 15, 2020 08:55

fabianfett changed the title ~~Added syncShutdown() throws to the ByteBufferLambdaHandler~~ Added shutdown() -> EventLoopFuture<Void> to the ByteBufferLambdaHandler Jun 15, 2020

fabianfett force-pushed the ff-add-syncShutdown-to-handler branch 2 times, most recently from b7cc3af to 23688d5 Compare June 15, 2020 09:01

ktoso approved these changes Jun 15, 2020

View reviewed changes

weissi reviewed Jun 15, 2020

View reviewed changes

tomerd reviewed Jun 16, 2020

View reviewed changes

fabianfett force-pushed the ff-add-syncShutdown-to-handler branch 4 times, most recently from a772032 to 35fb3ab Compare June 17, 2020 09:25

weissi approved these changes Jun 17, 2020

View reviewed changes

fabianfett force-pushed the ff-add-syncShutdown-to-handler branch 2 times, most recently from b779861 to 6188493 Compare June 17, 2020 14:23

fabianfett added 3 commits June 17, 2020 16:24

Added syncShutdown() throws to the ByteBufferLambdaHandler

8199dbf

Updated to use a ShutdownContext

1629c5a

Review comments addressed.

ed7d57a

fabianfett force-pushed the ff-add-syncShutdown-to-handler branch from 6188493 to ed7d57a Compare June 17, 2020 14:24

tomerd merged commit 437a60d into swift-server:master Jun 17, 2020

fabianfett deleted the ff-add-syncShutdown-to-handler branch June 29, 2020 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `shutdown() -> EventLoopFuture<Void>` to the `ByteBufferLambdaHandler` #122

Added `shutdown() -> EventLoopFuture<Void>` to the `ByteBufferLambdaHandler` #122

fabianfett commented Jun 12, 2020 •

edited

Loading

ktoso Jun 12, 2020

fabianfett Jun 12, 2020

ktoso Jun 15, 2020

tomerd Jun 12, 2020

fabianfett Jun 12, 2020

tomerd Jun 12, 2020

tomerd left a comment

fabianfett Jun 12, 2020

weissi Jun 12, 2020

weissi Jun 12, 2020

tomerd Jun 12, 2020

fabianfett Jun 15, 2020

ktoso left a comment

weissi Jun 15, 2020

fabianfett Jun 15, 2020

weissi Jun 17, 2020

weissi Jun 17, 2020

weissi Jun 17, 2020

tomerd Jun 16, 2020

fabianfett Jun 17, 2020

weissi Jun 17, 2020

ktoso Jun 29, 2020

tomerd Jun 16, 2020 •

edited

Loading

fabianfett Jun 17, 2020

weissi left a comment

Added shutdown() -> EventLoopFuture<Void> to the ByteBufferLambdaHandler #122

Added shutdown() -> EventLoopFuture<Void> to the ByteBufferLambdaHandler #122

Conversation

fabianfett commented Jun 12, 2020 • edited Loading

Motivation

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

Added `shutdown() -> EventLoopFuture<Void>` to the `ByteBufferLambdaHandler` #122

Added `shutdown() -> EventLoopFuture<Void>` to the `ByteBufferLambdaHandler` #122

fabianfett commented Jun 12, 2020 •

edited

Loading

tomerd Jun 16, 2020 •

edited

Loading