http-types icon indicating copy to clipboard operation
http-types copied to clipboard

http_types::Error should implement std::error::Error

Open ghost opened this issue 5 years ago • 12 comments

Because it doesn't, this does not compile:

async fn get(addr: &str) -> anyhow::Result<Response> {
    // ...
    let resp = async_h1::connect(stream, req).await?;
    Ok(resp)
}

It's not obvious what to do, so I'm using a hack like this:

let resp = async_h1::connect(stream, req).await.map_err(anyhow::Error::msg)?;

In any case, all error types should implement std::error::Error in order to work seamlessly with the ? operator.

ghost avatar Mar 26 '20 12:03 ghost

@stjepang unfortunately this is not possible because of constraints in the trait resolver. We've inherited this restriction from anyhow, and adopted their way around it, which is to use AsRef<Error> (docs).

I don't know how to resolve between the two. What I'd recommend is using a single error container type throughout: so perhaps consider using http_types::Result in the bound of your get function.

yoshuawuyts avatar Mar 26 '20 15:03 yoshuawuyts

I think the issue is simply that some of the blanket impls are too broad (more than needed).

We can implement std::error::Error for http_types::Error if we're a bit more conservative about those blanket impls for error conversion. See https://github.com/http-rs/http-types/pull/90

ghost avatar Mar 31 '20 18:03 ghost

Another example where I have to do manual conversions instead of ?:

async fn fetch(&self, url: Url) -> Result<String> {
    let stream = self.conn_r.recv().await.unwrap();
    let body = async_h1::connect(stream.clone(), Request::new(Method::Get, url))
        .await
        .map_err(Error::msg)?;
    let body = body.body_string().await.map_err(Error::msg)?;
    self.conn_s.send(stream).await;
    Ok(body)
}

ghost avatar Apr 01 '20 14:04 ghost

I opened a related issue on the anyhow side: https://github.com/dtolnay/anyhow/issues/89.

would a pull_request for a from_anyhow constructor so you didn't have to drop down to from_str and loose the backtrace be appreciated?

dnsco avatar May 04 '20 09:05 dnsco

Just saw #90, I'll hold off until that lands.

dnsco avatar May 04 '20 09:05 dnsco

looks like #90 got close. just fyi

camerondavison avatar May 23 '20 21:05 camerondavison

http_types::Error not implementing std::error::Error is preventing Tide's Status impls applied to Result<T, tide::Error>. cc @yoshuawuyts

tirr-c avatar Aug 22 '20 13:08 tirr-c

Is there any plan for this issue? To be honest, without impl StdError for http_types::Error, there are too many bother code in my project, since I'm using anyhow.

global()
    .post(&new_url)
    .body(surf::Body::from_form(&params).map_err(|x| x.into_inner())?)
    .send()
    .await
    .map_err(|x| x.into_inner())?;

DCjanus avatar Feb 09 '21 10:02 DCjanus

Just came across this. http_types::Error can not be used as cause error in other libraries with thiserror because it does not implement StdError.

It sounds like generally, it is not recommended for libraries to use anyhow/eyre directly, as it essentially ends the error chain.

Use eyre if you don't think you'll do anything with an error other than report it. This is common in application code. Use thiserror if you think you need an error type that can be handled via match or reported. This is common in library crates where you don't know how your users will handle your errors.

Also relevant: #367

NotAFile avatar Jul 30 '21 20:07 NotAFile

Could we have some clarifying from Surf/this crate's maintainers as to this situation? This is the number one issue that always stops me using surf. As per #90:

We talked about this over chat a while back, and this patch is not the right fit for where we're taking this crate. Closing.

Okay, but what is?

passcod avatar Aug 24 '21 06:08 passcod

This is indeed relevant and I agree it should be addressed.

otavio avatar Aug 24 '21 11:08 otavio

If you would like to discuss this further, I suggest joining this Zulip thread: https://http-rs.zulipchat.com/#narrow/stream/261411-surf/topic/new.20Error.20type

In particular I give a run-down of the problems with the current approach in Surf here: https://http-rs.zulipchat.com/#narrow/stream/261411-surf/topic/new.20Error.20type/near/250422987

So to clarify there is actually three issues here I believe:

  • Surf's return error does not implement StdError, making it hard to work with outside of a Tide handler
  • Surf's return error always contains a status code even if the server did not reply
  • Surf does not return any easily-matched-upon error codes/variants outside of status code

Some thought on the topic of http_types::Error probably need to be put in to "can / should Surf and Tide continue to have the same base error or is a seamless interop story good enough?"

My 2c on that would be "http_types::Error built on top of anyhow/eyre suits Tide well but suits Surf not as well"

Fishrock123 avatar Aug 24 '21 17:08 Fishrock123