Regression in v0.12.0: External error types no longer work with lambda_runtime::run

TethysSvensson commented 1 month ago

With the release of v0.12.0, it is no longer possible to use error types defined externally to your own crate. This is caused by the inclusion of #897.

Example:

// Works in both v0.11.3 and v0.12.0
type Error = Box<dyn std::error::Error>;

// Works in v0.11.3 but not in v0.12.0
// type Error = std::io::Error;

// Works in v0.11.3 but not in v0.12.0
// type Error = anyhow::Error;

// Works in neither v0.11.3 nor v0.12.0
// type Error = lambda_runtime::Diagnostic<'static>;

async fn handler(_event: lambda_runtime::LambdaEvent<()>) -> Result<(), Error> {
    Ok(())
}

#[tokio::main]
async fn main() {
    lambda_runtime::run(lambda_runtime::service_fn(handler))
        .await
        .unwrap();
}

Note that it is not possible to implement Into<Diagnostic<'a>> yourself for any of these types because of the orphan rules.

calavera commented 1 month ago

Feel free to open a PR to improve the support of those errors 🙏

TethysSvensson commented 1 month ago

I don't see any way to fix this without compromising on #880.

You will need to choose whether you want to automatically support error types that implement Display or not. In either case, if your choice does not match the users, then they will need to implement a work-around in their code.

I will list a few different options for how to proceed from here. Let me know which one you prefer, and I will implement a PR for that.

This is the context as I see and and how I will evaluate the different options:

Almost all users will want to use error types that implement Display.
The majority of users would prefer an implementation that just works out of the box. They do not care about customizing their conversion.
A minority of users would like to customize their conversion to Diagnostic.
We want to support both the majority user-case and the niche use-case with as little friction as possible.
If friction has to be introduced, make it as small as possible, but prioritize minimizing friction in smaller in the majority case.

Option 1: Keeping things as they are

With the current code, all error types besides the few types blessed in diagnostic.rs will be slightly annoying to deal with.

Your choices are to either convert to one of the few blessed types:

A type internal to lambda_runtime
A few different box types, specifically one of:
- lambda_runtime_api_client::BoxError aka Box<dyn std::error::Error + Sync + Send>
- Box<dyn std::error::Error>
- Box<T> where T: std::error::Error
String and &'static str.

Or alternatively you can make you can define your own custom error type like this:

struct MyErrorType(...);

impl<'a> From<MyErrorType> for Diagnostic<'a> {
    fn from(value: MyErrorType) -> Diagnostic<'a> {
        ...
    }
}

Note, you cannot implement From/Into for types external to your own crate, so you will need to do one of these conversion if you use e.g. anyhow.

Option 2: Partially revert #897 to re-add the blanked impl

Reverting would fix the majority use-case, but it would re-introduce friction in the minority use-case. It would become difficult to use a custom conversion for your own error types that already implement Display.

The only real way to achieve custom conversion for your own types would be something like this:

#[derive(Debug, thiserror::Error)]
struct MyError { ... }

struct AnnoyingErrorWrapper(MyError);

impl<'a> From<AnnoyingErrorWrapper> for Diagnostic<'a> {
    fn from(value: AnnoyingErrorWrapper) -> Diagnostic<'a> {
        ...
    }
}

impl From<MyError> for AnnoyingErrorWrapper {
    fn from(value: MyError) -> AnnoyingErrorWrapper {
        AnnoyingErrorWrapper(value)
    }
}

Option 3: Partial revert with ergonomic improvements

As I see it, there is no way to introduce some friction to either the majority or minority use-cases, however that friction could be made much smaller than in either option 1 or 2.

I think the best way would be to re-add the blanket impl, but also make it easier to return custom diagnostics. I see two non-conflicting ways we could reduce the friction.

Friction reduction 1:

It is currently impossible to directly return a Diagnostic<'static> from your handler, because we require for<'b> Into<Diagnostic<'b>>. This is because Diagnostic<'static> only has an implementation of Into<Diagnostic<'static>> despite being coercible into any Diagnostic<'a>.

We could fix this by using a custom trait IntoDiagnostic<'a> instead of using Into<Diagnostic<'a>>. That way the user would be able to create a helper function like this:

async fn handler(event: lambda_runtime::LambdaEvent<()>) -> Result<(), Diagnostic<'static>> {
    real_handler(event)
        .await
        .map_err(|e: MyError| -> Diagnostic {
            ...
         })
}

async fn real_handler(_event: lambda_runtime::LambdaEvent<()>) -> Result<(), MyError> {
    ...
}

Friction reduction 2:

We could also add an additional type CustomDiagnostic<'a>, which did not have an automatic blanket impl, where users could implement their own conversion logic. It could look something like this:

impl<'a> From<MyError> for CustomDiagnostic<'a> {
    fn from(value: MyError) -> Self {
        todo!()
    }
}

async fn handler(event: lambda_runtime::LambdaEvent<()>) -> Result<(), CustomDiagnostic<'static>> {
    func1(&event).await?;
    func2(&event).await?;
    Ok(())
}

async fn func1(event: &lambda_runtime::LambdaEvent<()>) -> Result<(), MyError> {
    todo!()
}

async fn func2(event: &lambda_runtime::LambdaEvent<()>) -> Result<(), MyError> {
    todo!()
}

Friction reduction 3:

We could create a macro for creating custom conversion types. It could look something like this:

create_error_wrapper! { MyError => AnnoyingErrorWrapper with converter }

fn converter(value: MyError) -> Diagnostic<'static> {
    ...
}

This would then unfold to this:

struct AnnoyingErrorWrapper(MyError);
impl<'a> From<AnnoyingErrorWrapper> for Diagnostic<'a> {
    fn from(value: AnnoyingErrorWrapper) -> Diagnostic<'a> {
        converter(value.0)
    }
}

impl From<MyError> for AnnoyingErrorWrapper {
    fn from(value: MyError) -> AnnoyingErrorWrapper {
        AnnoyingErrorWrapper(value)
    }
}

Option 4: Ergonomic improvements without a blanket impl

It would also be possible to do some of the ergonomic improvements without re-adding the blanket impl. Specifically the first and third ergonomic improvements would still make sense in this case.

Conclusion

Personally I think the best option is Option 3, with friction reduction number 1. I would not mind also adding number 2 and 3 though.

calavera commented 1 month ago

Let's start with option 1. I don't fully understand what the benefit of option 2 is(I might need more information), and I'd rather avoid macros unless they're absolutely necessary.

Additionally, I think we can add other common error implementations, like std::io::Error that you brought up. I've been thinking more about implementations for errors crates like anyhow, and it might be ok to add implementations for them behind a feature flag.

TethysSvensson commented 1 month ago

I am confused about whether you mean options or the friction reductions I've discussed.

Basically my outline had are two orthogonal choices:

Whether to bring back the blanket impl
Which ergonomic improvements to use.

My suggestion was to bring back the blanket impl to benefit the majority case, but then add some ergonomic improvements to make the minority case less cumbersome.

calavera commented 1 month ago

I consider giving people the ability to completely customize the error to return more important that returning string messages for any type of error. We're not going to bring back the blanket implementation. Returning an anyhow message is not that much complicated with the current version, this seems to work:

async fn function_handler(event: LambdaEvent<Request>) -> Result<(), Error> {
  Err(anyhow!("ooops").into())
}

We've never guaranteed backwards compatibility for this exact reason, we need to ensure that we can provide access to all features that Lambda provides, including returning the right error payloads, and we need to be able to make these kind of breaking changes before we stabilize the runtime.

I'm happy to merge further improvements on top of the current implementation though.

TethysSvensson commented 1 month ago

Sure, but if you do that, then you:

Introduce friction into the majority case instead of the minority case. It is easy to support both, the question is where you prefer a small amount of friction to be introduced.
You require double-boxing all errors, since anyhow is already boxed and so is lambda_runtime::Error.
You loose the benefit of the diagnostic, which is the error_type, since the error type will always be "alloc::boxed::Box<dyn core::error::Error + core::marker::Send + core::marker::Sync>".

tim3z commented 1 month ago

As a naive user who until the upgrade to 0.12 was happily returning some impl of Error from his lambda handler, I can only second @TethysSvensson's point here. This is a surprising breaking change for everyone who didn't even know what Diagnostic was until the crate upgrade broke something. Sure it's easy to fix. But making the default path more complicated to simplify some customization is usually a bad tradeoff.

calavera commented 1 month ago

You loose the benefit of the diagnostic, which is the error_type, since the error type will always be "alloc::boxed::Box<dyn core::error::Error + core::marker::Send + core::marker::Sync>".

I would argue that both anyhow::Error and "alloc::boxed::Box<dyn core::error::Error + core::marker::Send + core::marker::Sync>" are bad error types.

But making the default path more complicated to simplify some customization is usually a bad tradeoff.

@tim3z error types are not a customization, they are part of Lambda's feature set. Unfortunately, we've been ignoring it for way too long, and now we're paying the price. This is definitely on me for not approaching the problem sooner. This has also created some bad practices, like parsing the error message to observe and understand why a function call failed, where the error_type exists for this specific reason.

As I mention earlier, I'm happy to review improvements upon this breaking change to improve the experience, as long as defining diagnostics doesn't get buried again.

[EDIT] The best possible way to find a solution is through PRs. There are several error handling examples in the examples directory. Feel free to propose a solution that makes those examples work without making it complicated to implement diagnostics.

calavera commented 1 month ago

I've made some improvements in https://github.com/awslabs/aws-lambda-rust-runtime/pull/907. Let me know what you think.

github-actions[bot] commented 1 month ago

This issue is now closed. Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one.

awslabs / aws-lambda-rust-runtime