Add http::Error::inner_ref and cause

[dekellum]

Fixes #302

Provides access by reference to the inner error type.

Also uses Error::inner_ref to implement std::error::Error::cause, for now. When MSRV is raised to 1.30.0+, this could be used for Error::source instead, without any breaking change.

[carllerche]

Thanks for the PR. I commented on the original issue. I think a downcasting strategy is fine, but the details should be tweaked. I commented on the original issue.

[seanmonstar]

We could add a build.rs to detect the version to be able to implement Error::source when it's available...

[carllerche]

@seanmonstar not following.

The way I'm seeing it, the question is if http::Error "stands alone" or if it is a forwards compat layer to hide the inner variants.

So, should the inner error be exposed via Error::source or is the inner error the "real" error. I am leaning towards the latter.

[dekellum]

It could be exposed via a http::Error::downcast_ref::() -> Option<&T> type of fn.

As tested, one can also call Any::downcast_ref on the returned reference from http::Error::inner_ref implemented here, or use Any::is::<T> tests, which may be all that is needed and easier to use.

I think this has a slight advantage, also in that it implements Error::cause (in terms of inner_ref) which is commonly available with these kinds of wrapper error types.

[seanmonstar]

should the inner error be exposed via Error::source or is the inner error the "real" error. I am leaning towards the latter.

That's a fair point. Also, re-reading through the builder PR, it mentioned that if one wanted to handle specific errors, one could check for the errors before passing to the builder, like Response::builder().header("foo", value.parse()?) or something.

[dekellum]

All the inner errors are currently public, exported types, because when not using builders, these are the error types returned. If some private inner errors were added in the future, users just wouldn't be able to name and cast these.

We could add a build.rs to detect the version to be able to implement Error::source when it's available...

I could add that here, but am concerned about complicating things, e.g. adding a version_check build dependency. The alternative is use standard cause (not downcastable) or inner_ref (downcastable). Then once MSRV is 1.30+, implementing Error::source is one line based on current inner_ref.

[dekellum]

Not sure if I missed answering some open question in your consideration of this PR (or if the associated issue is valid)?

@carllerche wrote:

should the inner error be exposed via Error::source or is the inner error the "real" error.

I suggest that the inner error should be exposed (by reference from http::Error) because it is the "real" error, or more specifically, because it is the only error that gives some specific indication of the problem, beyond the Display or Debug format output.

@seanmonstar wrote:

one could check for the errors before passing to the builder, like Response::builder().header("foo", value.parse()?) or something

Assuming that it is possible to preempt all http::Error's from the builders this way (for example from response::Builder::body) then I would:

1. Have to be confident enough to call unwrap() on that call (risking a panic if I'm wrong with some future change of a seemingly harmless new invariant). Otherwise I would still need to contend with this Stringly-http::Error type.

2. Might need to end up recreating the innards of the current http::Error myself, in order to pass any of various Request/Response construction errors up through several methods to where the error would more reasonably be introspected/pulled-apart, to give my lib's caller more context, or the calling application a sense of the cause. For example, an InvalidHeaderName might be a configuration problem, where as an InvalidStatusCode would be a internal server error.

Said another way: having a single http::Error wrapper type is desirable, it just needs a means of access to the inner error.

Or, what's the argument for leaving http::Error opaque like it is now, when all "inner" errors are already public types?

[dekellum]

Why did we go silent on this? I am comfortable adding a build.rs and version_check build dep to this PR, to implement Error::source when rustc is 1.30+. For rustc < 1.30 this would implement Error::cause, and the (currently PR implemented) Error::inner_ref would remain public for all rustc versions. This is my best interpretation of @seanmonstar 's suggestion.

Would you like to clarify or otherwise give me a hint if such a modified PR would be merged?

Thanks for your consideration!

[seanmonstar]

Sorry for dropping the ball, I was out for the week thanks to a small procedure. Let's get back to this.

I think @carllerche was saying it shouldn't be cause or source, because the inner error is the error, not the cause.

The question is what should we expose for http::Error. We could have something like inner_ref, and let users deal with std::error::Error, but it feels a little clunky. What if instead there were fn is<T: 'static>() -> bool?

[dekellum]

I won't belabor this, but I don't really follow the distinction being made, re: "is" the error. Recommendations (incl by error/failure/quick/chain libs) all seem to implement cause for this situation of multiple possible inner errors being wrapped as a single type.

I added 2d88403 to demonstrate an Error::is helper. This would be minimally workable, and if that remains the preference I can remove the Error::cause impl.

[carllerche]

@dekellum The main difference is what happens when Into<Box<Error>> is called. Is the returned error type the wrapper or the inner error. I believe it should be the inner error, which implies that it should not be exposed via cause or source.

[carllerche]

Almost there 👍

[dekellum]

I'll be out of town myself for the next 2 weeks, so if you need more changes, unfortunately I will be further delayed in responding. Thanks.

[seanmonstar]

Thanks for pushing on this!