Skip to content
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

Fix Lambda Error Handling #91

Closed
wants to merge 4 commits into from

Conversation

ajschmidt8
Copy link
Contributor

@ajschmidt8 ajschmidt8 commented Mar 9, 2022

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
Copy link
Contributor Author

ajschmidt8 commented Mar 9, 2022

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
Copy link
Contributor

gr2m commented Mar 9, 2022

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

No I trust you on this one :)

@ajschmidt8
Copy link
Contributor Author

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.

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

awaiting changes as discussed in PR

The `error` field is not valid for an API Gateway response. As noted in the AWS docs below, when an error response contains invalid fields, it will always return a `502` error code with a body of `{"message": "Internal server error"}`. This PR updates the error response fields to contain valid fields and a more sensible response message.

- https://docs.aws.amazon.com/lambda/latest/dg/services-apigateway.html#services-apigateway-errors
@ajschmidt8
Copy link
Contributor Author

@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 ajschmidt8 changed the title Fix error response Fix Lambda error handling Dec 22, 2022
@ajschmidt8 ajschmidt8 changed the title Fix Lambda error handling Fix Lambda Error Handling Dec 22, 2022
@ajschmidt8 ajschmidt8 mentioned this pull request Dec 22, 2022
@ajschmidt8
Copy link
Contributor Author

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

@ajschmidt8
Copy link
Contributor Author

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

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

@ajschmidt8 ajschmidt8 closed this Jan 9, 2023
@ajschmidt8
Copy link
Contributor Author

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
Copy link
Contributor

gr2m commented Jan 9, 2023

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

@ajschmidt8 ajschmidt8 deleted the fix-error-response branch January 9, 2023 23:40
@ajschmidt8
Copy link
Contributor Author

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

@ajschmidt8
Copy link
Contributor Author

#116

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants