A better Client Error experience

[seanmonstar]

As discussed in #1128, there is good reason to consider having 2 different error types, and they would be useful for the Client as well. The hyper::Error is the correct error type for the Client to return from its Future, as well as from the response's body stream.

However, It does not makes sense for the request body stream to require sending a hyper::Error if generating the body failed. In that case, nothing in hyper failed, and the only relevant thing to hyper is that the stream cannot be completed. So the error type sent should be some sort of Disconnect or Abort or whatever, since an error on the body stream means the only thing hyper can reasonably do is to give up on the connection.

As in the server issue, the From<std::io::Error> and From<hyper::Error> implementations would be useful to allow easily sending streams from other sources, but it would be clear that the actual error type means "kill it dead."

---

New proposal: #1431

[sanmai-NL]

Every function/method can have its own error type. More general abstractions may seem reasonable now, but have such a large API surface that revising error handling will break code all over the place. The abstraction of Errors as being server or client-related, or problematic vs. not really problematic (your request body streaming comment) is not needed in my view.

[seanmonstar]

@sanmai-NL I don't quite understand which way you lean. You say each method can have its own type, but that it's not needed in hyper.

[sfackler]

One thing I'd like is to differentiate IO errors based on where they came from. I care about connect errors in particular, since you can retry the request on a different host if you can't connect to one of them, even if the request isn't idempotent or you have a body you can't reset.

[seanmonstar]

I made a comment in #1131 about having an internal Connect variant (and related ways to inspect for that).

[sanmai-NL]

@seanmonstar, regarding #1130 (comment):
I’m saying the current abstract distinction between Server and Client in the API is not ‘needed’ w.r.t. to error handling. With ‘needed’, I meant to say that it works better to have an error type for every fallible function. That’s simple and always works.

#[derive(Copy, Clone, Debug)]
pub enum MyFnError {
    FailurePathOne { cause: OtherFnError },
}

impl Display for MyFnError {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        fmt::Display::fmt(match *self {
            FailurePathOne::OtherFnError { .. } => "Failed, ... . ",
        }, f)
    }
}

impl Error for MyFnError {
    fn description(&self) -> &'static str { "" }

    fn cause(&self) -> Option<&Error> {
        match *self {
            MyFnError::FailurePathOne { ref cause, .. } => Some(cause),
        }
    }
}

pub type MyFnResult = Result<(), MyFnError>;

[dekellum]

Edit: What follows was ill-informed on my part, but gives an example of how a new and foolish user might make the mistake of trying to instantiate a hyper::Error. See comment below.

Hi, I'm not sure this is still open for feedback or where its progressing, but given my evolving use case, I'm surprised by this opening comment:

The hyper::Error is the correct error type for the Client to return from its Future, as well as from the response's body stream.

As of hyper 0.11 this is of course a requirement, but I'm finding that awkward. I'm implementing a client that needs to be resilient to retrieving large responses up to ~1GiB in size and processing these with more than one pass. After retrieving a threshold size the client will switch from in-memory to buffering the response on disk. In addition the client may wish to terminate response streaming early if the initial Content-Length header, or the actually received chunked response, is too large (for example, given available disk space.) I don't want to panic in these cases, but I do want the body stream to terminate and hyper to close the connection (which won't be completely read). These kinds of errors are entirely in the domain of my application, but as it stands, my best route is along the lines of:

if length_to_read > self.max_payload() {
    Err(IoError::new(
        ErrorKind::Other,
        format!("[APP] response body too large: {}+", length_to_read)
    ).into()) // -> hyper::Error
} //...

Also my disk based implementation can io::Error, but as it stands its going to be difficult without hacks like the above to reliably distinguish between I/O errors originating from the HTTP sockets and my disk impl. I/O errors. But distinguishing these is important to determine which should be retried vs. resulting in a fatal application error.

My personal ideal would be the ability to use the failure crate's Error type inside of FutureResponse::and_then and Body::for_each, or some kind of trait object. If that is too high level or costly, then how about an AppError(u32) variant in the hyper::Error enum or a reserved application error range in a numeric hyper error?

[sfackler]

@dekellum Just because the Client's future returns a hyper::Error doesn't mean your future that works off of that has to return a hyper::Error. client.get("http://google.com").map_err(|hyper_error| MyErrorType::from(hyper_error)).

[dekellum]

Yes, thanks, but like with my above code example originating in hyper::Body::for_each(Fn) I would end up hoping(!) I'm the only one using ErrorKind::Other or prefix matching on "[APP]" in order to convert the sub-set of all possible errors that are my own applications errors. Its a hack at best.

[sfackler]

Like this?

response.body().map_err(|hyper_error| MyErrorType::from(hyper_error)).for_each(Fn)

[dekellum]

No, unfortunately the logic that decides upon and produces the error needs to be in the Fn itself, which is defined by hyper to return a Future with type Error=hyper::Error.

[sfackler]

I don't understand. for_each passes the closure a Chunk, not a Result. It never sees the error that is transformed by the map_err.

[dekellum]

I'll try and get to where I can show more of my code, and my apologies if I'm explaining myself badly, but the for_each passed Fn argument must (per Hyper) return a Future::<Item=(), Error=hyper::Error>. So I have my own application errors in here that I need to shoehorn through a hyper::Error and transform into something better outside, when the future is resolved.

[sfackler]

You can use map_err to transform the error type of the stream. The Fn passed to response.body().map_err(|e| MyErrorType::from(e)).for_each(Fn) must return a Future<Item=(), Error=MyErrorType>, not a Future<Item=(), Error=hyper::Error>. This is specified by the futures crate's Stream trait, not hyper.

[dekellum]

@sfackler: thanks for your patient suggestions. I now finally get it, and its much better! I was missing the wrapping nature of Future/Stream map_err, and then the rustdoc for the hyper::Body stream with associated Error type set as hyper::Error gives the impression there is no further option:

type Error = [hyper::]Error
The type of error this stream may generate.
[...]
fn for_each<F, U>(self, f: F) -> ForEach<Self, F, U>
where
F: FnMut(Self::Item) -> U,
U: IntoFuture<Item = (), Error = Self::Error>,

So now the issues opening comment makes perfect sense and I'll edit down my prior ill-informed input. Since the issue title includes experience I would just suggest that somewhere between futures' or hyper's rustdoc and/or a hyper client demo with error handling including map_err and futures::future::err could save the next foolish user about 1.5 days!

Thanks again and sorry for the noise.

[seanmonstar]

As I've been working on HTTP/2 support in hyper, I realized that there is a similar situation in h2, which is that one could at anytime send a RST_STREAM frame. A user could definitely trigger a request, and spawn a long read of something, and if at some point that read fails, just want to tell the end point "welp, things blew up, ignore this stream". In h2, that could be done without killing the connection, but just sending a reset.

In HTTP/1, the only action would be to close the connection, but that's fine.

The point here is that this concept exists in HTTP/2, and could perhaps be used for both client and server errors. It could be a hyper::error::Reset, but that might be too specific to HTTP/2, or might mislead in thinking that it's only for HTTP/2, or that the connection will survive (even it's HTTP/1). So, we may want a different name, but I'm now thinking that for client request bodies, the error type would be this Reset type.

Or perhaps even better, where B: Stream, Reset: From<B::Error>. Then the bigger hyper::Error type could be passed, for piping a stream from another request or response, but it'd also be clearer by looking at the documentation for Reset that you just use Reset::internal() (or whatever, naming), instead of looking at the variants of hyper::Error and wondering which to pick.

Also, this type would never be given to the user, only created by the user to signal a stream errored. I wrote up more of the design in #1431.