[Do not merge] Refactor to address RFCs #63

sapessi · 2018-12-27T21:30:31Z

This is a major refactor of the crate structure to address the comments in RFCs #53. While the changes are major, they should not be breaking for existing code.

Description of changes: Moved all of the "main loop" logic in a runtime-core crate that defines a Vec<u8>-based handler. The runtime crate now simply wraps the bytes handler into a typed handler method using serde.

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.

…ve ErrorReponse directly

…e to handle any error type

…ration to the new() function on the ErrorResponse

…+ Send + Sync + Debug. Updated samples to reflect this

…nes a Vec<u8> based handler

davidbarsky · 2018-12-28T02:19:19Z

lambda-runtime-client/src/error.rs

@@ -135,6 +153,8 @@ impl Error for ApiError {
        None
    }
 }
+unsafe impl Send for ApiError {}


Hmm, why's this is necessary? would be useful to add a comment.

This will go away, with the next iteration all errors will switch to failure. I have quite a bit of legacy error handling code in here. Just wanted to make one big change at a time.

davidbarsky · 2018-12-28T02:24:29Z

lambda-runtime-core/src/error.rs

+    ///
+    /// # Returns
+    /// A new `RuntimeError` instance with the `recoverable` property set to `false`.
+    pub(crate) fn unrecoverable(msg: &str) -> RuntimeError {


This could be late, but a potential option would be to have a struct named RuntimeError, several convenience methods, and an internal enum called RuntimeErrorKind that has several variants that include whether or not this an irrecoverable error.

While I'm questioning assumptions in this crate, what do we consider to be an irrevocable error? Is it just an error that wasn't caught and handled?

Same comment as above.

See my comment above.

davidbarsky · 2018-12-28T02:37:26Z

lambda-runtime-core/src/error.rs

+/// The `RuntimeError` object is returned by the custom runtime as it polls
+/// for new events and tries to execute the handler function. The error
+/// is primarily used by other methods within this crate and should not be relevant
+/// to developers building Lambda functions. Handlers are expected to return


should not be relevant to developers building Lambda functions.

I'd maybe phrase this along the lines of:

This error is primarily used by the Lambda runtime itself. It not expected that Lambda function implementations will need to construct or handle this error.

As I wrote that, I realized that maybe RuntimeError can be pub(crate). Thoughts?

Agree, no need to expose this. I will make the change as I work on the errors refactor.

Wanted to followup—should these remain public?

lambda-runtime-core/src/handler.rs

davidbarsky · 2018-12-28T02:46:17Z

lambda-runtime-core/src/runtime.rs

@@ -252,7 +194,7 @@ where
                                    "Error for {} is not recoverable, sending fail_init signal and panicking",
                                    request_id
                                );
-                                self.runtime_client.fail_init(&e);
+                                self.runtime_client.fail_init(&ErrorResponse::from(Box::new(e)));


Perhaps I'm missing something, but why does this need to be boxed first?

Same as above - haven't touched errors at all

lambda-runtime-core/src/runtime.rs

lambda-runtime/Cargo.toml

lambda-runtime/examples/custom_error.rs

lambda-runtime/examples/custom_error_failure.rs

davidbarsky · 2018-12-28T03:19:07Z

lambda-runtime/src/lib.rs

+/// defined in the `lambda_runtime_core` crate. The closure simply uses `serde_json`
+/// to serialize and deserialize the incoming event from a `Vec<u8>` and the output
+/// to a `Vec<u8>`.
+fn wrap<E, O>(mut h: impl Handler<E, O>) -> impl FnMut(Vec<u8>, Context) -> Result<Vec<u8>, error::HandlerError>


I like this!

…or type

… runtime core now imports a generated function to collect the runtime info

davidbarsky

These changes look good to me.

…ted correctly

…trace out of failure :(

lambda-runtime-client/Cargo.toml

davidbarsky · 2019-01-21T22:06:26Z

lambda-runtime-client/src/client.rs

    runtime_agent: String,
+    host: String,


Why the addition of host and change from endpoint to next_endpoint?

next_endpoint is the pre-parsed uri for the /next endpoint, which remains stable throughout the lifetime of the client. The host is used to re-generate the Uri for the response endpoints for each request. I'm caching the next_endpoint Uri value to avoid parsing it each time.

lambda-runtime-client/src/error.rs

softprops

Posted a handful of comments. Notably I'm wondering if this change could been broken down into digestable chunks (pulls). This felt like a lot to absorb in one pull.

softprops · 2019-01-21T20:19:48Z

lambda-runtime-client/src/error.rs

-
-        err
-    }
+    /*#[test] skip this tests as it's broken right now


I aways feel better about using git branches to manage unfinished code. Does this test provide ant value in it's commented out form? If it's a signal I'd suggesting using #[ignore] https://doc.rust-lang.org/1.5.0/book/testing.html#the-ignore-attribute

I removed this as per @davidbarsky's suggestion. We will re-introduce this once we finally figure out what is going on with backtraces in failure... #!@?!!&

yeah, it might've been an older commit.

lambda-runtime-client/Cargo.toml

softprops · 2019-01-21T22:02:31Z

lambda-runtime/src/lib.rs

+}
+
+/// Initializes the Lambda runtime with the given handler. Optionally this macro can
+/// also receive an instance of Tokio runtime for the Hyper HTTP client. If the Tokio


I feel like it would be more useful for readers of these api docs to see something like "receive a customized instance of Tokio runtime to drive internal lambda operations to completion". The mention of Hyper HTTP client is kind of an implementation detail that's not useful for readers.

Like it. Made the change

I think this might've been addressed partially in #63 (comment).

softprops · 2019-01-21T22:07:52Z

lambda-runtime/src/lib.rs

+impl<Function, Event, Output, EventError> Handler<Event, Output, EventError> for Function
+where
+    Function: FnMut(Event, Context) -> Result<Output, EventError>,
+    EventError: Fail + LambdaErrorExt + Display + Send + Sync,


Few notes here.

Because Fail requires Display + Send + Sync. Adding Display + Send + Sync on top of Fail is redundant.

I'll have to read through a bit more to understand the purpose of LambdaErrorExt but my initial impression is this. The core interface should make the trivial easy and the complex possible. If a dependency on Fail wasn't enough implementing LambdaErrorExt may seem like a tall order for making the simple trivial. Thoughts?

The reason why we have Display, Send, and Sync there is that I don't trust the failure crate to maintain those in the Fail trait. This library has burned me more than once recently (scorched me to death is more appropriate). Since in the next iteration of the library we plan to make the core async we wanted to introduce a constraint of our own in this, even if failure removes it.

Lambda gives us the ability to return an error type field which we plan to use to specify the error's root cause. The LambdaErrorExt trait defines the error_type(&self) -> &str to provide this. In the crate we have included implementations of the trait for almost all errors in the standard library as well as failure types. With these default implementations, I'm hoping to still make the trivial easy (+1 on this turn of phrase by the way, very nicely put).

The reason why we have Display, Send, and Sync there is that I don't trust the failure crate to maintain those in the Fail trait. This library has burned me more than once recently (scorched me to death is more appropriate). Since in the next iteration of the library we plan to make the core async we wanted to introduce a constraint of our own in this, even if failure removes it.

You're not wrong to distrust Failure given your experience, but the way the Failure is evolving is to just provide a derive for std::error::Error + Display + Send + Sync bounds. Additionally, the standard library encourages implementing Debug + Display on errors, and I can think of a single error in the standard library (std::sync::PoisonError) that doesn't implement Send/Sync. Most errors I've seen implement Send/Sync, and if they don't, that's often grounds for opening a PR/issue.

(With regard to std::sync::PoisonError—I believe that most people just panic on a poisoned mutex. I'll be shocked if a Lambda customer has to worry about poisoned mutexes.

Im thinking then may it's a good idea to put the Failure dependency behind a feature flag to maintain a more stable API. Scorch free!

Responded in another comment too (perhaps we should switch to the main PR thread for this conversation). With the current implementation, we don't force the failure crate or the Fail trait on developers building Lambda functions, their errors become Fail for us automatically because we include failure. This gives us the ability to switch away from failure in the future without making a breaking change.

lambda-runtime-client/src/error.rs

lambda-runtime-errors-derive/Cargo.toml

lambda-runtime-errors/errorgen.py

lambda-runtime-errors/src/error_ext_impl.rs

lambda-runtime-errors/src/lib.rs

softprops · 2019-01-21T22:57:35Z

lambda-runtime/examples/custom_error.rs

+}
+// the value return by the error_type function is included as the
+// `errorType` in the AWS Lambda response
+impl LambdaErrorExt for CustomError {


I can foresee this getting very onerous. Not only for those familiar with rust but for those new to rust dabbling through experimenting transitioning a lambda to rust. Is there any thing the runtime could provide that could provide a default impl that allows for specialization when needed? This goes back to making the trivial easy and the complex possible.

Something I'm debating—not really brought up with Stefano—is to point people to just use Failure and be done with it, but I'm not convinced that's the right approach.

Failure sounds like a same middle ground as it plays nice with the ecosystem what already exists. The complexity seems to come from an implementation detail but if feels like too much weight to on users which won't benefit as much/appreciate the feature of the named errors inside lambda responses. It's likely user logging errors to cloudwatch logs will provide the same debugging benefit at no extra interface cost.

That said do we gain anything from failure over Box beside the back trace? Std Errors are really the least common denominator and work well with ? Chaining

Failure sounds like a same middle ground as it plays nice with the ecosystem what already exists. The complexity seems to come from an implementation detail but if feels like too much weight to on users which won't benefit as much/appreciate the feature of the named errors inside lambda responses. It's likely user logging errors to cloudwatch logs will provide the same debugging benefit at no extra interface cost.

I keep going back and forth on this, but the InvocationError endpoint does expect an ErrorType. I wonder if we should have a top-level error handler—required that a customer implement this—that handles such reporting. The issue with a Display impl, iirc, is that the Display won't give an accurate error report.

The other thing is that I have opinions as to how things should be, but Stefano works with customers who use Lambda, and he sees how people use.

That said do we gain anything from failure over Box beside the back trace? Std Errors are really the least common denominator and work well with ? Chaining

Easier derives + From impls. It's really a nicer experience, IMO.

I'm a customer as well!

That's why I had included the error derive crate, to allow developers to just:

#[derive(LambdaErrorExt)] struct BasicCustomError;

Do you feel we need more? What should our generic be for the trait implementation be, any Fail?

@softprops

I'm a customer as well!

That's true, sorry! I just remember him telling me that people are creating Dashboards on specific error strings.

One more thing. We implemented errors this way to avoid forcing the failure crate on users of the runtime - it's purely a utility for us. When they take the runtime as a dependency their errors automatically become fails because we bring the failure crate. Once the standard library errors include back traces we can remove failure from the runtime implementation without it being a breaking change

lambda-runtime-client/src/error.rs

lambda-runtime-client/Cargo.toml

davidbarsky · 2019-01-21T23:45:59Z

lambda-runtime-client/src/error.rs

+
+    #[test]
+    fn does_not_produce_stack_trace() {
+        env::remove_var("RUST_BACKTRACE");


Backtraces... are kinda tricky to test. I'm not sure of a better way, though.

lambda-runtime-core/build.rs

davidbarsky · 2019-01-21T23:49:20Z

lambda-runtime-core/src/error.rs

+/// The `RuntimeError` object is returned by the custom runtime as it polls
+/// for new events and tries to execute the handler function. The error
+/// is primarily used by other methods within this crate and should not be relevant
+/// to developers building Lambda functions. Handlers are expected to return


Wanted to followup—should these remain public?

lambda-runtime-errors/src/lib.rs

davidbarsky · 2019-01-22T00:12:04Z

lambda-runtime-errors/src/lib.rs

+
+pub use crate::error_ext_impl::*;
+
+use failure::{format_err, Compat, Error, Fail};


I think Failure can be placed behind a feature flag that's on by default. Do you disagree?

Crowbar did that with error_chain. It's actually a nice feature of rust that fits the 0 cost tagline. Only pay for what you use, and make it possible to opt out of what you dont

See my answer in the other comments - why put it behind a feature flag if it has no impact on developers using the library? They do not have to use failure, it's just a utility for us.

The feature flag allows you conditionally disable compilation for libraries that you're not using.

I think we should use Failure internally, but customers shouldn't need to pay the compilation cost if they're not using Failure in their application. The conditional compilation/feature flag might make some the conversions easier.

davidbarsky · 2019-01-22T00:13:42Z

lambda-runtime/examples/custom_error.rs

+}
+// the value return by the error_type function is included as the
+// `errorType` in the AWS Lambda response
+impl LambdaErrorExt for CustomError {


Something I'm debating—not really brought up with Stefano—is to point people to just use Failure and be done with it, but I'm not convinced that's the right approach.

lambda-runtime/examples/custom_error_failure.rs

sapessi · 2019-01-22T02:56:04Z

Posted a handful of comments. Notably I'm wondering if this change could been broken down into digestable chunks (pulls). This felt like a lot to absorb in one pull.

Apologies for the huge number of changes. We worked on bite-sized chunks with @davidbarsky as he reviewed each commit during development. The nature of the changes (refactor) made them hard to publish them in separate PRs without potentially breaking stuff for anyone depending on master. Hopefully it won't (need to) happen again.

softprops · 2019-01-22T03:29:18Z

lambda-runtime-errors-derive/src/lib.rs

+    }*/
+
+    for v in s.variants() {
+        println!("Variant: {}", v.ast().ident);


whoopsie - some careful debugging there on my part.

The derive crate is very much a work in progress. Few more things I'd like to do.

sapessi · 2019-01-23T16:27:37Z

Moving the failure conversation to a thread here to make it more manageable.

I spoke with @davidbarsky this morning. The dependency on failure is something we decided to take because it makes our life easier when accepting any kind of error and also allows us to collect backtraces. Because of how the Fail trait works, we do not need developers to include failure as a dependency in their functions. Their errors - as long as they implemented Error from the standard library - automatically implement Fail because we include failure.

Both David and I agree that this is a temporary dependency that we will remove in the future, once backtraces are available in the standard library. Since we do not require failure to be included in functions the change should be non-breaking.

Other than compilation time, and on the basis that we plan to remove failure in the future once backtraces are stable in the standard library, do you have any other concern with releasing 0.2 with failure @softprops? Unless we have a serious issue with the dependency, I'd rather not block the release, it's been a long time coming.

softprops · 2019-01-23T23:44:31Z

No concerns here. I didn't realize all you need is std Error. That sounds great. I'd love a release. I've got a growing number of serverless framework templates that depend on git urls for the http lambda module. I'd love to update those to a release

sapessi · 2019-01-23T23:47:00Z

Huzza! I'll merge and push 0.2 tomorrow. The http crate will go out as a 0.1. I'll probably hold off on the errors_derive crate until I've had time to clean it up properly.

sapessi added 19 commits December 2, 2018 15:50

Fixed typo for UNHANDLED error constant

a77abb1

Removed RuntimeApiError trait form the client and changed it to recei…

36bcc8f

…ve ErrorReponse directly

Changed HandlerError type to alias Box<dyn Error> to allow the runtim…

f53bd30

…e to handle any error type

Added custom error examples and updated existing examples

8b6d8fc

Fixed failure example

2e034d2

Updated comment on HandlerError type

4f20464

Exported HandlerError in root of the crate

d17b7c8

Added error code to response structure and centralized backtrace gene…

08772e9

…ration to the new() function on the ErrorResponse

Updated example in lib docs

a6d2c5b

Temporary fix for failure example

fee02e8

Switched to HandlerError struct that can be created from any Display …

4969624

…+ Send + Sync + Debug. Updated samples to reflect this

Addressed code review changes

0bce533

Changed error handling codes according to latest specs clarified in #23

bd3cb34

Removed custom_attribute feature to fix build

5b9bad9

Cleaned up lifetimes/borrows

9ed9a43

New runtime core crate that implements the runtime main loop and defi…

7fa1167

…nes a Vec<u8> based handler

Slimmed down runtime crate that only wraps the runtime core crate

5165242

Rebased on master

3bcb150

Moved HandlerError to error module for backward compatibility

ab54a2e

sapessi requested a review from davidbarsky December 28, 2018 00:32

Fixed error package in examples

99b1328

davidbarsky suggested changes Dec 28, 2018

View reviewed changes

sapessi added 8 commits December 27, 2018 20:10

Addressing CR on newline at the end of file and import grouping

3844b43

More style changes from code review

fa2a5b1

Fixed tests, including http crate

b6423f7

First refactor of errors in the client crate and new errors crate

77459b5

Bump dependencies

ecbae1c

New derive crate (experimental) for LambdaErrorExt and new HandlerErr…

6896b1b

…or type

Switched back to ApiError and added new is_recoverable() method

3f79fcb

Parametrized error type and updated examples

1b1e624

sapessi added 2 commits January 4, 2019 14:15

Added build script to generate the metadata file

bedbf92

Changed the runtime client SDK to receive a user-agent as init param.…

e3f6b34

… runtime core now imports a generated function to collect the runtime info

davidbarsky approved these changes Jan 6, 2019

View reviewed changes

sapessi mentioned this pull request Jan 7, 2019

Handlers can be async #68

Closed

Fixed format of build.rs

253a481

sapessi mentioned this pull request Jan 7, 2019

[RFC] Error handling in Rust runtime #54

Closed

sapessi added 2 commits January 9, 2019 19:32

Fixed critical issue in runtime client - request URIs were not genera…

db7f994

…ted correctly

Fixed code issue - committed to fast earlier

f0de223

sapessi mentioned this pull request Jan 10, 2019

Binary output/response? #69

Closed

sapessi added 4 commits January 21, 2019 10:31

New error tests and failure featurs in Cargo. Still cannot get a back…

d705def

…trace out of failure :(

failure features import

a3aaedb

Added doc comment on lambda! macro

fd35180

merge latest master changes

8827cdb

davidbarsky reviewed Jan 21, 2019

View reviewed changes

sapessi added 2 commits January 21, 2019 14:19

Fixed failure dep in Cargo (CR feedback)

32cd6e0

Removed unnecesary test (CR feedback)

0f54490

softprops reviewed Jan 21, 2019

View reviewed changes

davidbarsky reviewed Jan 22, 2019

View reviewed changes

softprops reviewed Jan 22, 2019

View reviewed changes

Another round of CR feedback

bb61d71

sapessi added 3 commits January 24, 2019 09:28

Added comment to error header as per CR

6891eaa

Changed order of deps while cleaning up

ee12d37

Removed commented code as per CR

315c979

sapessi merged commit 50b71ae into master Jan 24, 2019

sapessi mentioned this pull request Jan 24, 2019

Handle any errors #26

Closed

dependabot bot mentioned this pull request Mar 15, 2021

Bump lambda_http from 0.1.1 to 0.3.0 friedemannsommer/website#98

Closed


		pub use crate::error_ext_impl::*;

		use failure::{format_err, Compat, Error, Fail};

[Do not merge] Refactor to address RFCs #63

[Do not merge] Refactor to address RFCs #63

Conversation

sapessi commented Dec 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbarsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softprops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapessi commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapessi commented Jan 23, 2019

softprops commented Jan 23, 2019

sapessi commented Jan 23, 2019

sapessi commented Dec 27, 2018 •

edited

Loading