surf::Exception does not implement std::error::Error

[dtolnay]

This is an unfriendly choice for errors returned by a library because it makes them not work with ?.

async fn repro() -> Result<(), failure::Error> {
    let _ = surf::get("https://www.rust-lang.org").await?; // doesn't work
    Ok(())
}

async fn repro() -> anyhow::Result<()> {
    let _ = surf::get("https://www.rust-lang.org").await?; // doesn't work
    Ok(())
}

Application-focused error types like failure::Error are built on impl<T: std::error::Error> From<T> which is why it matters that library errors implement std::error::Error.

[yoshuawuyts]

@dtolnay thanks for pointing this out; I agree this is not right, and we should fix this. Thanks for reporting, and sorry this spilled over into your issue tracker!

[matklad]

@yoshuawuyts this might want to be marked with 2.0 milestone.

[richard-uk1]

From anyhow::Error docs

Error works a lot like Box, but with these differences:

• Error requires that the error is Send, Sync, and 'static.
• Error guarantees that a backtrace is available, even if the underlying error type does not provide one.
• Error is represented as a narrow pointer — exactly one word in size instead of two.

• The first one of these is fixed by using Box<dyn std::error::Error + Send + Sync> (I'm informed boxed errors are 'static unless they have a lifetime parameter).
• anyhow::Error creates a backtrace where the error is first converted into an anyhow::Error, I'm not sure that adds much in this use case over not having one.
• anyhow::Error is a thin pointer — so you add one indirection to save one usize width. Again I don't think this matters much in this context: your Result<T> will be sized by T (unless it is less than 2 usizes), and unless you've only got a tiny amount of memory (in which case you can't use alloc, and therefore this crate) the nature of making http requests means that you're not going to create enough to make memory usage an issue.

All in all just use Box<dyn std::error::Error + Send + Sync> which is also 'static. 1 less dependency and does the job of a type-erased error.

I made an RFC for adding the type alias to std.

[dtolnay]

@derekdreery Box<dyn Error + Send + Sync> does not implement std::error::Error so it does not address this issue at all.

[matklad]

I'm informed boxed errors are 'static unless they have a lifetime parameter

The precise rule here is that lifetime elision works for trait object (dyn Trait), and elided lifetime is 'static, unless the trait object is behind an &[mut] 'a, in which case the lifetime is 'a. That is, boxed errors are not special, a DST dyn Error is also equivalent to dyn Error + 'static.

All in all just use Box<dyn std::error::Error + Send + Sync> which is also 'static. 1 less dependency and does the job of a type-erased error.

The general problem here is that any two type-erased errors are not interoperable with each other, be it Box<dyn Error>, anyhow::Error or failure::Error.

In the ideal world, anyhow::Error would contain From<Box<dyn Error>> for anyhow::Error impl, but that is incoherent, because upstream crate (std) could add impl Error for Box<dyn Error>. The irony is that std can't actually add this imp, because that would make the two From impls for Box<dyn Error> overlap.

It looks like, if a library wants to use dynamic errors internally, it has to do some manual wrapping into struct Error { dyn_error: Box<dyn std::error::Error> } at the boundary, or it has to force some specific dynamic error type onto the user.

[richard-uk1]

@dtolnay you're right I'm thinking about it the wrong way round - it's the outer error (anyhow/failure) that could be Box<dyn Error +...>. The solution here is just to implement std::error::Error some way that makes sense for Exception.

[Fishrock123]

I believe this was effectively solved by moving to http_types::Error in c8366fb. cc @yoshuawuyts

[dtolnay]

The http_types::Error type in your link does not implement std::error::Error so I'm not sure that would solve it.

[joshtriplett]

This seems to still be an issue today. I tried surf 2.0.0-alpha.4, and attempted to use ? in a function using anyhow::Error as its error type. This failed because surf::Error doesn't implement std::error::Error.

[Fishrock123]

I don't think this is going to make the cut for 2.0 as I am unsure what to do about this.

[leo60228]

Fixing this requires a breaking change. I think this should've been considered a release blocker.

[Fishrock123]

We will have a 3.0 at some point and we should think about it for that.

[jbr]

I'm pretty sure that the type system won't let us implement std::error::Error for http_types::Error, but we can make it easier to use http_types::Errors inside of anyhow, and possibly even unwrap them into anyhow::Errors outside of surf, as the status_code isn't important once it's outside of a surf context. This is at least partly a http_types issue

[Fishrock123]

That is also my understanding, however the error handling project group is also working on fixing StdError in ways that should help, as far as I'm aware.

[ohmree]

A relatively easy fix for users of eyre (which is a fork of anyhow, so I believe it should also work for the latter) is:

surf::get(my_url).await.map_err(|e| eyre!(e))?
// or
surf::get(my_url).await.map_err(|e| anyhow!(e))?

Alternatively you can also return a eyre::Result<T, surf::Error> (or anyhow::Result<T, surf::Error>) from your functions, although (with eyre at least) you miss out on pretty output that shows where the error occured and possibly more info (depends on the edition of eyre you use).

[leo60228]

For what it's worth, I think http_types::Error existing in the first place was a mistake. It makes sense for user-defined errors in a web server framework, but that's much more specific than HTTP in general. In my opinion, surf should contain its own error type, probably using something like thiserror.
I'd be willing to PR this, and in my experience it is a major usability issue that IMHO would deserve a 3.0.0 on its own.

[lefuturiste]

Is this issue on the roadmap?