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

Error Display implementation contains source display impl as well #213

Closed
faern opened this issue Dec 29, 2019 · 2 comments · Fixed by #217
Closed

Error Display implementation contains source display impl as well #213

faern opened this issue Dec 29, 2019 · 2 comments · Fixed by #217
Labels
A-tonic C-enhancement Category: New feature or request E-help-wanted Call for participation: Help is requested to fix this issue.

Comments

@faern
Copy link
Contributor

faern commented Dec 29, 2019

Feature Request

When printing a connection refused error and all its source errors you currently end up with:

Error: Client: error trying to connect: tcp connect error: Connection refused (os error 111)
Caused by: 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)

That is very redundant and not very nice to read. A good rule is to not include the Display implementation of your source error in your error. Or if you do, don't return it from source(). Never do both.

I created a forum post on this topic a while back: https://users.rust-lang.org/t/do-people-not-care-about-printable-error-chains-a-k-a-how-to-nicely-implement-display-for-an-error/35362

I see the stack of errors here is not only tonic specific, but the problem exists in hyper as well.

Proposal

Change some error type implementations. Especially if there are errors which are basically just combinations of sub-errors, then make them "transparent". Meaning their Display impl just proxies to the Display of the sub-error and source() become something like:

fn source() -> Option<dyn Error> {
    match self {
        Io(error) => error.source(),
        Something(error) => error.source(),
        ...
    }
}
@LucioFranco
Copy link
Member

@faern hi! I agree the current error situation with the transport errors is a bit funky. I plan on improving them but am lacking some time right now to get there. I think what you're suggesting seems good! If you're open to PRing it I would love that and I'm happy to help out :)

@LucioFranco LucioFranco added A-tonic C-enhancement Category: New feature or request E-help-wanted Call for participation: Help is requested to fix this issue. labels Dec 30, 2019
@faern
Copy link
Contributor Author

faern commented Jan 4, 2020

With the submitted PR, #217, The output of printing the error chain is now reduced to:

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)

The remaining problems exist in layers outside of tonic. I will have to do something similar to hyper later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tonic C-enhancement Category: New feature or request E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants