Error type redesign

[seanmonstar]

The Error type in hyper is currently an enum, wrapping up the various cases of things that could go wrong when parsing incoming HTTP data. Some of the problems with the current design:

• Being an enum means that adding variants is a breaking change. There is a __Nonexhaustive variant to try to warn that exhaustive matches can cause breaking changes, but that it's better when internals are hidden.
• Users sometimes try to create hyper::Errors, even though they really shouldn't. It is meant only for saying "this thing went wrong while hyper was parsing HTTP", and nothing else. Part of this can be fixed also by changing the actual error types used, such as in outgoing Streams.

Instead, I'd like to propose making the Error an opaque struct, with some methods to inspect the type and details of an error, somewhat along the lines of std::io::Error.

(Still thinking through the specific code, will add a design section later).

[mp4096]

Have you considered using error-chain? TBH I didn't use it before, but it sounds a lot like what you want to do.

Anyway, maybe @brson himself could say if error-chain might be helpful in your case?

[KodrAus]

Can you declare the ErrorKind enum in error-chain to be private? error-chain does have the nice benefit of collecting a backtrace from the error's source.

[seanmonstar]

A backtrace is neat (but expensive?). I'm not sure I'd want to expose any enum at all with regards to the Error or ErrorKind, because of the inability to grow it. I'd reconsider when and if Rust ever grows some form of private variants (or however is used to prevent exhaustive matches).

It's worth thinking about the things some would do with an Error from hyper.

• Just log it. So it needs fmt::Debug and fmt::Display.
• Try to send a proper Response depending on the error that occurred? Like, return 431 when there was a Error::TooManyHeaders, perhaps a 414 if there were Error::TooLarge, and a 400 with other parse errors.
• Maybe check for certain io::Errors, to allow an application to adapt, such as if the error involves too many file descriptors open, or something.

Anything else?

I'm thinking of having methods on the type to allow inspecting the type of error it was, since it's always possible to add more methods. Like, error.is_uri_too_long() or whatever.

[sanmai-NL]

@seanmonstar: I do not think it is a problem if programmers do exhaustive matching on the enum that implements Error. Yes, their code will need some small fixes, but only if they upgrade their hyper dep. If a new Error variant can be returned and exhaustive matching is done, they probably have to alter the logic in their own code anyway, no?

I use Error enums extensively (in closed source code), I did not encounter ergonomic or API stability issues.

Update:
Also, note that I do not use error-chain. Producing non-technical but complete ‘backtraces’ required very little boilerplate code and could be abstracted away easily. Hopefully, you can consider my comment as a data point that keeping things simple and primitive (keeping the status quo wrt. the issue reported) may be a wiser, be it conservative choice.

Update:
I was unclear in my last sentence. I mean keeping the status quo or better yet, improving error handling in a completely different direction, not splitting hyper::Error into a client and server-related Error.

[KodrAus]

@sanmai-NL That's interesting, are you able to share a bit about what you do with specific hyper::Error variants? In the code I've written using hyper I don't think I've had a case where I could do something useful with any specific variant of Error, just shrug and pass it along. But that could definitely just be me so I'm interested to know how other people hold it.

[sanmai-NL]

No sorry, I do nothing with hyper::Error variants, did you understand my comment as if I do? I do retain such Error values as the cause field (of some struct variants from some custom Error enum) for my own backtraces. What I am trying to point out is that having one Error type to cover lots of code (e.g. the current hyper::Error, as well as the proposed split into server and client errors) seems less productive to me than having specific Error types. The latter can easily remain enums, since they are specific to some function scope and only break exhaustive matches on values of that specific Error type (i.e., return values of that particular function).

[dtolnay]

That's interesting, are you able to share a bit about what you do with specific hyper::Error variants?

Maybe translate error messages into another language.

[sanmai-NL]

Another general comment: in error handling the viewpoint of regular people needs to be taken into account. I have found this requires tight developer control over error reporting. Backtraces etc. are not very user friendly (may even instill fear), and leak information about the source code, which from a business perspective is intellectual property. For hyper, the user faces the web content and I think hyper has an adequate API to return HTTP status codes and nice error pages.

[KodrAus]

Maybe translate error messages into another language.

@dtolnay Oh that's interesting... I've never thought of translating errors before. I guess you could do that on an arbitrary text stream though, rather than individual error variants?

What I am trying to point out is that having one Error type to cover lots of code ... seems less productive to me than having specific Error types.

@sanmai-NL I have the opposite opinion on this with respect to libraries like hyper that are consumed by other libraries. I think having multiple error types that span a single use-case adds boilerplate when trying to chain errors, and cognitive load when consuming them. Now instead of one error type to deal with you've got potentially many. Since the server and client use-cases are distinct splitting them seems reasonable to me.

For application code though I agree having an individual error type per operation is nicer to manage.

Backtraces etc. are not very user friendly (may even instill fear), and leak information about the source code ...

I think that's a good point. Returning backtraces in responses isn't very good. But having backtraces in logging is very good, and makes issues much easier to diagnose. This is especially true in Rust where you could be handling an error downstream from where it was produced, so the backtrace you get out-of-the-box might be really unhelpful. Maybe for hyper having a backtrace somewhere in its internals isn't any more useful than a backtrace where you receive it? But that could be made a concern for application logging rather than on error types directly.

As far as mapping a hyper response to a http response in a server goes I thought that would be done by hyper directly, rather than by callers? If that's the case then the ergonomics of inspecting the error don't matter so much. If not then maybe we should look at how that code would change with this new design and see if it becomes clunky.

[seanmonstar]

I do not think it is a problem if programmers do exhaustive matching on the enum that implements Error.

It is a bit of a problem. With exhaustive matching, that means that adding a new error case is a breaking change. For instance, if at some point it was desirable to separate the Error::TooLarge into a UriTooLong and HeaderValueTooLong, that would break other people's code from compiling. If hyper were at 1.0, that would be mean needing to go to 2.0 just because we changed an internal detail of tracking errors.

Maybe translate error messages into another language.

A possibility is Error::code() -> u64 (maybe a smaller number type, probably not going to have trillions of error codes). As a number, you'd have to treat the case of future codes specifically, so adding new ones should be fine.

Backtraces etc. are not very user friendly (may even instill fear), and leak information about the source code

This definitely true if someone were to print the a hyper::Error directly back on a webpage. A worthy thing to consider regarding the use of backtraces in hyper.

What I am trying to point out is that having one Error type to cover lots of code seems less productive to me than having specific Error types.

That's actually part of the point of this issue. There are 2 rather distinct cases of failure in when using a Service (Server or Client), and using hyper::Error in both cases is odd.

[seanmonstar]

Relevant to the design of type, from this comment:

One thing I'd like is to differentiate IO errors based on where they came from. I care about connect errors in particular [...]

In terms of internal variants, things I see that would be useful to group are:

• Connect
• Parse
• Io