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: wrap io::Error in Arc for clone #2181

Merged
merged 1 commit into from Apr 14, 2024
Merged

Conversation

cpu
Copy link
Contributor

@cpu cpu commented Apr 13, 2024

ProtoErrorKind is Clone, but the Io variant holding io::Error runs into trouble with this: since the io::Error error can't be cloned we have to reconstruct it and this is a lossy proces resulting in a "simple" io::Error that only holds the error type from the parent it was cloned from. This loses important details like the underlying error source/message.

This branch changes ProtoErrorKind::Io to hold Arc<io::Error> instead (Note: this is a breaking change). This makes implementing Clone trivial - we clone the arc - and no error information is lost.

With this fix in place we get a much better error fidelity from failures like trying to perform DNS-over-HTTPS using Rustls but without a source of trust anchors (e.g. #2066). Previously the error was surfaced as:

failed to lookup HTTPS record type: ResolveError { kind: Proto(ProtoError { kind: Io(Kind(InvalidData)) }) }'

With this change in place, the error is now surfaced with much more detail:

failed to lookup HTTPS record type: ResolveError { kind: Proto(ProtoError { kind: Io(Custom { kind: InvalidData, error: InvalidCertificate(UnknownIssuer) }) }) }

Updates #2066

`ProtoErrorKind` is `Clone`, but the `Io` variant holding `io:Error`
runs into trouble with this: since the error can't be cloned we have to
reconstruct it and this is a lossy process: resulting in a "simple"
`io::Error` that only holds the error type from the parent it was cloned
from. This loses important details like the underlying error
source/message.

This commit changes `ProtoErrorKind::Io` to hold `Arc<io::Error>>`
instead. This makes implementing `Clone` trivial - we clone the arc
- and no error information is lost.
Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is great!

@djc djc merged commit 2e84c11 into hickory-dns:main Apr 14, 2024
18 checks passed
@cpu cpu deleted the cpu-io-err-clone branch April 14, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants