hjr3 / weldr

A HTTP 1.1 proxy written in Rust using tokio.
Apache License 2.0
218 stars 20 forks source link

#71 Should now bubble errors up the scope in health.rs #81

Closed mclark4386 closed 7 years ago

mclark4386 commented 7 years ago

Please let me know if you would rather this be handled differently or if you would like anything tweaked! This is one of my first rust patches, so VERY open to any needed changes! Thanks!

mclark4386 commented 7 years ago

Wow... ok, I'm going to fix this... sorry about that!

mclark4386 commented 7 years ago

Going to close this until I can get it figured out^^; The Error types are getting me at the moment, but I'll get it figured out^^;

yanns commented 7 years ago

don't be impatient with yourself. Take your time, and ask for help if needed.

hjr3 commented 7 years ago

@mclark4386 this looks really close. i am hjr3 on irc (irc.mozilla.org) if you want to talk through anything.

mclark4386 commented 7 years ago

Thanks for your time! Yeah, I figured out that it needed to return a Result and then have been running into issues with the return type because it looks like there are different error types that it may return and they aren't compatible. Here are the errors I'm getting with the latest code:

error[E0277]: the trait bound tokio_timer::TimerError: std::convert::From<hyper::error::ParseError> is not satisfied --> src/health.rs:41:27 41 let url = try!(server.url().join(&self.uri_path)); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait std::convert::From<hyper::error::ParseError> is not implemented for tokio_timer::TimerError

= note: required by std::convert::From::from = note: this error originates in a macro outside of the current crate

error[E0277]: the trait bound std::io::Error: std::convert::From<()> is not satisfied --> src/health.rs:70:9 70 try!(core.run(work)); ^^^^^^^^^^^^^^^^^^^^^ the trait std::convert::From<()> is not implemented for std::io::Error

= help: the following implementations were found: = help: <std::io::Error as std::convert::From> = help: <std::io::Error as std::convert::From<std::io::IntoInnerError>> = help: <std::io::Error as std::convert::From> = help: <std::io::Error as std::convert::From<tokio_timer::TimeoutError>> = help: and 4 others = note: required by std::convert::From::from = note: this error originates in a macro outside of the current crate

error: aborting due to 2 previous errors

I could be reading that wrong, but it almost seems like the error type from the first error (tokio_timer::TimerError) doesn't have the functionality to convert to a compatible error for the io::Result and then the second error... I'm not sure. Maybe the same thing?

Thank you for being patient with me! I'm doing this on the side and have had a sudden surge at work so it's be hectic^^;

mclark4386 commented 7 years ago

Is there a way to trigger github to update the the view of the code here? Would reopening the issue do that?

hjr3 commented 7 years ago

@mclark4386 i opened the issue back up. i see travis running.

yanns commented 7 years ago

One idea: you could define your own error, like HealthCheckError to use on a HealthCheckResult instead of io:Result. Then implement some From to be able to convert the different errors to HealthCheckError.

mclark4386 commented 7 years ago

@hjr3 had a big long post and then accidentally left the page and lost it all when I came back. Main gist is that I was still having the first error when I did the custom error (HealthCheckError), but I guess I could have made your solution work for it, but thought your solution seemed more idiomatic... but I don't know. Either way I implemented your changes and I'm not positive how to fix that error (can see it in the stable log in travis). Thank you for your time and patience! I'm learning a ton thanks to this^^;

yanns commented 7 years ago

@mclark4386 this is the long post: https://github.com/hjr3/weldr/pull/81#discussion_r106672273

mclark4386 commented 7 years ago

@yanns sorry I meant that I had writen a big long post in reply to @hjr3 's post, but lost it all to my browser not remembering it after I accidentally clicked a link that took me to a different page.

hjr3 commented 7 years ago

@mclark4386 thank you for working on this. It was unexpectedly more complicated than I originally thought it would be!

mclark4386 commented 7 years ago

@hjr3 of course! Sorry I wasn't more helpful, but like I said I learn a lot from it, so thank you for help me through it! Thank you @yanns as well!^^