search

sam-goodwin / punchcard

Type-safe AWS infrastructure.
Apache License 2.0
507 stars 20 forks source link

add callbackWaitsForEmptyEventLoop on handler wrappers #65

Closed pocesar closed 4 years ago

pocesar commented 4 years ago

I think, since the wrappers are acting as bridges to the real handlers, it's safe to assume context.callbackWaitsForEmptyEventLoop = false, since the code will most likely be using async (event, context) => Promise<any> instead of (event, context, callback) => void

This has bitten me in the past (on unrelated projects), with lambda timing out, even though it had completed. Some issues in the wild: https://www.serverlesslife.com/AWS_Serverless_Common_Mistakes_Communication_with_Other_Systems.html#connections https://github.com/awslabs/aws-serverless-express/issues/134 https://medium.com/@hichaelmart/reviewing-the-aws-lambda-nodejs10-x-runtime-84de1e500aac https://www.jeremydaly.com/reuse-database-connections-aws-lambda/ https://stackoverflow.com/a/41623101/647380

I understand if the desired behavior should default to letting the consumer decide, feel free to disregard, but since the code does async things before eventually calling the underlaying handler, it might catch early cases of errors. gotta say, it's a weird behavior, for sure

sam-goodwin commented 4 years ago

If Promises are used to handle the request, then the event loop should always be empty when we return the result, right? Or do DB connections have issues?

Perhaps we should make this configurable on a per-function basis?

pocesar commented 4 years ago

the issue I see is that when using variables that "hang around" outside of the handler, it might give varying results. on my test code, I'm trying to connect to mongodb atlas from the handler, and I also have external db connection 'cached variables' (as per documentation). this might be true for things that run longer than the handler itself, but databases are a prime example. but you're right, it shouldn't blindly assume that

side note: the handlers don't receive the third argument for the lambda context, would be nice to expose this, so callbackWaitsForEmptyEventLoop can be effectively applied per-function

sam-goodwin commented 4 years ago

You should implement Dependency for your Mongo DB database and use the depends: logic to bootstrap your runtime handler with access to the DB.

Related: We should also support AWS's managed MongoDB database. This would be easy once the CDK implements their L2 constructs for it.