hyper::Error should distinguish http1 TooLarge from other parse errors

[acfoltzer]

Currently if an http1 message head exceeds the http1_max_buf_size, a hyper::Error is returned where err.is_parse() == true. This is indistinguishable from other errors that httparse might bubble up, which poses a problem for deciding how to respond to these errors. A TooLarge can occur just because of the choices made when setting up the max buffer size (many implementations default to 1MB vs the ~400KB default for hyper), vs. an actually malformed HTTP message which probably indicates a misbehaving server.

Could we add a new method to hyper::Error that distinguishes these? hyper::Error::is_buf_size_exceeded() as a strawman?

[acfoltzer]

When chasing down a few other errors we've observed for customer services, I'm coming around to the idea that hyper::Error might need a broader scale overhaul. Perhaps with #[non_exhaustive] now in the language, we could expose the error enums more directly without risking too much in terms of compatibility?

Currently there are only 8 flavors of error that can be inspected programmatically through the hyper::Error::is_*() methods. But, depending on the feature flags enabled, there are at least 31 error kinds (7 Parse variants + 11 User variants + 13 other Kinds), plus dynamically typed errors via the cause field. While these can be distinguished by parsing the Display output of the error, this is of course not a sustainable approach.

I checked the rest of the issues for something speaking to a broader Error overhaul effort, so apologies if I missed something, but this would be really valuable to have sorted out before 1.0. How can we help?

[seanmonstar]

Having a method to distinguish that it's a TooLarge internally would be fine.

Regarding the general idea of making the variants public, this comment still generally fits what I've though about the design issue of doing so in Rust.

[acfoltzer]

@seanmonstar thanks. That's good context, but the thread seems to be inconclusive, so I'm not sure what the right path forward is. Would you like to see the method approach extended to all the currently-possible error variants? There was also mention in that thread of distinguishing between server and client errors; should that still be considered as well?

[seanmonstar]

Yea that thread itself doesn't have a conclusion, I mostly was just meaning to link to that specific comment. I've also largely embraced the idea that only a few error conditions are ever interesting in code (such that you would want to react and do something differently), and that most are just something to eventually tell the user about.

I don't yet know what the way forward is for matching error variants. But I've started a draft blog post about the difficulties of pattern matching vs extensibility...

[acfoltzer]

The problems for us arise less in terms of code needing to react and do things differently, and more in transparency and reporting to customers, as well as observability into the health of our systems. In other words, it's just something to eventually tell the user about, but how we tell them matters. While we could present the Display output to users and let them parse it if they wish to quantize the data, it would be less error-prone and more straightforward to be able to provide the more granular error information directly.

Given that, we'd really like to see a comprehensive programmatic way to distinguish errors, whether that's via pattern matching, methods, or some other approach (other than string matching 😉). It sounds like you'd prefer to continue with the methods approach, but I'd like to make sure of that before we work on putting a PR together with a boatload of new methods.

[seanmonstar]

I think the methods option, with a ton of is_*, is ugly 😢 . But it also allows extending to new variants. For example, #2466 is asking that Parse::TooLarge be expanded into more specific sub variants. If we had exposed a normal, flat ErrorKind enum, it'd be impossible to do that without breaking people.

I've never gotten around to more fully trying that wacky idea I linked to... like, if the exposed kinds were somehow completely non-exhaustive, then this could be possible, maybe?

match e.kind() {
    Kind::TooLarge(_) => {},
    _etc => {},
}

And some eventual addition like this would still work, I think?

enum TooLarge {
    HeaderSection(Sealed),
    HeaderName(Sealed),
    // etc
}

enum Kind {
    TooLarge(TooLarge),
    // etc
}

[nox]

When you say Sealed, I assume you mean a public type in a private module that downstream cannot refer by name?

[seanmonstar]

Yea exactly, the linked comment tries to explain it more. Note though, I'm not saying that this is what we definitely should do, just that it seems plausible and I wish I had time to explore it more in an experiment to see how good or bad it is in reality.

[nox]

That seems like a reasonable plan to me. Do you think we should try to get people's opinion on this somewhere? Can we start implementing that error encoding?

[seanmonstar]

Certainly, trying it out on a smaller project where it's less expensive (to the ecosystem) to try it out would be a great start. Also happy to get other people's opinions, wherever you think they're likely to provide them.

[nox]

No I mean as a PR to Hyper hah.