awkward HandlerError requirements

[softprops]

I'm finding that, in practice, the new HandlerError' type constraints may have made error handling hard/awkward to satisfy for users

common cases I run into is trying to represent a foreign crate type for with lambda_runtime_errors::LambdaErrorExt. Since the Fail crate is becoming more common I think that's okay, but still rough if the foreign crates error type sticks to the standard library error type

I find myself often cluttering map_err(HandlerError::new) code with this

// I don't control the Err type of foo_bar as its defined in another crate
// so I can't impl lambda_runtime_errors::LambdaErrorExt for ThatError { ... }
foo_bar(input).map_err(HandlerError::new)? // fails to compile because no impl for lambda_runtime_errors::LambdaErrorExt

This is unfortunate for users because often these are simple hello world integrate xxx with lambda cases where the majority of the code written is satisfying the trait constraints on errors and not the code what brings business value.

I understand why this constraint exists but I want to revisit the value for users this is providing vs what its costing to author lambdas.

I've ran into this enough that I wanted to reopen the floor for alternative options.

[davidbarsky]

Thanks for opening this issue with the detailed problem statement. For what it's worth, I think that I agree with you. Since there's additional movement on RFC 1428, we might be able to get away from that trait entirely in the short term.

Regarding LambdaErrorExt: I'd be comfortable with any of the following options:

• Removing it as a requirement and instead falling back to a Display or Debug.
• If no Display or Debug implementation exists for an error, we just report “Unhandled error” to the runtime APIs.
• For those keen on reporting errors to the runtime APIs, we can expose a function whose sole function is error reporting.

Additionally, I think that with support for XRay + decent structured logging, we'd be able to provide sufficient information to customers that the error type name matters less. Thoughts, @sapessi?

[softprops]

That's my hope as well. I have good confidence in rust doing the right thing but am a little worried that lambda rust users will have to wait for an impl + stabilization period to pass.

I just thought of an alternative that may get the runtime what it wants with less hassle for users.

What do you think about an aux constructor for HandlerError where you could just provide the error name locally. Relax the type constraint on err to the very common unfancy cases but since LambdaErrorExt is mostly about providing a name for an error, if users don't have control over impl that for a foreign crates types, maybe an alternative is to fill that in place.

foo_bar(input).map_err(|e| HandlerError::named(e, "MyErrorName"))? // you get the type name and users are required to do less

I think this is still not great and will likely get onerous over time but a step better than what we have today w/o having to wait on an rfc. Benefit is you get to keep whats there for what the error crate has implemented for the std lib types but also extensible in a way that doesn't require setting up boiler plate to wrap foreign types.

In short form what I'd like to shoot for is making the simple cases simple and more complicated possible if needed without making simple cases pay extra tax for admission.

[softprops]

Re XRay + decent structured logging: I think there's a scheme in sdks ( I'm still digging through this ) for making the strategies serialization of errors user configurable thing. Maybe the default is as good as the std lib gives you but if you want fancy, pay the fancy tax and make it possible.

[sapessi]

We definitely plan to stop requiring (and potentially deprecate) LambdaErrorExt as soon as the error type_name rfc makes its way into stable.

The challenge with using Display or Debug - the first thing we explored - is that customers tend to use the error type field to aggregate errors in their dashboards. If we start including the full display output there - which regularly includes an extended error message - they won't be able to group as smoothly.

[softprops]

I wanted to share for better context what an upgrade to the newer version looked like I had to throw up my hands and essentially ended up errors as strings, tossing out a lot of the value of the type system

-fn poll(event: Value, ctx: Context) -> Result<(), HandlerError> {
+fn poll(
+    event: Value,
+    _: Context,
+) -> Result<(), HandlerError> {
     info!("recv {:?}", event);
-    let config = envy::from_env::<Config>()?;
+    let config = envy::from_env::<Config>().map_err(|e| failure::err_msg(e.to_string()))?;
     CORE.with(|c| {
         let mut core = c.borrow_mut();
-        let work = assign(&core.handle(), config);
-        core.run(work)?;
+        let work = assign(config);
+        core.run(work)
+            .map_err(|e| failure::err_msg(e.to_string()))?;
         Ok(())
     })
 }

[softprops]

thanks @sapessi the dash boarding use case is really helpful (missing) context for me.

[sapessi]

What about using the LambdaResultExt?

[softprops]

The compiler error didn't surface that as an option so I didn't discover it yet. Thanks.

It seems to have the same constraint on a LambdaErrorExt impl though so I'm still in the same stale mate position when I'm working with a foreign crate's error type.

[davidbarsky]

Thanks for reminding me of the dashboarding usecase, @sapessi—I keep forgetting that because I haven't done that before 😅.

I wonder if we can support that through an optional extension for customers that do care about this. To that end, I've been talking to @carllerche—he might be able to comment on this more—about the usage of Tower in underlying of the Lambda runtime, and my current thinking is that we can provide several valid handler signatures as implemented by specialized tower services and a corresponding Into<Service> (for context, this is the foundational Service trait) that'll be executed by the the Lambda runtime itself. Given this definition, we can provide several specialized services along the lines of:

• Service<(http::Request<T>, Context)> -> http::Response<U>
• Service<(http::Request<T>, Context)> -> Result<http::Response<U>, HandlerError>
• Service<(aws_lambda_events::event::sqs::SqsEvent, Context)> -> U
• Service<(aws_lambda_events::event::sqs::SqsEvent, Context)> -> Result<U, HandlerError>

Note: each service function signature would be implemented in terms of a sealed trait similar to HttpService.

In the absence of a handler error, the Into<Service> implementation would place a default value for HandlerError. Handler construction will still be available using a something similar to a ServiceFn.

To address some potential concerns:

• We might be able to cut down on our compile times by using more, smaller crates and disabling some of the chunkier features in Tokio, Hyper, and by removing our usage of Failure.
• We can provide support for Tide's HttpService behind a feature flag.

[softprops]

I'm not opposed to any of these solutions.

I'd like to come to a solution for this particular problem more quickly. One way to do that would be to have narrow problems keep narrow focus to avoid designing suspension bridges for potholes. The concrete and specific problem here is: the design of the type that all errors would eventually need to materialize into.

If I'm understanding the proposed solution above correctly, it would be to make it such that handler implementations could return different types of errors where currently handlers defined as anything that satisfies Fail + LambdaErrorExt + Display + Send + Sync. With the proposed solutions above, handler adapters (I think) would still have to eventually materialize a single error type at the end of the chain for the runtime to return to the lambda api.

Is there something that the solutions above solve about the actual error type itself?

I think adding something HandlerError::named(err, "MyErrorName") with a relaxed set of type constraints on err (perhaps just Display + Send + Sync) would satisfy lambdas error api. At a minimum, that's just an error message and type. Solving that could get us closer to the shape of the type the lambda api is looking for and still leave the door open solving the separate problem of rethinking handler interfaces. For that the handler interface changes I'd like to see a gh issue laying out the problem so that we can make sure we're applying a fitting solution. Representing errors may be part of that problem but I see redesiging handler types entirely as orthogonal a uniform error type.

Something did catch my eye above though which is very near and dear to my heart. There was mention of cutting down compile times... :) I'm very interested in that topic.

[davidbarsky]

I'd like to come to a solution for this particular problem more quickly. One way to do that would be to have narrow problems keep narrow focus to avoid designing suspension bridges for potholes. The concrete and specific problem here is: the design of the type that all errors would eventually need to materialize into.

That's a good point. I think the design I proposed is an acceptable long-term solution, but in the interest of addressing pain points now I think your proposal of a HandlerError::named(err, "MyErrorName") is a good first step.

If I'm understanding the proposed solution above correctly, it would be to make it such that handler implementations could return different types of errors where currently handlers defined as anything that satisfies Fail + LambdaErrorExt + Display + Send + Sync. With the proposed solutions above, handler adapters (I think) would still have to eventually materialize a single error type at the end of the chain for the runtime to return to the lambda api.

That's correct—for folks that don't care about dashboarding error types, we'd rely on HandlerError::default() (or just a String, as that allows us to place the HandlerError behind a feature flag + cut down on compile times. Like you said, “At a minimum, that's just an error message and type.”).

For that the handler interface changes I'd like to see a gh issue laying out the problem so that we can make sure we're applying a fitting solution. Representing errors may be part of that problem but I see redesiging handler types entirely as orthogonal a uniform error type.

That's a good point. I'll open a new issue with the handler design + close out #62, as it's pretty different now.

Something did catch my eye above though which is very near and dear to my heart. There was mention of cutting down compile times... :) I'm very interested in that topic.

Glad you noticed that 😀. I've noticed that because of Tower's insistence on a bunch of tiny crates that, in composition, provide similar feature sets to Hyper, Cargo is able to compile these tiny crates in parallel.

[carllerche]

Just looking at this from a high level, my recommendation would be to stick with std::error::Error. The failure crate is far from standard.

I would set bounds to: E: Into<Box<std::error::Error + Send + Sync>>. Then provide:

struct HandlerError {
    source: Box<Error + Send + Sync>,
    name: &'static str,
}

From the lambda runtime, to get the error name would be a two step process:

• First, walk the source list until you find an error of type HandlerError. If you find one, use that as the name.
• If no HandlerError was found, use the Debug implementation and, assume the format is TypeName { [fields] }, parse out the name.
• Fall back to "unknown error".

Is it great? No... but my gut reaction is that it would work better than forcing the error to be HandlerError as no web framework or whatever would have that type, so getting access to the name of the root error would be tricky.

Then, once type_name is stable, use that.