http-rs / http-types

Common types for HTTP operations
https://docs.rs/http-types
Apache License 2.0
200 stars 84 forks source link

http_types::Error should implement std::error::Error #88

Open ghost opened 4 years ago

ghost commented 4 years ago

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.

yoshuawuyts commented 4 years ago

@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.

ghost commented 4 years ago

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 commented 4 years ago

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)
}
dnsco commented 4 years ago

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 commented 4 years ago

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

camerondavison commented 4 years ago

looks like #90 got close. just fyi

tirr-c commented 4 years ago

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

DCjanus commented 3 years ago

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())?;
NotAFile commented 3 years ago

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

passcod commented 3 years ago

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?

otavio commented 3 years ago

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

Fishrock123 commented 3 years ago

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"