probot / adapter-aws-lambda-serverless

An extension for running Probot on Lambda
ISC License
94 stars 36 forks source link

Fix Lambda Error Handling #91

Closed ajschmidt8 closed 1 year ago

ajschmidt8 commented 2 years ago

Using a try/catch block prevents Lambda function errors from being properly interpreted by AWS.

Removing this block will maintain the existing functionality, but allow Probot errors to be properly propagated to AWS Lambda.

ajschmidt8 commented 2 years ago

Actually, I have some additional thoughts/questions on this. @gr2m, can we remove the try/catch block in this file? Here's my reasoning:

According to the docs linked in my original post:

API Gateway treats all invocation and function errors as internal errors. If the Lambda API rejects the invocation request, API Gateway returns a 500 error code. If the function runs but returns an error, or returns a response in the wrong format, API Gateway returns a 502. In both cases, the body of the response from API Gateway is {"message": "Internal server error"}.

When the serverless.yaml file has a value of async: false (the default value), the try/catch works fine because API Gateway waits for either the 200 or 500 response that is returned by the Lambda function and returns that to the client.

However, when a value of async: true is used, where API Gateway does not wait for a response from and instead returns a 200 response to the client immediately, the try/catch block makes it hard to track down errors (as in #78) since the Lambda function does not currently throw any errors. So in this case, we have both an API Gateway response returning a 200 and also the Lambda function not throwing any errors despite errors having actually occurred.

The impact of removing the try/catch block would be this:

When async: false (the default), if an error occurs, the Lambda will correctly throw an error, and API Gateway will return a 502 response with a JSON response of {"message": "Internal server error"}.

When async: true, if an error occurs, the Lambda will correctly throw an error, but API Gateway will still continue to return a 200 response as expected.

Hopefully that all makes sense. I can elaborate more if necessary.

gr2m commented 2 years ago

Hopefully that all makes sense. I can elaborate more if necessary.

No I trust you on this one :)

ajschmidt8 commented 2 years ago

No I trust you on this one :)

Awesome! This all sounds good in theory, but I'd like to test it locally before pushing those changes to this PR. I will try to test later today and update this PR with my findings.

ajschmidt8 commented 1 year ago

@gr2m, we should be all set to merge this. I just pushed some changes and tested them locally.

The tables below show how Probot errors are interpreted by the respective AWS services on the master branch vs. my fix-error-response PR branch when async is true or false.

It was the Lambda column that was messed up prior to these changes.

master Branch

- Lambda API Gateway
async: false does not report errors does report errors
async: true does not report errors does not report errors

fix-error-response Branch

- Lambda API Gateway
async: false does report errors does report errors
async: true does report errors does not report errors
ajschmidt8 commented 1 year ago

@gr2m, any chance we can get this, #111, and #112 merged today?

ajschmidt8 commented 1 year ago

@gr2m, any chance we can get this, #111, and #112 merged today?

@gr2m, just checking in. can these PRs be merged?

ajschmidt8 commented 1 year ago

I closed this to prevent the deletion from getting merged.

These changes were included as a part of #111 (see my note in the PR body)

gr2m commented 1 year ago

sweet, thank you! I was just confused about the merge conflicts, now it makes sense

ajschmidt8 commented 1 year ago

@gr2m I'll open a PR to fix the style issues that are failing.

ajschmidt8 commented 1 year ago

https://github.com/probot/adapter-aws-lambda-serverless/pull/116