Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hyper::Error should either print source in display xor return a Some from Error::cause(), not both #2732

Closed
nagisa opened this issue Jan 7, 2022 · 5 comments · Fixed by #2737
Labels
A-error Area: error handling B-rfc Blocked: More comments would be useful in determine next steps.

Comments

@nagisa
Copy link

nagisa commented Jan 7, 2022

Version
f1b89c1

Platform
Any

Description
Today the std::fmt::Display impl for hyper::Error has the following snippet:

hyper/src/error.rs

Lines 500 to 501 in f1b89c1

if let Some(ref cause) = self.inner.cause {
write!(f, "{}: {}", self.description(), cause)

and the std::error::Error::source impl for hyper::Error reads as such:

hyper/src/error.rs

Lines 510 to 513 in f1b89c1

self.inner
.cause
.as_ref()
.map(|cause| &**cause as &(dyn StdError + 'static))

This is somewhat problematic for code that properly prints out the error causation chain. e.g. I have seen an error like this:

error: error trying to connect: tcp connect error: Connection refused (os error 111)
caused by: tcp connect error: Connection refused (os error 111)
caused by: Connection refused (os error 111)

this is pretty unreadable. Now one might argue that hyper::Error::description() serves this use-case, and while that's true, it seems pretty awkward to pull in hyper into the code that's implements error presentation layer just to downcast and check for hyper.

@nagisa nagisa added the C-bug Category: bug. Something is wrong. This is bad! label Jan 7, 2022
@nagisa nagisa changed the title hyper::Error should either print cause in display xor return a Some from Error::cause(), not both hyper::Error should either print source in display xor return a Some from Error::cause(), not both Jan 7, 2022
@seanmonstar
Copy link
Member

See also #2542

@seanmonstar
Copy link
Member

I should probably write up some thoughts as to why this is currently the way it is.

Wrapping an error (so that there's an inner source) frequently provides more context that is useful to users. Without showing the source, the extra context can lose some of its meaning. For example, seeing just "error trying to connect" is also too vague, just as "Connection refused" can be. Most users of hyper tend to just log the Display output of the error, without anything fancy. So the format for hyper is to default to be most useful to them.

I recognize that makes it more verbose for people using utilities to crawl the source chain. I currently chose to cater to the more common case.

I'm also aware that the "error working group" currently recommends against doing what hyper does. But I don't agree, since it means hurting the common case. And since I believe most users will generally just default to logging {} of an error, I don't think "more education" will fix that. Thus, I'd suggest that the WG consider instead that errors print their source chain by default, and allow advance users to opt-in to custom formatting. I explored this concept in seanmonstar/errors#1, and have a not-published crate that does this, even...

@seanmonstar seanmonstar added B-rfc Blocked: More comments would be useful in determine next steps. A-error Area: error handling and removed C-bug Category: bug. Something is wrong. This is bad! labels Jan 7, 2022
@nox
Copy link
Contributor

nox commented Jan 11, 2022

@seanmonstar What do you think of making {:#} not print the source parts?

@davidpdrsn
Copy link
Member

I think {:#} would also work for us as well. Then we could make a newtype around hyper::Error that used {:#} in its Display impl.

I also considered adding a method a la fn message(&self) -> impl Display that returns hyper::Errors message without the source, which we could then use in a newtype. Asked on Discord about that yesterday.

The second option feels nicer to me but either should work.

seanmonstar pushed a commit that referenced this issue Feb 5, 2022
This adds Error::message which returns the message that is unique to the error, without the message from the source. That way users can create a newtype around hyper::Error and use this in the Display impl to work around #2732.

Closes #2732
@davidpdrsn
Copy link
Member

To others who might find this. I used the new hyper::Error::message to fix this like so:

struct DedupHyperErrorSources {
    error: hyper::Error,
}

impl DedupHyperErrorSources {
    fn new(error: hyper::Error) -> Self {
        Self { error }
    }
}

impl fmt::Debug for DedupHyperErrorSources {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        self.error.fmt(f)
    }
}

impl fmt::Display for DedupHyperErrorSources {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}", self.error.message())
    }
}

impl std::error::Error for DedupHyperErrorSources {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        if self.error.is_connect() {
            // hyper's internal `ConnectError` also includes the source in its `Display` impl but
            // otherwise doesn't add anything valuable to the source chain. So we can just skip it.
            self.error.source()?.source()
        } else {
            self.error.source()
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error Area: error handling B-rfc Blocked: More comments would be useful in determine next steps.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants