Error handling with failure (using Error and ErrorKind) by hashmap · Pull Request #713 · mimblewimble/grin

hashmap · 2018-02-16T15:14:32Z

This PR adresses #166

We explored two approaches:

Custom Fail type https://boats.gitlab.io/failure/custom-fail.html
Error and ErrorKind pair https://boats.gitlab.io/failure/error-errorkind.html

This PR illustrate the second approach in the wallet code, see a separate PR for the first one. This approach changes a lot in error handling but potentially is more flexible. It allows n:m mapping of underlying errors to the wallet errors.

This is work in progress, the code can't be merged or even compiled (2 errors outside wallet module) we'd like to get some initial feedback before investing more efforts into it.

ignopeverell · 2018-02-16T21:11:41Z

I would favor this over #712 but would like to hear what @antiochp, @yeastplume and @quentinlesceller would have to say as well.

Sorry for the stupid question, I'm not familiar with failure (yet) and the docs weren't really explicit: how does one specify whether a failure captures its stacktrace?

hashmap · 2018-02-16T22:17:01Z

@ignopeverell I agree, this approach is more promising but requires more efforts. The author of failure mentioned that it could be (more) automated in the future.

There is no such thing as stupid question. It follows the panic convention - if RUST_BACKTRACE is set a stacktrace will be generated https://boats.gitlab.io/failure/fail.html#backtraces

hashmap · 2018-02-16T22:25:45Z

Forgot to mention that we had to implement sstd::error::Error for errors in keychain and transaction. It's temporary solution until it's converted to failure.

Sign in to view


 fn single_send_partial_tx(url: &str, partial_tx: &PartialTx) -> Result<PartialTx, Error> {
-	let mut core = reactor::Core::new()?;
+    let mut core = reactor::Core::new().context(ErrorKind::Hyper)?;


Sign in to view

-	fn from(e: hyper::Error) -> Error {
-		Error::Hyper(e)
-	}
+impl From<ErrorKind> for Error {


Sign in to view

-				"Could not remove wallet lock file. Maybe insufficient rights?"
-			))
-		})?;
+		fs::remove_file(lock_file_path).context(ErrorKind::WalletData("Could not remove wallet lock file. Maybe insufficient rights?"))?;


antiochp

Yeah I think this makes sense.
I suspect this is something we would need to use for a while before fully understanding the benefits...
👍

hashmap · 2018-02-18T23:46:27Z

Thanks for nice and helpful feedback. Merged with the current master. We will check couple of places where .map_err() is still used. Need some help with verifying context in each case, could be incorrect.

hashmap · 2018-02-20T21:49:50Z

It's ready for review.

antiochp

Yeah I like this.

antiochp · 2018-02-28T15:21:20Z

@hashmap looks like there are some conflicts to clean up.
Lets get this merged in if @ignopeverell and @yeastplume are happy with this approach.

yeastplume · 2018-02-28T16:11:44Z

seems to makes sense, very much in favour of centralising error handling code better and even though this looks a bit boilerplate heavy to set up, it looks like it will ultimately make error handling easier to follow.. will be much better informed after trying to use it properly for a while, but I’m okay giving this approach a try

antiochp · 2018-02-28T17:55:22Z

Sounds like everyone is on board? I'm going to go ahead and merge.

heunglee · 2018-03-16T22:00:40Z

While I am working on taking iron out of wallet lib, I noticed the changes on error handling. Using failure::Fail in wallet seems not fully embraced by Api library yet with respect to client/server error handling. At this moment, I map Api::Error into Http response:
Error::Argument -> Http::StatusCode::BadRequest
Error::Internal -> Http::StatusCode::InternalServerError
Error::NotFound -> Http::StatusCode::NotFound
and vise versa on the client side.

Sine I am not familiar with failure, I am not sure how to map Fail(e) on coinbase handler in wallet into Http status code. e is of Api::Error. So for now, I will comment the part for later visit and just map e into http status code. Any opinion and thoughts?

hashmap · 2018-03-22T23:50:20Z

@heunglee I'd suggest to map ErrorKind's variants (https://github.com/mimblewimble/grin/blob/master/wallet/src/types.rs#L76) to HTTP status codes
here is an example https://github.com/mimblewimble/grin/blob/master/grin/tests/framework/mod.rs#L339

This PR adresses mimblewimble#166 Error handling in wallet was ported to failure in mimblewimble#713 Using the same error model makes wallet code simpler and may simplify migration to Hyper.

This PR adresses #166 Error handling in wallet was ported to failure in #713 Using the same error model makes wallet code simpler and may simplify migration to Hyper.

…#713) * Initial version * store failure parameters inside ErrorKind variants * continue failure transformation * 4 errors left * still two errors * return old code back * finally compiling * Fix compilation and test errors after merge

This PR adresses mimblewimble#166 Error handling in wallet was ported to failure in mimblewimble#713 Using the same error model makes wallet code simpler and may simplify migration to Hyper.

…#713) * Initial version * store failure parameters inside ErrorKind variants * continue failure transformation * 4 errors left * still two errors * return old code back * finally compiling * Fix compilation and test errors after merge

This PR adresses mimblewimble#166 Error handling in wallet was ported to failure in mimblewimble#713 Using the same error model makes wallet code simpler and may simplify migration to Hyper.

Alexey Miroshkin and others added 7 commits February 1, 2018 16:12

Initial version

8e84929

store failure parameters inside ErrorKind variants

19e58d3

continue failure transformation

a2ca9f1

4 errors left

6f6eb9d

still two errors

176d63e

return old code back

fb9fc90

finally compiling

b5d68a5

antiochp reviewed Feb 17, 2018

View reviewed changes

Comment thread wallet/src/client.rs

fn single_send_partial_tx(url: &str, partial_tx: &PartialTx) -> Result<PartialTx, Error> {

let mut core = reactor::Core::new()?;

let mut core = reactor::Core::new().context(ErrorKind::Hyper)?;

This comment was marked as spam.

Sign in to view

antiochp reviewed Feb 17, 2018

View reviewed changes

Comment thread wallet/src/types.rs

fn from(e: hyper::Error) -> Error {

Error::Hyper(e)

}

impl From<ErrorKind> for Error {

This comment was marked as spam.

Sign in to view

antiochp reviewed Feb 17, 2018

View reviewed changes

Comment thread wallet/src/types.rs

"Could not remove wallet lock file. Maybe insufficient rights?"

))

})?;

fs::remove_file(lock_file_path).context(ErrorKind::WalletData("Could not remove wallet lock file. Maybe insufficient rights?"))?;

This comment was marked as spam.

Sign in to view

antiochp reviewed Feb 17, 2018

View reviewed changes

Alexey Miroshkin added 2 commits February 18, 2018 23:42

Merge with the current master

7d1d477

Fix compilation and test errors after merge

36c5dcd

hashmap mentioned this pull request Feb 18, 2018

[WIP] Error handling with failure #712

Closed

hashmap changed the title ~~[WIP] Error handling with failure (using Error and ErrorKind)~~ Error handling with failure (using Error and ErrorKind) Feb 20, 2018

antiochp approved these changes Feb 28, 2018

View reviewed changes

merge with the latest changes

d868f7f

antiochp merged commit 9e11afe into mimblewimble:master Feb 28, 2018

hashmap mentioned this pull request Apr 9, 2018

Error handling using failure in API #949

Merged

yeastplume pushed a commit that referenced this pull request Apr 16, 2018

Error handling using failure in API (#949)

b28de95

This PR adresses #166 Error handling in wallet was ported to failure in #713 Using the same error model makes wallet code simpler and may simplify migration to Hyper.

hashmap deleted the try-failure branch September 4, 2018 08:54

Uh oh!

Conversation

hashmap commented Feb 16, 2018

Uh oh!

ignopeverell commented Feb 16, 2018

Uh oh!

hashmap commented Feb 16, 2018

Uh oh!

hashmap commented Feb 16, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

antiochp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashmap commented Feb 18, 2018

Uh oh!

hashmap commented Feb 20, 2018

Uh oh!

antiochp left a comment

Choose a reason for hiding this comment

Uh oh!

antiochp commented Feb 28, 2018

Uh oh!

yeastplume commented Feb 28, 2018

Uh oh!

antiochp commented Feb 28, 2018

Uh oh!

heunglee commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hashmap commented Mar 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

antiochp left a comment •

edited

Loading

heunglee commented Mar 16, 2018 •

edited

Loading