add initialization context #126
Conversation
motivation: in more complext initialization scearios you may want access to a logger or other utilities changes: * introduce new InitializationContext type that can be extened in the future without breaking the API in semantic-major way * instead of passing in EventLoop to the handler factory, pass in a context that includes a Logger and and EventLoop * fix a bug where we dont hope back to the event loop when coming back from the handler * adjust tests to the new signature
| @@ -57,6 +58,7 @@ 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) | |||
| .hop(to: self.eventLoop) | |||
There was a problem hiding this comment.
@tomerd The only situation in which we would need to hop is if developers bring their own EventLoopGroup. Is that something that is likely?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| extension Lambda { | ||
| /// Lambda runtime initialization context. | ||
| /// The Lambda runtime generates and passes the `InitializationContext` to the Lambda handler as an argument. | ||
| public final class InitializationContext { |
There was a problem hiding this comment.
also considered InitContext but this feels more correct since the function is called initialize
|
cc @adam-fowler |
There was a problem hiding this comment.
Thanks @tomerd for getting this out! The only thing I'm unsure about is if the hop is necessary.
|
@fabianfett one thing I considered was also exposing an allocator in the new context. do you think it would be a good addition or wait-and-see? |
| // 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. |
There was a problem hiding this comment.
just added a comment (it was already hopping) the diff is mostly formatting
@tomerd Yes. I think we should include it. If people need to make some json calls or whatever a allocator comes in handy. |
Co-authored-by: Fabian Fett <[email protected]> Co-authored-by: Yim Lee <[email protected]>
tomerd
force-pushed
the
feature/init-context
branch
from
768f67d to
f21f4a3
Compare
|
motivation: in more complext initialization scearios you may want access to a logger or other utilities
changes: