switch panic handling method #319

rasviitanen · 2021-04-20T07:27:26Z

Issue: #310

Description of changes:
Uses std::panic::catch_unwind instead of spawning a new tokio task to catch any panics.
By using std::panic::AssertUnwindSafe we don't have to add any new bounds on the handler, so this should be backwards compatible.

Using std::panic::AssertUnwindSafe should be OK. This is essentially what tokio did under the hood with the previous implementation, so the behavior should be equivalent. See:
https://github.com/tokio-rs/tokio/blob/b42f21ec3e212ace25331d0c13889a45769e6006/tokio/src/runtime/task/harness.rs#L409

What this attempts to solve:
Get rid of the Send + Sync + 'static bounds.

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.

rimutaka · 2021-04-20T11:19:35Z

lambda-runtime/examples/shared_resource.rs

@@ -0,0 +1,63 @@
+// This example requires the following input to succeed:


This example demonstrates use of shared resources such as DB connections or local caches that can be initialized at the start of the runtime and reused by subsequent lambda handler calls. Run it with the following input:

Very good comment, thank you! Fixed in the latest push

coltonweaver

Great contribution, thank you! LGTM

coltonweaver · 2021-04-20T19:43:45Z

Added @bahildebrand since he's been more involved with this discussion too.

rimutaka · 2021-04-20T21:24:47Z

Can we not rush this PR? Even if it is backward compatible it is a major change in the expected behavior with some unexpected results in edge cases.

catch_unwind does not guarantee all panics will be caught
AssertUnwindSafe is not bullet-proof either

Even if the compiler guarantees memory safety of panics it does not statically guarantee that all panics will be safely unwound. Correct me if I'm wrong here.

I fully support this change, but would like to give it more time to understand all the implications.

coltonweaver · 2021-04-20T22:24:01Z

Can we not rush this PR? Even if it is backward compatible it is a major change in the expected behavior with some unexpected results in edge cases.
1. catch_unwind does not guarantee all panics will be caught

2. AssertUnwindSafe is not bullet-proof either
Even if the compiler guarantees memory safety of panics it does not statically guarantee that all panics will be safely unwound. Correct me if I'm wrong here.

I fully support this change, but would like to give it more time to understand all the implications.

Intention definitely wasn't to rush this PR, which is why I asked Blake to also take a look :). Completely agree with your points, my desire is to get more eyes on it in case something has been missed.

bahildebrand · 2021-04-22T02:23:59Z

Can we not rush this PR? Even if it is backward compatible it is a major change in the expected behavior with some unexpected results in edge cases.
1. catch_unwind does not guarantee all panics will be caught

2. AssertUnwindSafe is not bullet-proof either
Even if the compiler guarantees memory safety of panics it does not statically guarantee that all panics will be safely unwound. Correct me if I'm wrong here.

I fully support this change, but would like to give it more time to understand all the implications.

My reading of [catch_unwind(https://doc.rust-lang.org/std/panic/fn.catch_unwind.html) is that we would be guaranteed to catch any panics that were inherent to the types composing the handler since it requires an UnwindSafe implementation. I believe the only things that are !UnwindSafe are mutable references, which is not something that would make a ton of sense to use in this context.

One danger would be if someone were enable abort panics on their binary. I'm not sure if the compiler will get angry since we have a call to catch_unwind, or if it will allow someone to call an abort panic from within their handler. That being said I believe the behavior would be equivalent to spawn. I think this is kind of a foot gun that has existed from before this change.

Overall I don't see any problems with this change. It might be worthwhile to add a note that users should not compile this library panic abort.

rasviitanen · 2021-04-22T11:59:32Z

re:

I believe the only things that are !UnwindSafe are mutable references, which is not something that would make a ton of sense to use in this context.

I think there are some other things that would make sense in this context that are !UnwindSafe. For example std::sync::Once or std::io::Error. That said, a regular Rust thread isn't really Unwindsafe either, I don't see why we would need it here.

About panic = "abort": The compiler will not complain and it will allow someone to abort on panic if they want to. Would it be a problem?

Also: Do we want to catch panics in the created future as well? I.e. if it panics here:
https://github.com/awslabs/aws-lambda-rust-runtime/pull/319/files#diff-66445b7321d3a64a01b04af7edeb065318728d4a137b6795f7121d61ef744dd9R61-R62

LucioFranco · 2021-04-28T16:11:28Z

lambda-runtime/examples/shared_resource.rs

+    name: &'static str,
+}
+
+impl TryFrom<&'static str> for SharedClient {


Why does this need to be tryfrom if its infallible?

The initial though was to avoid consumers of this library (or future contributors) to not run into the idea where: "Hey why is this a From, you can just do let client = &SharedClient("name"); directly". The problem with this is that client in this case would be static, so it wouldn't be as clear that you can use some non-static reference. Whereas TryFrom would make people think one extra step and realize that you can do some more advanced setup.

But the example in its current form is indeed confusing.
The better solution here is to simply implement a new function for the SharedClient and force it to be bound like that instead. Then I think the barrier to understand the example will be much lower.

I'll work on a fix, thanks!

LucioFranco · 2021-04-28T16:12:20Z

lambda-runtime/src/lib.rs

            let request_id = &ctx.request_id.clone();
-            #[allow(clippy::async_yields_async)]
-            let task = tokio::spawn(async move { handler.call(body, ctx) });
+            let task = panic::catch_unwind(panic::AssertUnwindSafe(|| handler.call(body, ctx)));


Why this over set_hook?

The runtime needs to report any panics to the invocation error endpoint: https://docs.aws.amazon.com/lambda/latest/dg/runtimes-api.html.

(I'm assuming that the set_hook you're referring to would be set by the customer?)

bahildebrand

LGTM. I'm going to leave this open for another day in case anyone else has comments they'd like to get in. Otherwise great job @rasviitanen. This is an awesome contribution!

seanpianka · 2021-05-22T00:04:22Z

Hi @bahildebrand, what is the timeline for merging this PR?

bahildebrand · 2021-05-22T16:20:23Z

Merging this now. Rust fmt is broken, but I'll push up another PR to fix it.

rasviitanen force-pushed the remove-static branch from 170fca4 to 82068db Compare April 20, 2021 07:30

This was referenced Apr 20, 2021

Remove static ConfiguraAB/aws-lambda-rust-runtime#1

Closed

avoid static lifetime requirement for handler functions #310

Closed

rimutaka reviewed Apr 20, 2021

View reviewed changes

rasviitanen force-pushed the remove-static branch 2 times, most recently from 0a6f22a to 493ee5f Compare April 20, 2021 19:32

coltonweaver approved these changes Apr 20, 2021

View reviewed changes

coltonweaver requested a review from bahildebrand April 20, 2021 19:43

LucioFranco reviewed Apr 28, 2021

View reviewed changes

rasviitanen force-pushed the remove-static branch from 493ee5f to d6a5a59 Compare May 7, 2021 12:46

switch panic handling method

01368ec

rasviitanen force-pushed the remove-static branch from d6a5a59 to 01368ec Compare May 7, 2021 12:48

bahildebrand approved these changes May 12, 2021

View reviewed changes

coltonweaver approved these changes May 13, 2021

View reviewed changes

bahildebrand merged commit 53397cd into awslabs:master May 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

switch panic handling method #319

switch panic handling method #319

Uh oh!

rasviitanen commented Apr 20, 2021

Uh oh!

rimutaka Apr 20, 2021

Uh oh!

rasviitanen Apr 20, 2021

Uh oh!

coltonweaver left a comment

Uh oh!

coltonweaver commented Apr 20, 2021

Uh oh!

rimutaka commented Apr 20, 2021

Uh oh!

coltonweaver commented Apr 20, 2021

Uh oh!

bahildebrand commented Apr 22, 2021

Uh oh!

rasviitanen commented Apr 22, 2021 •

edited

Loading

Uh oh!

LucioFranco Apr 28, 2021

Uh oh!

rasviitanen Apr 29, 2021 •

edited

Loading

Uh oh!

LucioFranco Apr 28, 2021

Uh oh!

davidbarsky Apr 28, 2021

Uh oh!

bahildebrand left a comment

Uh oh!

seanpianka commented May 22, 2021

Uh oh!

bahildebrand commented May 22, 2021

Uh oh!

Uh oh!

		@@ -0,0 +1,63 @@
		// This example requires the following input to succeed:

switch panic handling method #319

switch panic handling method #319

Uh oh!

Conversation

rasviitanen commented Apr 20, 2021

Uh oh!

rimutaka Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

rasviitanen Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

coltonweaver left a comment

Choose a reason for hiding this comment

Uh oh!

coltonweaver commented Apr 20, 2021

Uh oh!

rimutaka commented Apr 20, 2021

Uh oh!

coltonweaver commented Apr 20, 2021

Uh oh!

bahildebrand commented Apr 22, 2021

Uh oh!

rasviitanen commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LucioFranco Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

rasviitanen Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucioFranco Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

davidbarsky Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

bahildebrand left a comment

Choose a reason for hiding this comment

Uh oh!

seanpianka commented May 22, 2021

Uh oh!

bahildebrand commented May 22, 2021

Uh oh!

Uh oh!

rasviitanen commented Apr 22, 2021 •

edited

Loading

rasviitanen Apr 29, 2021 •

edited

Loading