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

Adding functionality to catch panics that occur both inside the runtime handler and in the Future it returns #521

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

fermanjj
Copy link
Contributor

@fermanjj fermanjj commented Sep 1, 2022

Description of changes:
Previously, if the lambda runtime handler Future panicked, then this would not be caught. These change add the functionality to catch those panics.

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

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

Sorry it took me a while to review this PR. Thanks for adding a test, it makes things easier to follow. Have you bumped into any situation where this is helpful? I'm wondering if it'd be a good idea to document the panic behavior in general.

@fermanjj
Copy link
Contributor Author

fermanjj commented Sep 7, 2022

Thanks for taking a look! I've added in your suggestion and updated the commit. As far as running into this before, we've only noticed by looking at the implementation that the way panics would be caught wasn't handling panics inside an async block. Having this here is helpful when running concurrent handlers within a single runtime and not having a single thread kill the rest.

@calavera calavera merged commit 1cacdd8 into awslabs:main Sep 7, 2022
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