Repository: iqlusioninc/abscissa (language: rust)

Issue:
crates: Replacements for failure?
[std::error::Error](https://doc.rust-lang.org/std/error/trait.Error.htm) now contains a trait as expressive as the `Fail` trait from `failure`, including downcast support and downcastable [Error::source](https://doc.rust-lang.org/std/error/trait.Error.html#method.source) as a replacement for `Error::cause` (now with a `'static` lifetime bound).

Additionally, there are a number of custom derive crates for `std::error::Error` now available:

- [`err-derive`](https://gitlab.com/torkleyy/err-derive)
- [`snafu`](https://docs.rs/snafu/)
- [`thiserror`](https://github.com/dtolnay/thiserror)

Given that, it seems like we can eliminate `failure` as a dependency, potentially replacing `failure_derive` with one of the crates above.

## Serializable Errors

A bit of a sidebar, but one of the things I'd like to eventually accomplish is `serde` support for the framework's `Error` type, making it `Serialize`-able (and ideally `Deserialize`-able as well), with the intent of serializing them as JSON for reporting to an exception reporting service.

The `backtrace` crate offers `serde` support as an optional feature, however accessing this functionality was previously difficult as Abscissa's `Error` type uses [failure::Context](https://docs.rs/failure/latest/failure/struct.Context.html) to capture and store backtraces.
It seems like Abscissa could benefit from providing a similar `Context` type as a replacement for the one `failure` provided. Ideally this type could simply `derive(Serialize, Deserialize)`, permitting straightforward serialization of errors as well as their backtraces (and ideally, the entire error chain).

Unfortunately the story around all of this is complicated by [adding a `Backtrace` type to `std`](https://github.com/rust-lang/rust/pull/64154), presently only available on `nightly`. This `Backtrace` type has deliberately minimal functionality.

I'd consider supporting `Backtrace` on stable a requirement, and ideally preserving the `serde` serialization support. This seems possible with [`snafu`](https://docs.rs/snafu/) but needs more investigation.

Crates like [`err-derive`](https://gitlab.com/torkleyy/err-derive) and [`thiserror`](https://github.com/dtolnay/thiserror) have the advantage of being pure proc macros with no additional API dependencies, so if nothing else they stay nicely out of the way and orthogonal to this problem. I imagine if we don't go with `snafu`, we could pick one of these to be the out-of-the-box custom derive for errors, but with the only framework-level association being inclusion in the default application template (allowing downstream users of the users to potentially remove it or swap it out if they so desire)


Discussion / hints:
I think that this issue mixes two distinct questions, assuming the goal is to replace all uses of the `Fail` trait with the `Error` trait:

1. What crate, if any, should be used to derive Abscissa's error types;
2. What type (implies: from which crate) Abscissa should use as its catch-all type for working with errors within an Abscissa application.

Currently, Abscissa uses `failure` for (1) and `failure::Error` for (2).  But these are actually different functional roles and should be considered differently.  `thiserror` and `err-derive` are only useful for (1) and not (2), so choosing one or the other still leaves the choice for (2) open.

The answer to question (1) seems relatively less important to Abscissa users, because any answer to question (1) is an implementation detail for Abscissa.  The answer to question (2) seems relatively more important, because any answer shows up in the public API.

Two possible answers to question (2) are:

1. `type BoxedStdError = Box<dyn std::error::Error + Send + Sync + 'static>;`
2. [`anyhow::Error`](https://docs.rs/anyhow/1.0.16/anyhow/struct.Error.html), like a `Box<dyn Error + Send + Sync + 'static>` but with additional ergonomics.

As an Abscissa user, I would strongly prefer having all generic errors be bounded with `Send + Sync + 'static` to make using them with multithreaded async code easier.  Using a `BoxedStdError` type alias probably prevents adding serialization support.


I'd be fine with defining a framework level boxed error type. See the "[Proposal: add std::error::BoxError](https://internals.rust-lang.org/t/proposal-add-std-boxerror/10953)" rust-internals thread which explores various tradeoffs regarding how it should be defined.

The main problem with using that as "the" `Error` type, at least for now, is it can't carry a `Backtrace`.

`anyhow` solves this by using nightly's `Backtrace` type. Unfortunately that doesn't help stable users, or support backtrace serialization AFAIK. Longer term it seems nice though.
> The `backtrace` crate offers `serde` support as an optional feature, however accessing this functionality was previously difficult as Abscissa's `Error` type uses [failure::Context](https://docs.rs/failure/latest/failure/struct.Context.html) to capture and store backtraces.
> It seems like Abscissa could benefit from providing a similar `Context` type as a replacement for the one `failure` provided. Ideally this type could simply `derive(Serialize, Deserialize)`, permitting straightforward serialization of errors as well as their backtraces (and ideally, the entire error chain).

FWIW, I just thought I'd drop in and add that I'm currently experimenting with a `tracing-error` crate (not yet published) that adds support for capturing the `tracing` span context in which an error was constructed, and including it with the error. This forms something not unlike a backtrace, but consisting of user-provided semantic contexts rather than stack frames (a stack trace could be captured as well). If this is something you're interested in, especially in light of #149, I'd be happy to let you know when this experiment is ready to start trying out.
@hawkw very cool. It'd be quite interesting if it were possible to trace things like asynchronous events which originate in an RPC request but the fire off some sort of background job (kind of like an in-process equivalent of the similar functionality in Dapper/Zipkin/OpenTracing).

> a stack trace could be captured as well

I'd almost certainly want that
One quick note on backtrace capture in general: it seems like the best option there (and the [direction the `backtrace` crate is heading](https://github.com/rust-lang/backtrace-rs/issues/189)) is to use [`gimli`](https://github.com/gimli-rs/gimli) (as opposed to [`libbacktrace`](https://github.com/ianlancetaylor/libbacktrace)). The linked thread notes a history of security issues in `libbacktrace`, whereas `gimli` is written in Rust and should therefore be safer.

Unfortunately that pulls in a large number of dependencies today, so I'm wary to enable `gimli` by default until it lands in `std` and therefore those dependencies are linked as part of the Rust distribution itself and come at zero cost to Abscissa (which could then ideally leverage the currently nightly-only [`backtrace` method](https://doc.rust-lang.org/std/error/trait.Error.html#method.backtrace) defined on `std::error::Error`).

It would be easy enough to add an off-by-default Cargo feature to enable them, however, and a commented-out line in the application boilerplate template to optionally enable it (in which case I'd also turn it on in all of my personal apps).
> @hawkw very cool. It'd be quite interesting if it were possible to trace things like asynchronous events which originate in an RPC request but the fire off some sort of background job (kind of like an in-process equivalent of the similar functionality in Dapper/Zipkin/OpenTracing).

Yup, that's more or less the exact goal I had in mind! :)

> One quick note on backtrace capture in general: it seems like the best option there (and the [direction the `backtrace` crate is heading](https://github.com/rust-lang/backtrace-rs/issues/189)) is to use [`gimli`](https://github.com/gimli-rs/gimli) (as opposed to [`libbacktrace`](https://github.com/ianlancetaylor/libbacktrace)). The linked thread notes a history of security issues in `libbacktrace`, whereas `gimli` is written in Rust and should therefore be safer.
> 
> Unfortunately that pulls in a large number of dependencies today, so I'm wary to enable `gimli` by default until it lands in `std` and therefore those dependencies are linked as part of the Rust distribution itself and come at zero cost to Abscissa (which could then ideally leverage the currently nightly-only [`backtrace` method](https://doc.rust-lang.org/std/error/trait.Error.html#method.backtrace) defined on `std::error::Error`).

When I get around to adding stack backtraces to a `tracing-error` crate, I'll definitely keep this in mind; until `gimli` lands in std, I think it will end up having separate feature flags for `libbacktrace` and `gimli` backtrace capture.
> I'd be fine with defining a framework level boxed error type.

I added an `abscissa_core::error::BoxError` type alias in #151, bounded as `Send + Sync + 'static`:

https://github.com/iqlusioninc/abscissa/pull/151/files#diff-11635f54908685fd908957a138677245R14
Here's a comprehensive analysis of various popular Rust error handling libraries, what features they provide, and how they relate to each other:

https://blog.yoshuawuyts.com/error-handling-survey/
Now that v0.5.0 is out and I'm updating a lot of apps which previously used `failure_derive`, I've been using [`thiserror`](https://github.com/dtolnay/thiserror) in pretty much all cases.

I think it'd be worth including in the framework boilerplate (but not necessarily as a framework-level dependency).

This could be done in a v0.5.1 release of the `abscissa` crate without any changes to `abscissa_core`.

Fix patch:
diff --git a/README.md b/README.md
index 64ef6c61..19279d0b 100644
--- a/README.md
+++ b/README.md
@@ -167,9 +167,10 @@ default set of features in the application:
 | 13 | [strsim]             | [@dguo]          | MIT            | String similarity utils |
 | 14 | [syn]                | [@dtolnay]       | Apache-2.0/MIT | Rust source code parser |
 | 15 | [synstructure]       | [@mystor]        | Apache-2.0/MIT | `syn` structure macros  |
-| 16 | [tracing-attributes] | [tokio-rs]       | MIT            | App tracing / logging   |
-| 17 | [unicode-xid]        | [unicode-rs]     | Apache-2.0/MIT | Identify valid Unicode  |
-| 18 | [wait-timeout]       | [@alexcrichton]  | Apache-2.0/MIT | Timeouts for waitpid    |
+| 16 | [thiserror]          | [@dtolnay]       | Apache-2.0/MIT | `Error` custom derive   |
+| 17 | [tracing-attributes] | [tokio-rs]       | MIT            | App tracing / logging   |
+| 18 | [unicode-xid]        | [unicode-rs]     | Apache-2.0/MIT | Identify valid Unicode  |
+| 19 | [wait-timeout]       | [@alexcrichton]  | Apache-2.0/MIT | Timeouts for waitpid    |
 
 ### Dependency Relationships
 
@@ -233,6 +234,7 @@ so you only compile the parts you need.
 | [stable_deref_trait]   | `trace`            | [owning_ref]              |
 | [syn]                  | -                  | [abscissa_derive], [darling], [gumdrop_derive], [serde_derive] |
 | [termcolor]            | `terminal`         | [abscissa_core]           |
+| [thiserror]            | -                  | Abscissa boilerplate      |
 | [thread_local]         | `trace`, `testing` | [regex]                   |
 | [time]                 | `logging`          | [chrono]                  |
 | [tracing]              | `trace`            | [abscissa_core]           |
@@ -429,6 +431,7 @@ read the [CONTRIBUTING.md] and [CODE_OF_CONDUCT.md] files first.
 [syn]: https://crates.io/crates/syn
 [synstructure]: https://crates.io/crates/synstructure
 [termcolor]: https://crates.io/crates/termcolor
+[thiserror]: https://github.com/dtolnay/thiserror
 [thread_local]: https://crates.io/crates/thread_local
 [time]: https://crates.io/crates/time
 [toml]: https://crates.io/crates/toml
diff --git a/cli/template/Cargo.toml.hbs b/cli/template/Cargo.toml.hbs
index 3fcac9a8..0bbf0c05 100644
--- a/cli/template/Cargo.toml.hbs
+++ b/cli/template/Cargo.toml.hbs
@@ -7,6 +7,7 @@ edition = "{{edition}}"
 [dependencies]
 gumdrop = "0.7"
 serde = { version = "1", features = ["serde_derive"] }
+thiserror = "1"
 
 [dependencies.abscissa_core]
 version = "{{abscissa.version}}"
diff --git a/cli/template/src/error.rs.hbs b/cli/template/src/error.rs.hbs
index d6713c5b..cdc847f7 100644
--- a/cli/template/src/error.rs.hbs
+++ b/cli/template/src/error.rs.hbs
@@ -6,14 +6,17 @@ use std::{
     io,
     ops::Deref,
 };
+use thiserror::Error;
 
 /// Kinds of errors
-#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+#[derive(Copy, Clone, Debug, Eq, Error, PartialEq)]
 pub enum ErrorKind {
     /// Error in configuration file
+    #[error("config error")]
     Config,
 
     /// Input/output error
+    #[error("I/O error")]
     Io,
 }
 
@@ -24,19 +27,6 @@ impl ErrorKind {
     }
 }
 
-impl Display for ErrorKind {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let description = match self {
-            ErrorKind::Config => "config error",
-            ErrorKind::Io => "I/O error",
-        };
-
-        f.write_str(description)
-    }
-}
-
-impl std::error::Error for ErrorKind {}
-
 /// Error type
 #[derive(Debug)]
 pub struct Error(Box<Context<ErrorKind>>);


Test patch: