Fix a bug where multiple returns in lambda handler cause runtime errror #69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Firstly this fixes a bug where if lambda handler has multiple return types (such as `(string, error)`) and something inside lambda handler panicked, an unexpected runtime error would occur with message "*reflect: wrong return count from function created by MakeFunc*". That's because the panic was being swallowed by the wrapper, and return values weren't set because the child handler never completed (or returned). Thus, wrapper returned with the wrong number of return values (zero), triggering this runtime issue. Secondly I am changing the behaviour of lambda wrapper so that the panic insider the handler is no longer swallowed, but rather bubbled back up. It's [perfectly valid strategy][1] to panic inside the lambda handler, and if that occurs, error should be bubbled up to AWS SDK (after it's reported to Rollbar) so that the response to the lambda request can be correctly indicated as failed, and it can get reported to CloudWatch. Thirdly, this fixes a potentially problematic bug where `c.LogPanic` would be called at the end of each handler, regardles of whether or not there was a panic. It's currently not an issue as `LogPanic` is a NO-OP when `err` is nil, but there's no guarantee for that in the future. [1]: https://docs.aws.amazon.com/lambda/latest/dg/go-programming-model-errors.html#go-errors-panic
|
@waltjones any feedback to this? |
|
@MatejBalantic Visual review looks good. I like to test hands on before signing off, and haven't got to it yet. I should be able to get to it this week. |
|
Sound great, thanks! Just wanted to follow up, but no hurry. |
|
Hi @waltjones, just a kind ping to have a look at this. I'm holding off using rollbar in our product, because the out-of-the-box solution is crashing on me, and I would really want my fix peer-reviewed before shipping it into my product. Thanks for your help |
|
@MatejBalantic Looks good. Merged. Thank you! |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Firstly this fixes a bug where if lambda handler has multiple return values (such as
(string, error)) and something inside lambda handler panicked, an unexpected runtime error would occur with a message "reflect: wrong return count from function created by MakeFunc". That's because the panic was being swallowed by the wrapper, and return values weren't set because the child handler never completed (or returned). Thus, wrapper returned with the wrong number of return values (zero), triggering this runtime issue.Secondly, I am changing the behavior of lambda wrapper so that the panic insider the handler is no longer swallowed, but rather bubbled back up. It's perfectly valid strategy to panic inside the lambda handler, and if that occurs, error should be bubbled up to AWS SDK (after it's reported to Rollbar) so that the response to the lambda request can be correctly indicated as failed, and it can get reported to CloudWatch.
Thirdly, this fixes a potentially problematic bug where
c.LogPanicwould be called at the end of each handler, regardless of whether or not there was a panic. It's currently not an issue asLogPanicis a NO-OP whenerris nil, but there's no guarantee for that in the future.This fixes #70