search

tailhook / rotor

The mio-based framework for rust for doing I/O in simple and composable way (ABANDONED)
MIT License
361 stars 25 forks source link

Document Response::error and fix `log_errors` feature #19

Closed TimNN closed 8 years ago

TimNN commented 8 years ago

Cargo features are available with feature = "..." in cfg's (Source).

tailhook commented 8 years ago

Merged, as it won't hurt probably.

Overall I'm not sure whether log_errors feature is a good idea. I used to think that I don't want to depend on log crate by default, but it's not the case, because (a) mio already depends on log and (b) when doing if cfg!() all macros that are used inside the if block must be defined regardless of the result of cfg!() expression.

Do you have any thoughts on the feature?

TimNN commented 8 years ago

I believe conditional dependence on the log crate could be solved by providing the various logging macros ourselves if the feature is not enabled (ie. #[cfg(not(feature = "log"))] macro_rules! debug{ ($($foo:tt)*) => {} } or something).

Apart from that I'm unsure how to handle this best. If Response::error actually takes an error as an argument, something should happen with it.

Until I looked at the source I spend some time trying to figure out what actually happens with the error I report. One thing I was looking / hoping for would be a way to register some kind of "error handler" which would be called whenever Response::error is returned. However in that case, one might as well report the error to the Context and handle it there.

I believe the best design would be to either

  1. Add an error method to Machine which gets called whenever a Response::error is returned or
  2. Remove the argument from Response::error and let the user handle it through the context.

Apart from the reporting, is there a functional difference between returning Response::error and Response::done ?

Being able to return Response::error with an argument would of course be pretty nice if the ? operator would support conversion between different kinds of results, which may happen at some point in the future.

tailhook commented 8 years ago

As far as I remember the idea was the following:

  1. There is an implementation of HTTP in rotor_http::client::Fsm which implements Machine trait (that is based on top of rotor_stream, but that's irrelevant).
  2. Then there is a rotor_stream::Persistent which establish TCP connection, and hands it off to the state machine (in this case client HTTP, but it's also used for carbon, redis...).
  3. Then if Machine dies, the Persistent should find out whether connection is closed successfully (e.g. received aConnection: close), or some error happened. This influences the reconnection policy.

So above is the case when behavior is different. In many other cases you can log error yourself and pass Response::done to the rotor.