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 types should not implement source _and_ print inner error in Display #1345

Closed
Tracked by #1582 ...
jonhoo opened this issue Apr 26, 2022 · 4 comments
Closed
Tracked by #1582 ...
Assignees
Labels
bug Something isn't working sdk-ga
Milestone

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Apr 26, 2022

SdkError, and possibly other error types within the SDK, currently expose inner errors both through Error::source and fmt::Display:

https://github.com/awslabs/smithy-rs/blob/692bf94ba30f570365f559c67d6a9981c037c371/rust-runtime/aws-smithy-http/src/result.rs#L176-L206

The recommendation in the ecosystem is to either return inner errors through Error::source or print the inner error in Display, but never both (see rust-lang/project-error-handling#27 (comment), rust-lang/project-error-handling#23, rust-lang/api-guidelines#210), as otherwise errors end up being duplicated whenever error reporters print error backtraces. For example, I recently got an error trace that ended in:

    6: failed to construct request: Failed to load credentials from the credentials provider: An error occurred while loading credentials: io error: error trying to connect: invalid certificate: UnknownIssuer
    7: Failed to load credentials from the credentials provider: An error occurred while loading credentials: io error: error trying to connect: invalid certificate: UnknownIssuer
@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 26, 2022

@jdisanti jdisanti added the bug Something isn't working label Apr 26, 2022
@rcoh
Copy link
Collaborator

rcoh commented Apr 27, 2022

yeah this is definitely the case currently—is there a plan to add backtrace reporting by default? I worry about customers not using a "fancy" printer missing error context

@rcoh
Copy link
Collaborator

rcoh commented Apr 27, 2022

I guess this may guide us towards "use display but not source"?

@jdisanti
Copy link
Collaborator

Implemented as part of #1926

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working sdk-ga
Projects
None yet
Development

No branches or pull requests

3 participants