[wip] Implement error handling using error-chain #332

derekdreery · 2017-09-04T15:06:10Z

This PR demonstrates the kind of way you would use error-chain to handle errors, and replaces all calls to expect/unwrap in the code and examples. Where I've created new error names to catch errors that were previously panicking, I'm not sure I've picked the right name, or that I'm capturing the right data, but at least this PR gives you a sense of how error handling could work.

It collects any errors in the parallel draw call into a vector.

The examples have been updated to use quick_main macro, which allows the "main" function to return a Result. If you run with RUST_BACKTRACE=1 cargo run ..., then whenever an error-chain error is created, a backtrace is attached, and reported if using quick_main.

derekdreery · 2017-09-04T15:07:48Z

I'm not sure of the impact on performance - you may want to silently ignore errors in production to run faster.

Also note that I haven't looked at amethyst itself yet - currently only the renderer builds.

Rhuagh · 2017-09-04T18:30:05Z

Seems it's not working very well with ECS. Could you look into that?

derekdreery · 2017-09-04T18:33:06Z

Yes, at this point I'm just gathering opinions on the general strategy.

…

On 4 Sep 2017 19:30, "Simon Rönnberg" ***@***.***> wrote: Seems it's not working very well with ECS. Could you look into that? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#332 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABU-XlJr6S3ST4RPBM0j9kMFjC8y2Lkhks5sfEGvgaJpZM4PMBvh> .

Rhuagh · 2017-09-04T19:48:30Z

FWIW, I like it.

Xaeroxe

Looks quite informative! I like the approach. I can't merge this until at minimum CI passes, ideally I'd like this implemented for our other crates in this PR as well. Pinging @amethyst/engine-devs Is anyone opposed to this style of error management? Please leave a comment saying you're not if you are not so we don't just have total silence here.

Xaeroxe · 2017-09-05T01:34:59Z

amethyst_renderer/examples/material.rs

@@ -1,5 +1,6 @@
 //! Launches a new renderer window.

+#[macro_use] extern crate error_chain;


Odd, I've been writing mine as

#[macro_use] extern crate error_chain;

Is it documented anywhere what style these should be? I wasn't able to find documentation for this.

Note: I'm not requesting this be changed I just want to know if I've been doing mine wrong.

I typically have been doing it your way, but there are some merits to the one liner.

I'm completely indifferent - I'll change to the amethyst standard :)

Did you do cargo fmt?

Aceeri

Good stuff! Looks fine so far to me.

Aceeri · 2017-09-05T07:34:02Z

amethyst_renderer/examples/sphere.rs

 }

+quick_main!(run);


In the future we should replace this with main again: rust-lang/rust#43301

zakarumych · 2017-09-05T11:43:50Z

amethyst_renderer/src/error.rs

+error_chain! {
+    errors {
+        /// Failed to create a buffer.
+        BufferCreation(e: gfx::buffer::CreationError) {


According to docs here
when you wrap another error (not a chain) you put it into foreign_links block.
This will add From implementations automatically.

If you will wrap this chain in main crate you need to add it in links block

If you use foreign_links, you gain ergonomics, but lose the ability to control the description (description is forwarded from contained error). I'll implement it using foreign_links, but want to note the trade-off.

derekdreery · 2017-09-05T12:09:14Z

Thanks for the replies, I'll implement this for all the crates

torkleyy

Thanks for your work! Here are few things a noticed after reading over it.

torkleyy · 2017-09-08T09:12:00Z

amethyst_renderer/Cargo.toml

@@ -37,6 +37,7 @@ rayon = "0.8"
 serde = "1.0"
 serde_derive = "1.0"
 winit = "0.7"
+error-chain = "0.10"


There already is 0.11 btw

torkleyy · 2017-09-08T09:12:43Z

amethyst_renderer/examples/material.rs

@@ -1,5 +1,6 @@
 //! Launches a new renderer window.

+#[macro_use] extern crate error_chain;


Did you do cargo fmt?

torkleyy · 2017-09-08T09:14:28Z

amethyst_renderer/examples/material.rs

 }

+quick_main!(run);


Examples should, if possible, avoid macros IMO.

quick_main! could easily be replaced with how it is recommended to be done: link

fn main() { if let Err(ref e) = run() { use std::io::Write; let stderr = &mut ::std::io::stderr(); let errmsg = "Error writing to stderr"; writeln!(stderr, "error: {}", e).expect(errmsg); for e in e.iter().skip(1) { writeln!(stderr, "caused by: {}", e).expect(errmsg); } // The backtrace is not always generated. Try to run this example // with `RUST_BACKTRACE=1`. if let Some(backtrace) = e.backtrace() { writeln!(stderr, "backtrace: {:?}", backtrace).expect(errmsg); } ::std::process::exit(1); } }

Emilgardis · 2017-09-08T11:39:31Z

amethyst_renderer/src/error.rs

-use std::fmt::{Display, Formatter};
-use std::fmt::Result as FmtResult;
-use std::result::Result as StdResult;
-
 use gfx;
 use gfx_core;


This is not recommended error_chain usage. It is advised to use paths from root, i.e ::gfx_core::*

However, if it works I don't see any problems.

Emilgardis · 2017-09-08T11:41:28Z

amethyst_renderer/src/lib.rs

@@ -65,6 +65,11 @@
 #![deny(missing_docs)]
 #![doc(html_logo_url = "https://tinyurl.com/jtmm43a")]

+// for error-chain
+#![recursion_limit="4096"]


Why have 4096 as the limit?
error_chain recommends 1024, and if I remember correctly, newer rust versions have this limit already set to 1024.

derekdreery · 2017-09-08T15:36:01Z

I was having some issues with maxing out recursion. It might have been before they increased the limit, so I will check without the config line before I say everything's ready for review.

…

On Fri, Sep 8, 2017 at 12:43 PM, Emil Gardström ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In amethyst_renderer/src/error.rs <#332 (comment)>: > @@ -1,126 +1,70 @@ //! Renderer error types. -use std::error::Error as StdError; -use std::fmt::{Display, Formatter}; -use std::fmt::Result as FmtResult; -use std::result::Result as StdResult; - use gfx; use gfx_core; However, if it works I don't see any problems. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#332 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABU-XiY6bxFEd7stw_XenlYAwApNMx2Nks5sgSh0gaJpZM4PMBvh> .

zakarumych · 2017-09-19T10:57:10Z

This feature becomes rather important as hierarchy of errors in amethyst grows.

derekdreery · 2017-09-19T11:50:04Z

I'm currently trying to understand the asset loader, so I can do an error chain. I need to have a think, because at the moment errors are not concrete types.

zakarumych · 2017-09-19T14:32:45Z

errors are not concrete types.

@derekdreery what do you mean?

derekdreery · 2017-09-20T06:29:13Z

An example of a generic error would be

#[derive(Clone, Debug)]
pub enum LoadError<A, F, S> {
    /// The conversion from data -> asset failed.
    AssetError(A),
    /// The conversion from bytes -> data failed.
    FormatError(F),
    /// The storage was unable to retrieve the requested data.
    StorageError(S),
}

I'm still exploring where these are specialised into concrete types. If that's done in user code, it may be necessary to use trait objects. Another option would be to remove the generics from these errors, and let the user put anything implementing Error into the chain. It might be easier if I ask some questions, maybe on gitter? I'll have to see if I have time to work on this today.

zakarumych · 2017-09-20T06:38:02Z

@derekdreery LoadError is instantiated for Loader functions

Xaeroxe · 2017-09-28T17:17:41Z

Out of 12 files changed 10 now have conflicts. It'd probably be easier to rewrite this PR rather than try and re-merge it at this point. If you disagree with me @derekdreery I'll re-open this but for now I'm closing it.

For future PR authors: Large architecture overhauls that hit multiple function signatures and existing processes have to be completed quickly or they rapidly become changes to outdated code. This PR has been open for almost a month now.

derekdreery · 2017-09-28T23:18:13Z

I agree.

Improve error handling

97ad188

derekdreery mentioned this pull request Sep 4, 2017

Remove all expect and unwrap from source (where reasonable) #301

Closed

Xaeroxe reviewed Sep 5, 2017

View reviewed changes

Aceeri approved these changes Sep 5, 2017

View reviewed changes

zakarumych suggested changes Sep 5, 2017

View reviewed changes

Use foreign links for error

9ee751d

derekdreery changed the title ~~Implement error handling using error-chain~~ [wip] Implement error handling using error-chain Sep 5, 2017

torkleyy self-requested a review September 8, 2017 07:21

torkleyy suggested changes Sep 8, 2017

View reviewed changes

torkleyy added project: debugging type: improvement An improvement or change to an existing feature. labels Sep 8, 2017

Emilgardis approved these changes Sep 8, 2017

View reviewed changes

torkleyy mentioned this pull request Sep 12, 2017

Crashes on Intel HD cpu driver (cannot reproduce) #344

Closed

Xaeroxe closed this Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] Implement error handling using error-chain #332

[wip] Implement error handling using error-chain #332

derekdreery commented Sep 4, 2017

derekdreery commented Sep 4, 2017 •

edited

Rhuagh commented Sep 4, 2017

derekdreery commented Sep 4, 2017 via email

Rhuagh commented Sep 4, 2017

Xaeroxe left a comment

Xaeroxe Sep 5, 2017

Xaeroxe Sep 5, 2017

Aceeri Sep 5, 2017

derekdreery Sep 5, 2017

torkleyy Sep 8, 2017

Aceeri left a comment

Aceeri Sep 5, 2017

zakarumych Sep 5, 2017

zakarumych Sep 5, 2017

derekdreery Sep 5, 2017

derekdreery commented Sep 5, 2017 •

edited

torkleyy left a comment

torkleyy Sep 8, 2017

derekdreery Sep 19, 2017

torkleyy Sep 8, 2017

torkleyy Sep 8, 2017

Emilgardis Sep 8, 2017 •

edited by torkleyy

Emilgardis Sep 8, 2017

Emilgardis Sep 8, 2017

Emilgardis Sep 8, 2017

derekdreery commented Sep 8, 2017 via email

zakarumych commented Sep 19, 2017

derekdreery commented Sep 19, 2017

zakarumych commented Sep 19, 2017

derekdreery commented Sep 20, 2017 •

edited

zakarumych commented Sep 20, 2017

Xaeroxe commented Sep 28, 2017

derekdreery commented Sep 28, 2017

		@@ -1,5 +1,6 @@
		//! Launches a new renderer window.

		#[macro_use] extern crate error_chain;

		}

		quick_main!(run);

		}

		quick_main!(run);

[wip] Implement error handling using error-chain #332

[wip] Implement error handling using error-chain #332

Conversation

derekdreery commented Sep 4, 2017

derekdreery commented Sep 4, 2017 • edited

Rhuagh commented Sep 4, 2017

derekdreery commented Sep 4, 2017 via email

Rhuagh commented Sep 4, 2017

Xaeroxe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aceeri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekdreery commented Sep 5, 2017 • edited

torkleyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emilgardis Sep 8, 2017 • edited by torkleyy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekdreery commented Sep 8, 2017 via email

zakarumych commented Sep 19, 2017

derekdreery commented Sep 19, 2017

zakarumych commented Sep 19, 2017

derekdreery commented Sep 20, 2017 • edited

zakarumych commented Sep 20, 2017

Xaeroxe commented Sep 28, 2017

derekdreery commented Sep 28, 2017

derekdreery commented Sep 4, 2017 •

edited

derekdreery commented Sep 5, 2017 •

edited

Emilgardis Sep 8, 2017 •

edited by torkleyy

derekdreery commented Sep 20, 2017 •

edited