Make Rust Errors more idiomatic

[mrinalwadhwa]

I wrote

 let mut forwarding_address = String::new();
 std::io::stdin().read_line(&mut forwarding_address)?;

Got

error[E0277]: `?` couldn't convert the error to `ockam::Error`
  --> examples/alice.rs:15:54
   |
15 |   std::io::stdin().read_line(&mut forwarding_address)?;
   |                                                      ^ the trait `From<std::io::Error>` is not implemented for `ockam::Error`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following implementations were found:
             <ockam::Error as From<CredentialError>>
             <ockam::Error as From<NodeError>>
             <ockam::Error as From<OckamError>>
             <ockam::Error as From<ockam_core::routing::error::RouteError>>
           and 7 others
   = note: required by `from`

Update:

I think we need to take a deeper look at the design of our Errors so that they carry source, backtrace, cause etc. as they propagate within Ockam or surface to a user. We chose the code and domain approach because we'll need to expose them over FFI. But that shouldn't hurt the experience of Rust users. Rust Users should get source, backtrace, cause etc.

[mcastorina]

Hey, I would like to help implement this, but I'm new to ockam. I see the error definition has an error code and domain string.

pub struct Error {
    code: u32,

    #[cfg(feature = "std")]
    domain: String,
}

What should those be when there is a std::io::Error?

[mrinalwadhwa]

@mcastorina thank you for picking this up 🙏
@jared-s @SanjoDeundiak @antoinevg what are your thoughts on good values here.

I think we need to take a deeper look at the design of our Errors so that they carry source, backtrace, cause etc. as they propagate within Ockam or surface to a user. We chose the code and domain approach because we'll need to expose them over FFI. But that shouldn't hurt the experience of Rust users. Rust Users should get source, backtrace, cause etc.

[mcastorina]

I'm excited to contribute! :)

Perhaps https://github.com/dtolnay/thiserror will be useful here.

[SanjoDeundiak]

@mrinalwadhwa I'm not sure we should implement Fromstd::io::Error for ockam::Error. That may lead to worse errors experience. E.g. in this case when std::io::Error is thrown, from a function caller side I would expect error that says "Invalid forwarder address user input" or something like that, but if we implement default conversion from std::io::Error caller will get something like "IO error", which is less informative.

[SanjoDeundiak]

As for extending Error information with stacktrace and additional data, that would be great, but I'm not very familiar with how that works and how that can or can't be compatible with our Actor framework. There's some research work to be done by someone

[devalain]

@mrinalwadhwa I your example, you are writing an application so I don't think you should return ockam::Result but anyhow::Result instead in order to be more idiomatic (see anyhow vs thiserror) then you can use Context which is an easy way to produce useful error messages for the user. Plus, the error in your example has nothing to do with ockam, it comes from reading the standard input which depends on the OS.

Anyway, IMHO @mcastorina is right about the usefulness of thiserror: it can be used on structs and does not bring breaking change, it supports backtrace, source, From<Error> automatic implementation. As for ffi, the implementation can be adapted to the possible changes.

[antoinevg]

Nice to see love for dtolnay/thiserror and dtolnay/anyhow! IMO they're excellent defaults when it comes to Rust error handling.

That said, Ockam has some special needs.

@mrinalwadhwa already mentioned the need to support FFI but our existing error handling strategy also has a number of important advantages for targeting embedded (no_std) platforms:

1. The code: u32 field only consumes 4 bytes and the domain: String field is optional. This matters because we want to be able to target resource constrained platforms. ¹
2. We rely on explicit conversion to a universal ockam_core::Error type which allows us to have per-crate error definitions while avoiding the Box<dyn std::error::Error> pattern. This matters because no_std:
   • does not have access to the std::error::Error trait and
   • does not have access to a heap allocated Box type. ²

So the big question is, without losing these advantages, how can we get features like:

• backtraces / error-chaining
• converting a numeric error code back to a string representation of its originating enum type
• human-readable error messages

It would be nice to also support these on no_std but, just as with FFI targets, it's not the end of the world if we can't have all of them.

i.e. Neither thiserror or anyhow ³ provide full no_std support but this is not necessarily a problem if it's possible to integrate them with our existing error implementation in a way that won't require a bunch of #[cfg(not(feature="std"))] boilerplate to avoid breaking no_std builds.

In general, the situation is not great when it comes to idiomatic unified error-handling and no_std and is unlikely to improve in the future:

• Consider moving std::error::Error to alloc rust-lang/rust#62502
• rust-lang/project-error-handling?q=no_std

So maybe we should also be looking at using a collection of simple crates & code to implement our wish list individually?

A couple that spring to mind are:

• yaahc/displaydoc - provides thiserror-ish human-readable errors via a doc comment and string interpolation (Also has no_std support)
• core-error/core-error - an attempt to provide a unified Error trait implementation for no_std
• richardanaya/no_error - A simple error library for no_std + no_alloc Rust

Can anyone suggest some more?

Finally, Rust's error-handling story is starting to solidify but there are still a few big changes coming (e.g. RFC-2504) so it would be nice to not lock the library or dependent apps into anything that's going to be deprecated.

---

[1] One change I would like to see here is to define domain as a &'static str rather than String as, again, String needs alloc when we're targeting no_std.

[2] We can get Box via alloc but requiring alloc for something as fundamental as Error handling is probably not a great idea.

[3] The anyhow crate does provide partial no_std support but this comes at the cost of relying on alloc.

[adrianbenavides]

#1655 (comment) @mrinalwadhwa In your example, you are writing an application so I don't think you should return ockam::Result but anyhow::Result

+1 to use anyhow when writing client applications.

#1655 (comment) So the big question is, without losing these advantages, how can we get features like:

• backtraces / error-chaining
• converting a numeric error code back to a string representation of its originating enum type
• human-readable error messages

I agree with @antoinevg that the best thing for now is to wait. Given my recent experience (which you can read below), points 1 (backtraces) and 2 (u32 to strrepr) should be easier down the road if errors don't contain parameters. Finally, point 3 (human-readable error messages) should be feasible if the core::fmt::Display trait is implemented, right?

---

Here's my two cents on whether enum's variants should contain parameters to build custom messages or not and how this impact the above three points.

In the project I've been working on for the past two years (close to 100k lines) we use thiserror. We don't use any feature other than the Error and Display trait implementations to save us from writing some boilerplate code. That is, we don't make use of the backtrace or source features.

We follow a similar approach to what's done in the ockam crates. We have a struct like the following:

pub struct Error {
    code: ErrorCode,
    friendly_message: String,
    internal_message: String,
}

where:

• ErrorCode: is a string code, where multiple errors can return the same code (project requirements)
• internal_message: prints the Display implementation, which contains a detailed description of the error
• friendly_message: prints the message we want to display to the final user (API), usually a simplified version of the internal_message

Then we have a bunch of enums with the errors of a given layer (api, commands, infrastructure, core, ...) implementing the From<AbcdError> for Error, where each variant can have arbitrary parameters. For example:

#[derive(thiserror::Error, Debug)]
pub enum CustomError {
    #[error("score below threshold [expected={}][actual={}]", _0, _1)]
    ScoreBelowThreshold(u32, u32),
    #[error("unknown error")]
    Unknown,
}

We really struggled for a while deciding how to deal with error handling. We were also new to Rust. And, honestly, we are not 100% happy with this approach, but it has proved to be pretty solid so far. Having a good error handling strategy can make a huge difference in the developer experience, that's for sure.

The biggest disadvantage we've found: variants with parameters make debugging easier but are limiting if you want to use thiserror's source and backtrace features, for example. Having parameters also prevents us from converting an ErrorCode back to the original error. So unless it's strictly necessary, I would use the parameters to build log messages and keep the errors variants without parameters.

[thomcc]

I've been thinking about this a bit, and feel like I should write down my thoughts in case I'm getting something massively wrong about the ockam codebase — totally possible, I'm very new. A lot of this is informed by my work on the stdlib and mozilla/application-services (which had some similar constraints, but, well, fewer).

I think a design like the std::io::Error type is probably most appropriate for this. This should allow us to avoid giving up the ability to expose things on the other side of the FFI cleanly (at least in some cases). In particular, the fact that in io::Error, you always have an ErrorKind — for our errors we could enforce always having an error code and a domain (which I would change to a &'static str, or even a &'static CStr, or something that wraps it anyway, for FFI compat).

It hopefully also would allow us to avoid allocation in many cases, see the internal io::Error::const_new function (or if rust-lang/rust#87869 has landed, io::const_io_error!... unless it has been renamed, which seems possible), and even for the custom error case, eventually (perhaps not immediately), something based on unsize or stackbox could be used to have a Box<dyn Error+Send+Sync> that doesn't use the heap (it's also possible that heapless has options for non-heap-allocating boxed trait objects, I haven't looked). It would also allow natural support for backtraces, of course, on platforms and configurations where they're available.

Unfortunately, the approach you get with something like thiserror on an enum tends to be better if you don't have to ferry user errors through it. It also grows fairly large over time, and is hard to expose to the FFI, except manually. We could probably design our own proc macro for this (mostly for assigning error codes, which is otherwise tedious) and I do still consider that an option (I haven't made up my mind here), but... My experience is it's a bit of a headache, and in https://github.com/mozilla/application-services became a bit unwieldy. This approach feels sloppy and bad to use from user code unless you curtail that, which can be very difficult (although it's probably the cleanest and simplest to write, ironically).

Also, direct use of anyhow (or similar, such as eyre) from library crates isn't great, since those types don't implement Error (they can't for coherence reasons), which means users can't wrap them. While we could invent something similar (I'm pretty familiar with the internals, and have patches in anyhow's low level guts), it wouldn't get the ergonomics we need, and I'm unsure that we could do it in a way that maintians both the no_std support we want, as well as the ability to expose to the FFI.

All that said, one constraint I haven't fully worked through is serialization. In #967 the errors were reworked due to the need to be deserializable, and that... complicates the design somewhat, especially the desire to have static data. Hrm.

Sorry that this is a bit disjointed, I felt like getting it down when I had the thoughts rather than waiting until morning (which may have been advisable).

(This is slightly related to #1562, which is partially about FFI error ergonomics)

[antoinevg]

Great article about std::io::Error for everyone (like me!) who didn't realize just how neat the implementation is:

https://matklad.github.io/2020/10/15/study-of-std-io-error.html

[thomcc]

Yep, it's a great and easily-overlooked example of Rust API design (and matklad's whole blog is top-notch, I highly recommend it).

[thomcc]

All that said, one constraint I haven't fully worked through is serialization. In #967 the errors were reworked due to the need to be deserializable, and that... complicates the design somewhat, especially the desire to have static data. Hrm.

Having slept on it, I think there are two approaches here I hadn't really considered yesterday (and it's possible that the right solution will involve both in some capacity).

1. Use heapless::String. Even if we have the stdlib, it doesn't seem unreasonable to put a max length on the "error domain" string, although maybe I'm mistaken here.

2. Have deserialization convert a &'static str into a String.

For example:

// This is a private inner type that isn't exposed outside of ockam_core.
enum Repr {
   Static { code: u32, domain: &'static str, ... },
   NonStatic { code: u32, domain: String, ... },
   // ...
}

When serializing Repr::Static, it would contain data independent of which variant it used, but it would deserialize as Repr::NonStatic. This means we'd need to write manual Serialize/Deserialize implementations, but I don't think this is the end of the world.

A variant of this approach is to pack the &'static str and String into one bundle, similar to a Cow<'static, str>. (Other crates to see along these lines are https://github.com/thomcc/arcstr, and https://github.com/maciejhirsz/beef, although I don't think of these we'd actually use).

I still have to work through my thoughts here on whether or not this actually buys us anything (aside from some perf), but it's an interesting consideration I hadn't thought of when making the writeup last night.

(Sorry for all the messages, this is both one of my first issues here, and also something I think is critically important to having a good-feeling API, so I'd rather over-share my thought process than under-share it. It also serves to document some of the logic for future users)