Support causes in amethyst::Error #1214

udoprog · 2018-12-04T15:51:29Z

Our existing error type is sparse and doesn't include any context of where an error happened.

We should include causes with contexts, and backtraces to our error type to improve our diagnostic capabilities both to users and developers.

Causes

The failure allows doing this by adding "contexts":

use failure::{Error, ResultExt};

type Result<T> = Result<T, Error>;

fn ring_bell(sound: &str) -> Result<()> {
  bail!("I don't feel like ringing the bell")
}

fn something() -> Result<()> {
  ring_bell("ding").with_context(|_| format_err!("Failed to ding"))?;
  ring_bell("dong").with_context(|_| format_err!("Failed to dong"))?;
  Ok(())
}

Causal chains are useful because errors can be quite generic (i.e. I don't feel like ringing the bell). But in this case an important piece of information is: did it fail when dinging or donging?

Backtraces

I think most folks should be familiar with back traces, but I'll just provide some brief thoughts on why they should be included.

Backtraces provide additional context, but can be quite low level. Panics by default include a backtrace. This is something we will be losing out on unless we include it in our error types.

Causes without backtraces could be rendered like this, and should be highly useful for many error cases:

Failed to ding
Caused by: I don't feel like ringing the bell.

Including backtraces provides much more detailed information:

Failed to ding
stack backtrace:
   0:     0x5616b3933aa6 - backtrace::backtrace::libunwind::trace::hebc3cbfc0bad42f5
                        at /home/udoprog/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.9/src/backtrace/libunwind.rs:53
                         - backtrace::backtrace::trace::h662d733ae29451d7
                        at /home/udoprog/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.9/src/backtrace/mod.rs:42
   1:     0x5616b392e053 - backtrace::capture::Backtrace::new_unresolved::hbefd36b5dad230fc
                        at /home/udoprog/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.9/src/capture.rs:88
   2:     0x5616b392cb8f - failure::backtrace::internal::InternalBacktrace::new::h47fcb9ce8ccc42f5
                        at /home/udoprog/.cargo/registry/src/github.com-1ecc6299db9ec823/failure-0.1.3/src/backtrace/internal.rs:44
   3:     0x5616b392b8ec - failure::backtrace::Backtrace::new::he764f9bfd4f865b9
                        at /home/udoprog/.cargo/registry/src/github.com-1ecc6299db9ec823/failure-0.1.3/src/backtrace/mod.rs:111
   4:     0x5616b2dbc019 - <failure::error::error_impl::ErrorImpl as core::convert::From<F>>::from::h765cf9c59ef2155c
                        at /home/udoprog/.cargo/registry/src/github.com-1ecc6299db9ec823/failure-0.1.3/src/error/error_impl.rs:19
   5:     0x5616b2db51e8 - <failure::error::Error as core::convert::From<F>>::from::h22d775fc32792a12
                        at /home/udoprog/.cargo/registry/src/github.com-1ecc6299db9ec823/failure-0.1.3/src/error/mod.rs:36
   6:     0x5616b2d94bb6 - failure::error_message::err_msg::h96fd6ab063debaa8
                        at /home/udoprog/.cargo/registry/src/github.com-1ecc6299db9ec823/failure-0.1.3/src/error_message.rs:12
   7:     0x5616b2c1688d - quickcfg::ring_bell::hd60e60cb927b4bf8
                        at src/main.rs:20
   8:     0x5616b2c168da - quickcfg::something::h98ab5d22e04a3248
                        at src/main.rs:24
   9:     0x5616b2c17658 - quickcfg::try_main::h8e376b3c3dd53ebe
                        at src/main.rs:63
  10:     0x5616b2c17298 - quickcfg::main::he714b694ac2ecf8a
                        at src/main.rs:52
  11:     0x5616b2c9ff3f - std::rt::lang_start::{{closure}}::h82daacb8cc7771f5
                        at /rustc/b68fc18c45350e1cdcd83cecf0f12e294e55af56/src/libstd/rt.rs:74
  12:     0x5616b397a632 - std::rt::lang_start_internal::{{closure}}::h72c946a380ebec10
                        at src/libstd/rt.rs:59
                         - std::panicking::try::do_call::h221042e980f16854
                        at src/libstd/panicking.rs:310
  13:     0x5616b398f349 - __rust_maybe_catch_panic
                        at src/libpanic_unwind/lib.rs:102
  14:     0x5616b397b0c3 - std::panicking::try::h8b8c3a00343d5ed4
                        at src/libstd/panicking.rs:289
                         - std::panic::catch_unwind::h9533bb130dfaffce
                        at src/libstd/panic.rs:398
                         - std::rt::lang_start_internal::h4511552c790253e4
                        at src/libstd/rt.rs:58
  15:     0x5616b2c9ff18 - std::rt::lang_start::he32bc485e59e6dd3
                        at /rustc/b68fc18c45350e1cdcd83cecf0f12e294e55af56/src/libstd/rt.rs:74
  16:     0x5616b2c1db89 - main
  17:     0x7fdf42a6309a - __libc_start_main
  18:     0x5616b2bb41a9 - _start
  19:                0x0 - <unknown>
Caused by: I don't feel like ringing the bell

The text was updated successfully, but these errors were encountered:

torkleyy · 2018-12-04T16:00:20Z

Related: #690 #516 #1140 #460

udoprog · 2018-12-04T16:22:44Z

Hm, yeah. The errors ecosystem is a bit of a clustertruck right now and I could have dug deeper into the existing discussions.

@torkleyy So if I've read things correctly, the status quo is that someone should write an RFC?

torkleyy · 2018-12-04T16:33:22Z

I'd personally just go with failure. I know there's an argument that it will eventually end up in std, but it's what seems to be the standard right now (and I don't think it's good to just leave the error story as-is until the std situation improves). If that's something we can't agree on, an RFC would make sense, yes.

torkleyy · 2018-12-04T17:41:22Z

@amethyst/engine-devs please comment whether you'd like an RFC.

azriel91 · 2018-12-04T17:50:50Z

I'm happy with going with failure

LucioFranco · 2018-12-04T17:53:25Z

I would like to stay away from failure because in its current state its deprecated. I say we punt on this issue till rust has settled on a proper backtrace type.

There are two big areas that need to be addressed before I say we move forward. One is waiting for log to get structured logging and for failure or std to settle on proper error handlings types.

torkleyy · 2018-12-04T18:12:06Z

Can you provide any references for its deprecation?

…

On Tue, Dec 4, 2018, 18:53 Lucio Franco ***@***.*** wrote: I would like to stay away from failure because in its current state its deprecated. I say we punt on this issue till rust has settled on a proper backtrace type. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1214 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWFFuUwL4FHQbMNhlbu5vbCYDfvEI6Q0ks5u1raagaJpZM4ZA8fO> .

LucioFranco · 2018-12-04T18:13:58Z

@torkleyy Yes here

Evolution

Failure is currently evolving as a library. First of all there is work going on in Rust itself to fix the error trait secondarily the original plan for Failure towards 1.0 is unlikely to happen in the current form.

As such the original master branch towards 1.0 of failure was removed and master now represents the future iteration steps of 0.1 until it's clear what happens in the stdlib.

The original 1.0 branch can be found in evolution/1.0.

fu5ha · 2018-12-04T18:16:54Z

Is the error handling a large enough issue right now to warrant two rewrites to go to current-failure (or whatever custom error solution we make in an RFC) and then to std errors or new-failure when the time comes? I'd say probably not, but perhaps it could be argued; if not then I say we just punt the issue until std/Rust ecosystem in general figures it out as @LucioFranco suggested.

LucioFranco · 2018-12-04T18:18:28Z

Really, error handling is a 1.0 issue instead of a current milestone. It doesn't affect our ability to create things. So I say punt until rust has settled or else we will rewrite error handling for the 3rd and 4th time by the time we hit 1.0, and its not a good use of our time.

Takes off my @fhaynes hat

torkleyy · 2018-12-04T18:24:38Z

I don't consider that a deprecation.

My only motivation is to settle on something for now, even if it's not perfect. I don't see this important enough to make a big discussion about it right now though, so I'm also fine with leaving things as they are.

LucioFranco · 2018-12-04T18:26:19Z

@torkleyy what do you mean you don't consider it a depreciation? The failures team literally describes it like that.

I personally, would like to see amethyst move to just error enums, as that is the most lightweight way to handle errors right now. Then move to the std error trait later.

LucioFranco · 2018-12-04T18:28:31Z

Circling back this is the big issue we should be following rust-lang/rust#53487

torkleyy · 2018-12-04T18:29:39Z

I personally, would like to see amethyst move to just error enums, as that is the most lightweight way to handle errors right now.

That's pretty much what failure does.

LucioFranco · 2018-12-04T18:31:50Z

Right, but I say we hold off on touching any error handling for now. Wait, till rust has it stabilized. I don't even think we should invest our time into it until backtraces are in and stabilized.

udoprog · 2018-12-04T18:36:31Z

I think Amethyst both short and long term can maintain its own amethyst::Error type.

It would implement std::error::Error both before, and after the proposed changes. This would be compatible with people using failure today, and any std::error::Error trait that shows up tomorrow.

EDIT: I have done something like that in reproto, minus implementing std::error::Error because it's so bad: https://github.com/reproto/reproto/blob/master/lib/core/src/errors.rs

LucioFranco · 2018-12-04T18:43:29Z

So related to the RFC https://github.com/rust-lang/rfcs/blob/master/text/2504-fix-error.md#how-this-impacts-failure do you think its worth it to implement everything based on Fail then migrate it over. What benefit do we get now that we wouldn't get by just waiting?

That said too, backtrace in failure is not like the proposed one in the accepted RFC.

Mostly, my worries stem from failure being an experiment.

LucioFranco · 2018-12-04T18:47:05Z

@udoprog related to your reproto stuff why should we implement that now when we can just wait for it to exist within rust? As I see it you are implementing it similar to how backtrace will be done, but that part of the API is not resolved. So to me, it doesn't make sense to invest resources into having our own for the short future.

1220: Introduce amethyst_error r=fhaynes a=udoprog Following #1214 This introduces the minimal API necessary to convert existing Error types (some which includes causes). It currently mimics the `failure::Error` API, with methods named according to the ["fix Error" RFC](https://github.com/rust-lang/rfcs/blob/master/text/2504-fix-error.md). That means: * We include the `format_err!` macro and the `ResultExt` trait with an implementation of `with_context` from `failure`. * The error type includes backtraces directly (instead of wrapping it in through e.g. `Context<D>`) in comparison to `failure`. * We include a blanket `From<E> where E: std::error::Error` implementation to convert any type that implements `std::error::Error` into `Error`. * Note: this has the side-effect that this `Error` can't implement `std::error::Error`. * We are using `source` from `std::error::Error` instead of `cause`. * We don't implement `std::error::Error` directly, but we do implement some methods which are similar (`source`). We implement `as_error` to access a reference to the underlying `std::error::Error`. Similarly to how `failure::Error` doesn't implement `Fail` directly but provides `as_fail`. Note that since this RFC is so sparse, there's really not much to implement, and most other methods which are used to actually build errors are local to this crate (e.g. `ResultExt::with_context`). I have a branch where I've tried to apply this crate here: https://github.com/udoprog/amethyst/commit/amethyst-error-full I think I might have accidentally botched some of the `From<T>` implementations, but it should still give an idea of how this type is used. #### Composite APIs Composite APIs are APIs which potentially make use of local error information. Today these tend to lead to "stringy" errors where most of the underlying error information is lost and the errors undergoes a conversion. These are candidates to make use of `amethyst_error::Error` directly right now. The traits I've so far identified that has a tendency to lead to this are: * [`amethyst_assets::Source`](/amethyst/amethyst/blob/master/amethyst_assets/src/source/mod.rs) * [`amethyst_assets::PrefabData`](/amethyst/amethyst/blob/master/amethyst_assets/src/prefab/mod.rs) * [`amethyst_assets::SimpleFormat`](/amethyst/amethyst/blob/master/amethyst_assets/src/asset.rs) * Here's an example where SimpleFormat "stringifies" errors instead of propagating them to work around error constraints: https://github.com/amethyst/amethyst/blob/master/amethyst_ui/src/prefab.rs#L736 * [`amethyst_renderer::Pass/Pipeline` stuff](/amethyst/amethyst/blob/master/amethyst_renderer/src/pipe/pass.rs). Unsure because it spans over to UI and I don't know how "end user" this is. #### Open Discussions * Dropping the `Sync` constraint ([comment](#1220 (comment))). * ~~Either drop `no_std` support or improve it ([comment 1](#1220 (comment)), [comment 2](#1220 (comment) backtrace is no longer optional. Co-authored-by: John-John Tedro <udoprog@tedro.se> Co-authored-by: Thomas Schaller <torkleyy@gmail.com>

1220: Introduce amethyst_error r=omni-viral,torkleyy,LucioFranco a=udoprog Following #1214 This introduces the minimal API necessary to convert existing Error types (some which includes causes). It currently mimics the `failure::Error` API, with methods named according to the ["fix Error" RFC](https://github.com/rust-lang/rfcs/blob/master/text/2504-fix-error.md). That means: * We include the `format_err!` macro and the `ResultExt` trait with an implementation of `with_context` from `failure`. * The error type includes backtraces directly (instead of wrapping it in through e.g. `Context<D>`) in comparison to `failure`. * We include a blanket `From<E> where E: std::error::Error` implementation to convert any type that implements `std::error::Error` into `Error`. * Note: this has the side-effect that this `Error` can't implement `std::error::Error`. * We are using `source` from `std::error::Error` instead of `cause`. * We don't implement `std::error::Error` directly, but we do implement some methods which are similar (`source`). We implement `as_error` to access a reference to the underlying `std::error::Error`. Similarly to how `failure::Error` doesn't implement `Fail` directly but provides `as_fail`. Note that since this RFC is so sparse, there's really not much to implement, and most other methods which are used to actually build errors are local to this crate (e.g. `ResultExt::with_context`). I have a branch where I've tried to apply this crate here: https://github.com/udoprog/amethyst/commit/amethyst-error-full I think I might have accidentally botched some of the `From<T>` implementations, but it should still give an idea of how this type is used. #### Composite APIs Composite APIs are APIs which potentially make use of local error information. Today these tend to lead to "stringy" errors where most of the underlying error information is lost and the errors undergoes a conversion. These are candidates to make use of `amethyst_error::Error` directly right now. The traits I've so far identified that has a tendency to lead to this are: * [`amethyst_assets::Source`](/amethyst/amethyst/blob/master/amethyst_assets/src/source/mod.rs) * [`amethyst_assets::PrefabData`](/amethyst/amethyst/blob/master/amethyst_assets/src/prefab/mod.rs) * [`amethyst_assets::SimpleFormat`](/amethyst/amethyst/blob/master/amethyst_assets/src/asset.rs) * Here's an example where SimpleFormat "stringifies" errors instead of propagating them to work around error constraints: https://github.com/amethyst/amethyst/blob/master/amethyst_ui/src/prefab.rs#L736 * [`amethyst_renderer::Pass/Pipeline` stuff](/amethyst/amethyst/blob/master/amethyst_renderer/src/pipe/pass.rs). Unsure because it spans over to UI and I don't know how "end user" this is. #### Open Discussions * Dropping the `Sync` constraint ([comment](#1220 (comment))). * ~~Either drop `no_std` support or improve it ([comment 1](#1220 (comment)), [comment 2](#1220 (comment) backtrace is no longer optional. Co-authored-by: John-John Tedro <udoprog@tedro.se> Co-authored-by: Thomas Schaller <torkleyy@gmail.com>

1220: Introduce amethyst_error r=omni-viral,torkleyy,LucioFranco a=udoprog Following #1214 This introduces the minimal API necessary to convert existing Error types (some which includes causes). It currently mimics the `failure::Error` API, with methods named according to the ["fix Error" RFC](https://github.com/rust-lang/rfcs/blob/master/text/2504-fix-error.md). That means: * We include the `format_err!` macro and the `ResultExt` trait with an implementation of `with_context` from `failure`. * The error type includes backtraces directly (instead of wrapping it in through e.g. `Context<D>`) in comparison to `failure`. * We include a blanket `From<E> where E: std::error::Error` implementation to convert any type that implements `std::error::Error` into `Error`. * Note: this has the side-effect that this `Error` can't implement `std::error::Error`. * We are using `source` from `std::error::Error` instead of `cause`. * We don't implement `std::error::Error` directly, but we do implement some methods which are similar (`source`). We implement `as_error` to access a reference to the underlying `std::error::Error`. Similarly to how `failure::Error` doesn't implement `Fail` directly but provides `as_fail`. Note that since this RFC is so sparse, there's really not much to implement, and most other methods which are used to actually build errors are local to this crate (e.g. `ResultExt::with_context`). I have a branch where I've tried to apply this crate here: https://github.com/udoprog/amethyst/commit/amethyst-error-full I think I might have accidentally botched some of the `From<T>` implementations, but it should still give an idea of how this type is used. #### Composite APIs Composite APIs are APIs which potentially make use of local error information. Today these tend to lead to "stringy" errors where most of the underlying error information is lost and the errors undergoes a conversion. These are candidates to make use of `amethyst_error::Error` directly right now. The traits I've so far identified that has a tendency to lead to this are: * [`amethyst_assets::Source`](/amethyst/amethyst/blob/master/amethyst_assets/src/source/mod.rs) * [`amethyst_assets::PrefabData`](/amethyst/amethyst/blob/master/amethyst_assets/src/prefab/mod.rs) * [`amethyst_assets::SimpleFormat`](/amethyst/amethyst/blob/master/amethyst_assets/src/asset.rs) * Here's an example where SimpleFormat "stringifies" errors instead of propagating them to work around error constraints: https://github.com/amethyst/amethyst/blob/master/amethyst_ui/src/prefab.rs#L736 * [`amethyst_renderer::Pass/Pipeline` stuff](/amethyst/amethyst/blob/master/amethyst_renderer/src/pipe/pass.rs). Unsure because it spans over to UI and I don't know how "end user" this is. #### Open Discussions * Dropping the `Sync` constraint ([comment](#1220 (comment))). * ~~Either drop `no_std` support or improve it ([comment 1](#1220 (comment)), [comment 2](#1220 (comment) backtrace is no longer optional. Co-authored-by: John-John Tedro <udoprog@tedro.se> Co-authored-by: Thomas Schaller <torkleyy@gmail.com> Co-authored-by: udoprog <udoprog@tedro.se>

torkleyy · 2019-02-13T19:21:35Z

@udoprog I think this can be closed, right?

udoprog · 2019-02-15T11:38:31Z

@torkleyy yeah, you're right.

udoprog mentioned this issue Dec 5, 2018

Introduce amethyst_error #1220

Merged

udoprog added type: improvement An improvement or change to an existing feature. project: core labels Dec 10, 2018

fhaynes added team: documentation team: engine and removed project: core labels Jan 8, 2019

udoprog closed this as completed Feb 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support causes in amethyst::Error #1214

Support causes in amethyst::Error #1214

udoprog commented Dec 4, 2018

torkleyy commented Dec 4, 2018

udoprog commented Dec 4, 2018

torkleyy commented Dec 4, 2018 •

edited

torkleyy commented Dec 4, 2018 •

edited

azriel91 commented Dec 4, 2018

LucioFranco commented Dec 4, 2018 •

edited

torkleyy commented Dec 4, 2018 via email

LucioFranco commented Dec 4, 2018

Evolution

fu5ha commented Dec 4, 2018

LucioFranco commented Dec 4, 2018

torkleyy commented Dec 4, 2018

LucioFranco commented Dec 4, 2018

LucioFranco commented Dec 4, 2018

torkleyy commented Dec 4, 2018

LucioFranco commented Dec 4, 2018

udoprog commented Dec 4, 2018 •

edited

LucioFranco commented Dec 4, 2018 •

edited

LucioFranco commented Dec 4, 2018 •

edited

torkleyy commented Feb 13, 2019

udoprog commented Feb 15, 2019

Support causes in amethyst::Error #1214

Support causes in amethyst::Error #1214

Comments

udoprog commented Dec 4, 2018

Causes

Backtraces

torkleyy commented Dec 4, 2018

udoprog commented Dec 4, 2018

torkleyy commented Dec 4, 2018 • edited

torkleyy commented Dec 4, 2018 • edited

azriel91 commented Dec 4, 2018

LucioFranco commented Dec 4, 2018 • edited

torkleyy commented Dec 4, 2018 via email

LucioFranco commented Dec 4, 2018

Evolution

fu5ha commented Dec 4, 2018

LucioFranco commented Dec 4, 2018

torkleyy commented Dec 4, 2018

LucioFranco commented Dec 4, 2018

LucioFranco commented Dec 4, 2018

torkleyy commented Dec 4, 2018

LucioFranco commented Dec 4, 2018

udoprog commented Dec 4, 2018 • edited

LucioFranco commented Dec 4, 2018 • edited

LucioFranco commented Dec 4, 2018 • edited

torkleyy commented Feb 13, 2019

udoprog commented Feb 15, 2019

torkleyy commented Dec 4, 2018 •

edited

torkleyy commented Dec 4, 2018 •

edited

LucioFranco commented Dec 4, 2018 •

edited

udoprog commented Dec 4, 2018 •

edited

LucioFranco commented Dec 4, 2018 •

edited

LucioFranco commented Dec 4, 2018 •

edited