starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 11:15:54 +03:00

[GH-ISSUE #326] Migrate all from error-chain to failure #151

New issue

Closed

opened 2026-03-07 22:29:01 +03:00 by kerem · 4 comments

kerem commented

2026-03-07 22:29:01 +03:00

Owner

Originally created by @bluejekyll on GitHub (Jan 6, 2018).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/326

https://github.com/withoutboats/failure
https://docs.rs/failure/0.1.1/failure/

Originally created by @bluejekyll on GitHub (Jan 6, 2018). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/326 https://github.com/withoutboats/failure https://docs.rs/failure/0.1.1/failure/

kerem

2026-03-07 22:29:01 +03:00

closed this issue
added the
crate:all

cleanup
labels

kerem commented

2026-03-07 22:29:17 +03:00

Author

Owner

@silwol commented on GitHub (May 16, 2018):

I'm investigating a possible transformation to failure.
Using the structure as described in https://boats.gitlab.io/failure/error-errorkind.html seems to allow rather straight-forward translation.
Right now, the error_chain! macro uses names for the parameters, looking like this:

error_chain! {
  // …
  errors {
    DomainNameTooLong(len: usize) {
      description("name label data exceed 255")
      display("name label data exceed 255: {}", len)
    }
    // …
}

This would translate to either of these:

Variant 1:

#[derive(Copy, Eq, PartialEq, Debug, Fail)]
pub enum ProtoErrorKind {
    // …
    #[fail(display = "name label data exceed 255: {}", len)]
    DomainNameTooLong { len: usize },
    // …
}

Variant 2:

#[derive(Copy, Eq, PartialEq, Debug, Fail)]
pub enum ProtoErrorKind {
    // …
    #[fail(display = "name label data exceed 255: {}", _0)]
    DomainNameTooLong(usize),
    // …
}

In variant 1, the ErrorKind definition is much more readable, describing the parameters by name, so I would prefer that. This causes changes all over the rest of the source code wherever errors get constructed, which would not be necessary for variant 2:

return Err(ProtoErrorKind::DomainNameTooLong(length).into());

must be changed to:

return Err(ProtoErrorKind::DomainNameTooLong{len: length}.into());

or in the optimal case that the variable already has the right name:

return Err(ProtoErrorKind::DomainNameTooLong{len}.into());

Is there any significant reason to prefer one variant over the other in your opinion?

@silwol commented on GitHub (May 16, 2018): I'm investigating a possible transformation to failure. Using the structure as described in https://boats.gitlab.io/failure/error-errorkind.html seems to allow rather straight-forward translation. Right now, the error_chain! macro uses names for the parameters, looking like this: ```rust error_chain! { // … errors { DomainNameTooLong(len: usize) { description("name label data exceed 255") display("name label data exceed 255: {}", len) } // … } ``` This would translate to either of these: Variant 1: ```rust #[derive(Copy, Eq, PartialEq, Debug, Fail)] pub enum ProtoErrorKind { // … #[fail(display = "name label data exceed 255: {}", len)] DomainNameTooLong { len: usize }, // … } ``` Variant 2: ```rust #[derive(Copy, Eq, PartialEq, Debug, Fail)] pub enum ProtoErrorKind { // … #[fail(display = "name label data exceed 255: {}", _0)] DomainNameTooLong(usize), // … } ``` In variant 1, the ErrorKind definition is much more readable, describing the parameters by name, so I would prefer that. This causes changes all over the rest of the source code wherever errors get constructed, which would not be necessary for variant 2: ```rust return Err(ProtoErrorKind::DomainNameTooLong(length).into()); ``` must be changed to: ```rust return Err(ProtoErrorKind::DomainNameTooLong{len: length}.into()); ``` or in the optimal case that the variable already has the right name: ```rust return Err(ProtoErrorKind::DomainNameTooLong{len}.into()); ``` Is there any significant reason to prefer one variant over the other in your opinion?

kerem commented

2026-03-07 22:29:22 +03:00

Author

Owner

@bluejekyll commented on GitHub (May 16, 2018):

Thank you for looking into this change and for taking the time to put together a proposal. This is a great description of the different options.

Between the Variant 1 and 2, I also like the expressiveness of Variant 1, though 2 is a little more ergonomic. The nice thing about 1 is the expressiveness and clarity about what the associated field actually represents. So if you want to go in that direction, that would be great. Though, it's something that I don't think is a a huge issue. If using Variant 1 makes converting to Failure a larger and more painful task, then I would suggest using 2 for now to reduce the effort.

I think there is a different question here that is implied with the name ProtoErrorKind and that's: do we need a wrapper type ProtoError(ProtoErrorKind). As an example, I wanted to use this pattern in a different project: https://github.com/bluejekyll/foundationdb-rs/blob/master/foundationdb/src/error.rs#L30-L58. The reason for that was because of shared information (that should_retry field) which needed to be tracked across all kinds of errors. I don't think we have that issue here and can probably get away with a single ProtoError enum as the error type. This should be true for the general case of all the crates.

I'm not sure if you're planning to do the work for all the projects (client, server, proto, resolver). The client errors are a bit of a mess. They are holdovers from the very beginnings of the project. I think the right thing would be to consolidate those errors into a single ClientError type, and drop all the differences, but I'm open to other suggestions there.

@bluejekyll commented on GitHub (May 16, 2018): Thank you for looking into this change and for taking the time to put together a proposal. This is a great description of the different options. Between the Variant 1 and 2, I also like the expressiveness of Variant 1, though 2 is a little more ergonomic. The nice thing about 1 is the expressiveness and clarity about what the associated field actually represents. So if you want to go in that direction, that would be great. Though, it's something that I don't think is a a huge issue. If using Variant 1 makes converting to Failure a larger and more painful task, then I would suggest using 2 for now to reduce the effort. I think there is a different question here that is implied with the name `ProtoErrorKind` and that's: do we need a wrapper type `ProtoError(ProtoErrorKind)`. As an example, I wanted to use this pattern in a different project: https://github.com/bluejekyll/foundationdb-rs/blob/master/foundationdb/src/error.rs#L30-L58. The reason for that was because of shared information (that `should_retry` field) which needed to be tracked across all kinds of errors. I don't think we have that issue here and can probably get away with a single `ProtoError` enum as the error type. This should be true for the general case of all the crates. I'm not sure if you're planning to do the work for all the projects (client, server, proto, resolver). The client errors are a bit of a mess. They are holdovers from the very beginnings of the project. I think the right thing would be to consolidate those errors into a single `ClientError` type, and drop all the differences, but I'm open to other suggestions there.

kerem commented

2026-03-07 22:29:27 +03:00

Author

Owner

@silwol commented on GitHub (May 16, 2018):

I just made a short test without a ProtoErrorKind, just making ProtoError an enum, as described in https://boats.gitlab.io/failure/custom-fail.html because the idea is tempting.
However, the referenced cause error type FromUtf8Error prevents me from implementing Clone for ProtoError because the upstream error type does not permit copying or cloning. Clone is required by the FromProtoError trait.
Correct me if I'm wrong, but from my perspective it seems that a separation into an Error and an ErrorKind will be necessary, at least for ProtoError. I haven't looked into the other projects yet.
I managed to transform the proto project to a split-up Error + ErrorKind variant. This caused API changes which require modifications in the other projects, so the migration will surely break the exposed API surface.

For the embedded variables, I will use the mode proposed in variant 2, except for enum values containing multiple variables (e.g. IncorrectRDataLengthRead{read: usize, len: usize} which would be confusing without names).

I intend investigate migration of the other projects as well, because I think it's good to switch everything to the failure crate at once.

@silwol commented on GitHub (May 16, 2018): I just made a short test without a ProtoErrorKind, just making ProtoError an enum, as described in https://boats.gitlab.io/failure/custom-fail.html because the idea is tempting. However, the referenced cause error type `FromUtf8Error` prevents me from implementing `Clone` for `ProtoError` because the [upstream error type](https://doc.rust-lang.org/std/string/struct.FromUtf8Error.html) does not permit copying or cloning. `Clone` is required by the `FromProtoError` trait. Correct me if I'm wrong, but from my perspective it seems that a separation into an `Error` and an `ErrorKind` will be necessary, at least for `ProtoError`. I haven't looked into the other projects yet. I managed to transform the `proto` project to a split-up `Error` + `ErrorKind` variant. This caused API changes which require modifications in the other projects, so the migration will surely break the exposed API surface. For the embedded variables, I will use the mode proposed in variant 2, except for enum values containing multiple variables (e.g. `IncorrectRDataLengthRead{read: usize, len: usize}` which would be confusing without names). I intend investigate migration of the other projects as well, because I think it's good to switch everything to the failure crate at once.

kerem commented

2026-03-07 22:29:32 +03:00

Author

Owner

@bluejekyll commented on GitHub (May 16, 2018):

the referenced cause error type FromUtf8Error prevents me from implementing Clone for ProtoError

FromUtf8Error is Send/Sync, so maybe just wrap it in an Arc? I think that should work. I remember needing Clone for some reason or other, but that may have been to work around the fact that the library wasn't properly enforcing Send before. So that may not be an issue now. So maybe we can drop the Clone requirement?

Correct me if I'm wrong, but from my perspective it seems that a separation into an Error and an ErrorKind will be necessary

I haven't looked at these error types in a while, so I'll go with your opinion on it since you're looking at it now.

I will use the mode proposed in variant 2

Wonderful

I intend investigate migration of the other projects as well, because I think it's good to switch everything to the failure crate at once.

I agree, and thank you!

@bluejekyll commented on GitHub (May 16, 2018): > the referenced cause error type FromUtf8Error prevents me from implementing Clone for ProtoError `FromUtf8Error` is Send/Sync, so maybe just wrap it in an `Arc`? I think that should work. I remember needing Clone for some reason or other, but that may have been to work around the fact that the library wasn't properly enforcing Send before. So that may not be an issue now. So maybe we can drop the Clone requirement? > Correct me if I'm wrong, but from my perspective it seems that a separation into an Error and an ErrorKind will be necessary I haven't looked at these error types in a while, so I'll go with your opinion on it since you're looking at it now. > I will use the mode proposed in variant 2 Wonderful > I intend investigate migration of the other projects as well, because I think it's good to switch everything to the failure crate at once. I agree, and thank you!

kerem referenced this issue

2026-03-16 01:57:32 +03:00

[PR #151] [MERGED] Update ring and chrono #1275