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

[mclark4386]

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]

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

[mclark4386]

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]

you mean try! 😉

[yanns]

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

[hjr3]

In order to use try!, the run method needs to return an error. In this case, it looks like Core::new uses io::Result. So the run method could return something like io::Result<()>.

[hjr3]

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

[mclark4386]

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::Fromstd::ffi::NulError>
= help: <std::io::Error as std::convert::From<std::io::IntoInnerError>>
= help: <std::io::Error as std::convert::Fromstd::io::ErrorKind>
= 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]

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

[hjr3]

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

[yanns]

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.

[hjr3]

The try! followed by Ok is redundant. What the try! macro expands to is something like:

match core.run(work) {
   Err(e) => return e,
   Ok(o) => o
}

so you can get rid of the Ok(()) and have core.run(work). Notice there is no semi-colon. However, that will not fix the compiler error shown in travis-ci.

I originally told you the wrong return type for Core::run. With the version of tokio-core that weldr is running on, core.run(work) returns Result<F::Item, F::Error> which is the Item and Error type of the future. Because the work future sent to core.run maps all errors to (), we cannot use try! here One thing you could do is:

core.run(work).map_err(|_| io::Error::new(io::ErrorKind::Other, "Core failed"))

The better thing to do would be to change map_err at the end of the work future .map_err(From::from). This says to take any error returned by the timer.interval stream and call std::convert::From::from on the result. You can also write this using a more explicit closure .map_err(|e| From::from(e)). This works because the work future returns a tokio_timer::TimerError type if there is an error. The tokio-timer crate implemented the std::convert::From trait for tokio_timer::TimerError to be converted to io::Error. (btw, I didn't go and read the source code for tokio-timer to figure that out. I mapped the error to From::from and crossed my fingers that it was already implemented. it is a common pattern).

One more error to go! When you have try!(server.url().join(&self.uri_path)); the compiler is complaining that server.url().join(&self.uri_path) returns a hyper::error::ParseError type on error but timer.interval(self.interval).for_each() is expecting a tokio_timer::TimerError. Unlike our previous example, the hyper crate does not know how to convert hyper::error::ParseError into tokio_timer::TimerError. If you look at the [TimerError](https://github.com/tokio-rs/tokio-timer/blob/f044bd2f58d1192aff774eac9ee06852fd90a5a7/src/timer.rs#L41) in source code it makes sense. There is no logical conversion of a url parsing error into a timeout error. Instead of using try!` and bubbling up the error, we may want to just handle it here. You might think about doing something like:

let url = match server.url().join(&self.uri_path) {
    Ok(url) => url,
    Err(e) => {
        error!("Invalid health check url: {:?}", e);
        return Ok(());
    }
};

The main difference here is that we are returning the Ok variant instead of the Err variant. If we think about this, it makes sense. timer.interval(...).for_each is of type Stream from the futures crate. If we return an Err, then the entire stream is halted immediately. We probably do not want to halt the entire stream here, but instead abort this particular server. I say probably because this is one of those errors that only happens if either a) the server url is not valid or b) the uri path is not valid. In either case, it is user supplied data that has to be fixed.

I did not expect my comment to get so long. I appreciate your work here. If nothing else, it has forced me to think through how these interactions need to work. You are very close to getting it working! I also agree with @yanns that maybe a custom error type might be good here. However, if you want to get this working as-is for now, we can always put in the custom error type later.

[mclark4386]

@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]

@mclark4386 this is the long post: #81 (comment)

[mclark4386]

@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]

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

[mclark4386]

@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!^^