Skip to content

Fix a bug where multiple returns in lambda handler cause runtime errror#69

Merged
waltjones merged 1 commit into
rollbar:masterfrom
beamsfm:master
Nov 26, 2019
Merged

Fix a bug where multiple returns in lambda handler cause runtime errror#69
waltjones merged 1 commit into
rollbar:masterfrom
beamsfm:master

Conversation

@MatejBalantic

@MatejBalantic MatejBalantic commented Oct 23, 2019

Copy link
Copy Markdown
Contributor

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.LogPanic would be called at the end of each handler, regardless 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.

This fixes #70

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
@MatejBalantic

Copy link
Copy Markdown
Contributor Author

@waltjones any feedback to this?

@waltjones

Copy link
Copy Markdown
Contributor

@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.

@MatejBalantic

Copy link
Copy Markdown
Contributor Author

Sound great, thanks! Just wanted to follow up, but no hurry.

@MatejBalantic

Copy link
Copy Markdown
Contributor Author

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

@waltjones waltjones merged commit 4b2c800 into rollbar:master Nov 26, 2019
@waltjones

Copy link
Copy Markdown
Contributor

@MatejBalantic Looks good. Merged. Thank you!

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.

Bug: LambdaWrapper panics at runtime when panic occurs inside a lambda handler with multiple return values

2 participants