Skip to content

add initialization context #126

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

Merged
merged 7 commits into from
Jun 16, 2020
Merged
Changes from 1 commit
Commits
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
22 changes: 16 additions & 6 deletions Sources/AWSLambdaRuntimeCore/LambdaRunner.swift
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,19 @@ extension Lambda {
// 1. create the handler from the factory
// 2. report initialization error if one occured
let context = InitializationContext(logger: logger, eventLoop: self.eventLoop)
return factory(context).hop(to: self.eventLoop).peekError { error in
self.runtimeClient.reportInitializationError(logger: logger, error: error).peekError { reportingError in
// We're going to bail out because the init failed, so there's not a lot we can do other than log
// that we couldn't report this error back to the runtime.
logger.error("failed reporting initialization error to lambda runtime engine: \(reportingError)")
return factory(context)
// hopping back to "our" EventLoop is importnant in case the factory returns
// a future that originiated from a different EventLoop
// this can happen if the factory uses a library (lets say a DB client) that manages it's own EventLoops
// for whatever reason and returns a future that originated from that foreign EventLoop.
Copy link
Contributor Author

@tomerd tomerd Jun 13, 2020

Choose a reason for hiding this comment

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

just added a comment (it was already hopping) the diff is mostly formatting

.hop(to: self.eventLoop)
.peekError { error in
self.runtimeClient.reportInitializationError(logger: logger, error: error).peekError { reportingError in
// We're going to bail out because the init failed, so there's not a lot we can do other than log
// that we couldn't report this error back to the runtime.
logger.error("failed reporting initialization error to lambda runtime engine: \(reportingError)")
}
}
}
}

func run(logger: Logger, handler: Handler) -> EventLoopFuture<Void> {
Expand All @@ -58,6 +64,10 @@ extension Lambda {
let context = Context(logger: logger, eventLoop: self.eventLoop, invocation: invocation)
logger.debug("sending invocation to lambda handler \(handler)")
return handler.handle(context: context, event: event)
// hopping back to the "our" EventLoop is importnant in case the handler returns
// a future that originiated from a different EventLoop
// this can happen if the handler uses a library (lets say a DB client) that manages it's own EventLoops
// for whatever reason and returns a future that originated from that foreign EventLoop.
.hop(to: self.eventLoop)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👀

Copy link
Member

Choose a reason for hiding this comment

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

@tomerd The only situation in which we would need to hop is if developers bring their own EventLoopGroup. Is that something that is likely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this can happen is if they use some library (lets say a DB client) that manages it's own ELG for whatever reason and they return a future that originated from that ELG. while this is not a pattern we encourage, we also can't stop anyone from doing it so to be safe we should hop imo

Copy link
Member

Choose a reason for hiding this comment

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

Vapor doesn't do a hop in its routes and quite a number of people hit that issue at some point. After that they know its an issue and don't do it again. There is an argument for education here instead of protecting against. But at the same time a hop onto the same EventLoop is hardly a performance hit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a reference: Where ever NIO takes a future (not that many APIs outside of bootstraps), it'll hop it to the correct event loop.

.mapResult { result in
if case .failure(let error) = result {
Expand Down