refactor: overhaul errors #395

Fishrock123 · 2022-02-04T22:59:08Z

Overhauls errors to allow for 3 types of errors:

Concrete errors (enum) from http-types itself
A dynamic error type for response handlers (almost identical to how Error was previously)
A combined type for handling request+response for a client

There is some exposition for this in Zulip: https://http-rs.zulipchat.com/#narrow/stream/261414-http-types/topic/New.20error.20types.20.26.20result.20apis/near/254273467

Basically this solves two problems:

Errors from within http-types/tide/surf ware currently unable to be matched upon except by string search.
Surf's and http-types's error return types not being StdError is a massive pain point.

The first problem is pretty simple to solve but was quite laborious. It meant changing all those bail!("some string error") and what not to be concrete enum variants to some degree of specificity. I have kept some errors grouped together where the string output could mean something to anyone debugging a problem but is unlikely to be something which someone would reasonably want to check during program runtime. This is to reduce the amount of labor doing this but also to cap the enum variants and prevent the number of variants from becoming unmanageable.

The second problem is tricky and weird. My solution is RequestError. This allows dynamic errors from surf middleware but also allows us to have concrete errors there. Some indirection is required for ResponseError due to StdError's conflict with anyhow (The problem is a conflict with From<T> for T, which I will never not be profusely annoyed at).

There are some potential issues here / open questions:

Does this actually work well with Tide and Surf?
- Basically, those libraries will also need their own concrete error types, and one of the varients would be HTTP(http_types::Error) or such, which... isn't really correct.
- We could solve that by removing the central error enum here and only having the specific sub error types HeaderError, BodyError, etc. Would need to make a couple extra types.
- We could also reinterpret the main http-types error enum and translate it directly to surf and tide ones.
- I have a slightly worry about how this would work between tide and surf, i.e. Tide would have to turn a Surf concrete error into a dynamic error unless it shared the same error enum for all cases even from those libraries.
Should Error::ArgTryIntoError(Box<dyn std::error::Error + Send + Sync + 'static>) exist? Would returning Result<Result<T>> in the necessary places be better? (@yoshuawuyts this is what I was asking on Zulip)
Are the Send + Sync + 'static requirements correct or a problem?

Edit: the diff is obviously quite large, so here are the interesting parts:

Fishrock123 · 2022-02-04T23:03:46Z

Hmph. The CI duly notes that this makes interop with hyper http more difficult (obviously).

Cargo.toml

Fishrock123 · 2022-06-20T18:44:00Z

@yoshuawuyts Hey I would like to merge this tomorrow if there is no objection

Fishrock123 · 2022-06-20T18:47:49Z

Made one additional change to this... all the enums are #[non_exhaustive] now so we can add more cases in minors where it fits well

joshtriplett · 2022-06-20T19:12:05Z

src/errors/error_kind.rs

+
+    #[cfg(feature = "hyperium_http")]
+    #[error("Hyperium HTTP error: {}", .0)]
+    HyperiumHttp(#[from] http::Error),


Could we just bundle this into a case for third-party library errors, and support downcasting? That would avoid exposing third-party types in our API, which could introduce breakage if those libraries bump versions.

@joshtriplett What would that look like? Specifically the "bundle this into a case for third-party library errors" part of that

src/errors/error_kind.rs

joshtriplett · 2022-06-20T19:15:55Z

src/errors/error_kind.rs

+    #[error("ETag header was invalid")]
+    ETagInvalid,
+    #[error("Age header was invalid: length out of bounds (unsized 64 bit integer)")]
+    AgeInvalid,


In general, do we want to have one of these variants for every header, or do we want to have one variant that has a header name and a message?

That's a good question, I don't know, but it would become trickier in some circumstances.

I figured it might be useful in code sometimes to be able to match on the header error (but this is just a feeling).

Some headers have error situations with non-&'static str information.

Also some don't really have any error variance and I can't forsee where they would, like ETag or Age, so I didn't give them a &'static str, but maybe they should all have that.

Another question would be naming... how do we feel about <header name>Invalid<optional detailed invalid area>?

joshtriplett · 2022-06-20T19:17:04Z

src/errors/error_kind.rs

+    #[error("HTTP Date-Time invalid seconds")]
+    SecondsInvalid,
+    #[error("HTTP Date-Time invalid minutes")]
+    MinutesInvalid,
+    #[error("HTTP Date-Time invalid hours")]
+    HourInvalid,
+    #[error("HTTP Date-Time invalid day")]
+    DayInvalid,
+    #[error("HTTP Date-Time invalid month")]
+    MonthInvalid,
+    #[error("HTTP Date-Time invalid year")]
+    YearInvalid,
+    #[error("HTTP Date-Time invalid week-day")]
+    WeekdayInvalid,


Is anyone going to use individual variants like this, rather than a more general parse error? When would someone want to treat these programmatically differently, rather than just showing a different error?

Hmm. The existing code just made all these errors and I kinda replicated it... I don't know if it would be useful... if not these could probably be &'static strs...

joshtriplett · 2022-06-20T19:20:36Z

Posted a few bits of feedback, but generally this looks great. Thanks for the substantial amount of work here.

Overhauls errors to allow for 3 types of errors: - Concrete errors (enum) from http-types itself - A dynamic error type for response handlers - A combined type for handling request+response for a client

Fishrock123 · 2022-06-21T17:37:08Z

@yoshuawuyts if you are going to write a bunch of typed headers could you please take a look at this first?

yoshuawuyts · 2022-06-24T15:20:17Z

@Fishrock123 thanks so much for the ping! - we were actually running into very similar issues around error handling while looking to integrate http-types into the Azure SDK (Azure/azure-sdk-for-rust#848), and I'm fairly comfortable saying that this seems like the right direction to go.

I still need to look at this line-by-line, but want to at least respond that I agree that we should land this before any further work on typed headers.

rbtcollins · 2022-06-24T20:58:20Z

I haven't read the diff, but just a quick though since future API breakage seems to be a strong concern here. Consider making all the concrete enums nonexhaustive. That will permit expanding them in future without API breakage - everyone will need to explicitly handle unexpected errors and not assume the error space will expand. (See e.g. filesystem errors in the stdlib for the pain of not doing this in advance).

Fishrock123 · 2022-06-24T21:08:22Z

I haven't read the diff, but just a quick though since future API breakage seems to be a strong concern here. Consider making all the concrete enums nonexhaustive. That will permit expanding them in future without API breakage - everyone will need to explicitly handle unexpected errors and not assume the error space will expand. (See e.g. filesystem errors in the stdlib for the pain of not doing this in advance).

Yes, it will be a large api change. I think that’s fine since no one really likes the current api anyway.

Also, yes I already did mark them all as #[non_exhaustive]:
https://github.com/http-rs/http-types/pull/395/files#diff-87f966704ebddb378e447e13ddf9ee203cd9d31bb502338a5f3ed3478df41588R9

OneOfOne · 2024-04-07T21:50:08Z

I'm maintaining a hard fork of this repo that I plan to keep up to date, please resubmit this PR at https://github.com/OneOfOne/http-types-rs

Fishrock123 requested review from jbr, joshtriplett and yoshuawuyts February 4, 2022 22:59

zkat reviewed Feb 5, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Fishrock123 force-pushed the errors-overhaul branch 3 times, most recently from 71276ec to 39ecbd7 Compare February 11, 2022 17:52

zkat mentioned this pull request Apr 26, 2022

Using miette in a middleware trait definition zkat/miette#156

Closed

Fishrock123 force-pushed the errors-overhaul branch from 39ecbd7 to 56fda40 Compare June 20, 2022 18:43

Fishrock123 force-pushed the errors-overhaul branch from 56fda40 to d10f4a8 Compare June 20, 2022 18:47