Change `error-chain` to `thiserror` #88

RoDmitry · 2024-04-30T16:58:44Z

Changed error-chain to thiserror, and updated dependencies.
Maybe you would also want to remove error-chain from examples and tests.

Closes #66

This change is

faern

Thanks for giving this library some love! I'm all for getting rid of error-chain, but I do have some feedback and comments about exactly how it's done. See individual comments.

Error design is a debated topic. Personally I prefer that libraries use handwritten* errors, are #[non_exhaustive].

*Keeps down number of dependencies for not that much extra boilerplate

faern · 2024-05-02T07:56:52Z

src/lib.rs

+    pub info: Option<ErrorInfo>,
+    #[source]
+    pub source: ErrorSource,
+}


I have not really seen this exact way of desiging errors before. Do you have references to other libraries using this style? I mean splitting the error into a struct plus two enums and having the latter be public struct members etc.

Given how everything is public and the enums are not #[non_exhaustive] adding any new error variant is a breaking change, which is not ideal IMO.

This was discussed at Mullvad recently and we ended up with: mullvad/udp-over-tcp#57. I'd much prefer something along those lines.

Do you have references to other libraries using this style?

No. I guess I've just invented it myself. Just saved the original logic as it was before.

What's the idea behind having info as a separate thing here? Absolutely everything regarding the internal details of these errors are exposed, making it a breaking change to change anything in it. I'd advise you to take a look at the PR I linked above where we did error improvements.

There is no need to keep the logic similar to how the errors were before. That is really old and outdated code, created by a very outdated error library. So it's probably not a good source of inspiration.

faern · 2024-05-02T07:56:55Z

src/lib.rs

        }
+        self.source.fmt(f)


Including the source of the error in the display impl of it is not recommended. It makes printing a full error chain very redundant. Usually something like:

Error: Failed to start: Permission denied Caused by: Permission denied

Which just looks bad. Please see this guideline PR for more context: rust-lang/api-guidelines#210

faern · 2024-05-02T08:01:16Z

Cargo.toml

 scopeguard = "1.0"
+uuid = { version = "1", features = ["v4"] }


Please separate the dependency upgrades not related to swapping out error-chain to a separate PR. I prefer if PRs are as focused as possible on one thing.

I'll have to wait for the merge then. That's actually very annoying if you accept only one thing per PR.

If we have issues/complications it's going to be significantly easier to find what the cause is if changes are separated into smaller logical chunks. Also easier to revert single things instead of a blob of unrelated things. Also way easier to review and reason about.

If you submit small easy to review PRs I can review and merge them fast.

RoDmitry · 2024-05-02T09:28:44Z

Fixed

Cargo.toml

faern · 2024-05-02T10:54:18Z

src/lib.rs

+    pub info: Option<ErrorInfo>,
+    #[source]
+    pub source: ErrorSource,
+}


What's the idea behind having info as a separate thing here? Absolutely everything regarding the internal details of these errors are exposed, making it a breaking change to change anything in it. I'd advise you to take a look at the PR I linked above where we did error improvements.

There is no need to keep the logic similar to how the errors were before. That is really old and outdated code, created by a very outdated error library. So it's probably not a good source of inspiration.

faern · 2024-05-02T10:55:04Z

src/lib.rs

        }
+        Ok(())


This means that an error with no info prints as nothing (empty string). That's not good. An error should never have an empty display representation, there must always be some description of what the error means.

RoDmitry · 2024-05-02T13:42:21Z

Fixed. Is it better?

RoDmitry · 2024-05-02T13:57:54Z

Absolutely everything regarding the internal details of these errors are exposed

Still don't understand why it's bad

faern · 2024-05-03T06:07:10Z

Absolutely everything regarding the internal details of these errors are exposed

Still don't understand why it's bad

Again, see mullvad/udp-over-tcp#57. If every aspect of the error is part of the public API then every small "internal" change becomes a breaking change. Let's say we wanted to change InvalidRuleCombination(String), into InvalidRuleCombination(&'static str), for some reason. Because it's part of the public API we now need to release this as a breaking change (0.5.0 instead of 0.4.7). Because otherwise it would break compilation for users who might hypothetically destructure the error and rely on the embedded enum variant type to be a String.

If we don't expose details that a user usually won't ever need to rely on, the API surface both becomes cleaner, and we are freer to change the internals without breaking stuff.

RoDmitry · 2024-05-03T08:19:46Z

I see the reasoning, but I don't understand why would you lose a part of the error. For InvalidRuleCombination(String), string is the part of the error's display implementation. If you drop it, how would you display it? And also I don't understand why would anybody match and rely on the internal string, if they don't need it. Maybe they actually would need that string? Because that string makes this variant different from the same variant with the different string. It's not as "internal" as you claim.

RoDmitry · 2024-05-03T08:29:33Z

If that is "internal", but internally we don't use it, maybe we actually don't need it then? But it's used in the display of the error message. That's why I would like to leave it as is. So a user would get more information about the error.

pronebird · 2024-05-03T09:55:41Z

src/rule/mod.rs

@@ -128,14 +128,16 @@ impl FilterRule {
                    "StatePolicy {:?} and protocol {:?} are incompatible",
                    state_policy, proto
                );
-                bail!(ErrorKind::InvalidRuleCombination(msg));
+                Err(Error::InvalidRuleCombination(msg))


Would it be better to return Error::InvalidRuleCombination(cause) as opposed to a pre-reformatted string msg? where cause could be:

enum InvalidRuleCause { StatePolicyAndProtocol(StatePolicy, Proto) AddrFamily(AddrFamily, AddrFamily) }

Also pfctl::FilterRuleBuilder::build() maps to InvalidRuleCombination which is rather odd. I think that root crate::Error should have a separate variant for that, perhaps:

BuildFilterRule(#[source] String) // error_derive returns string?

Alternatively, which seems simpler (better) to me, three new variants can be added into crate::Error and InvalidRuleCombination removed.

I don't think the end user needs AddrFamily, because it can be incompatible only when they don't match. It can be a new Error variant something like: IncompatibleAddrFamily.
I can create a separate variant for validate_state_policy with StatePolicy and Proto enums, if @faern would approve this.
And also separate variants for FilterRuleBuilder and RedirectRuleBuilder.
@faern what do you think?

pronebird · 2024-05-03T10:10:42Z

src/lib.rs

+    #[error("Cstr not null terminated")]
+    CstrNotTerminated,
+
+    #[error("Anchor does not exist")]


Type conversion error

pronebird · 2024-05-03T10:12:17Z

src/lib.rs

+    InvalidPortRange,
+
+    #[error("String does not fit destination")]
+    StrCopyNotFits,


I'd rename it to InsufficientStringBufferLength. Description should state the same, does fit "destination" is really mysterious.

pronebird · 2024-05-03T10:14:24Z

src/lib.rs

+    StrCopyNotFits,
+
+    #[error("String has null byte")]
+    StrCopyNullByte,


I'd rename it to StringContainsNullByte and describe as "String should not contain null byte"

pronebird · 2024-05-03T13:03:47Z

Absolutely everything regarding the internal details of these errors are exposed

Still don't understand why it's bad

Again, see mullvad/udp-over-tcp#57. If every aspect of the error is part of the public API then every small "internal" change becomes a breaking change. Let's say we wanted to change InvalidRuleCombination(String), into InvalidRuleCombination(&'static str), for some reason. Because it's part of the public API we now need to release this as a breaking change (0.5.0 instead of 0.4.7). Because otherwise it would break compilation for users who might hypothetically destructure the error and rely on the embedded enum variant type to be a String.

If we don't expose details that a user usually won't ever need to rely on, the API surface both becomes cleaner, and we are freer to change the internals without breaking stuff.

Unless I misunderstood you. ApplyTcpOptionsErrorInternal in that PR can be replaced with much shorter implementation using thiserror (with error descriptions omitted):

#[derive(Debug, Error)]
enum ApplyTcpOptionsError {
    RecvBuffer(#[source] io::Error),
    SendBuffer(#[source] Io::Error),
    #[cfg(target_os = "linux")]
    Mark(#[source] nix::Error),
    TcpNoDelay(#[source] io::Error),
}

What you did there basically you told consumer that whatever you do internally is not their business because source() returns dyn std::error::Error. It would be unfortunate if consumer had to guess the error type to match against specific ioctl error if they want their code to behave in some fashion.

It doesn't help you either if you decide to remove some ApplyTcpOptionsErrorKind, it would be a breaking change if anyone actually matched against it.

String to &'static str does not change on a whim either, just like TcpNoDelay(io::Error) will not suddenly transform into TcpNoDelay(nix::Error).

In your case if anyone matched against io::Error, you'd silently break their assumption because source() returns dyn std::error::Error. It's good for you, but it's bad for anyone actually matching those errors.

The PR above also introduces ApplyTcpOptionsErrorKind which mirrors ApplyTcpOptionsError but strips out associated values. More manual labor/code -- less code is better in my opinion.

I'd personally be in favor of not going that route. I don't know who your users are in udp-over-tcp and what level of detail they need when it comes to error matching, but in pfctl-rs we do not handle 99% of ioctl errors beyond a few that we map to StateAlreadyActive and it would be better to be transparent and let consumers decide how they want to handle those errors. My opinion that it would be enough to have one flat error or two, i.e: crate::Error and crate::TryCopyToError.

What do you really propose to do here?

faern · 2024-07-23T08:55:40Z

Thanks for wanting to contribute! However, we merged a different solution to this issue over in #107. The main issue in this PR was how the entire API of the error was still public. Part of wanting to get rid of error-chain was that we wanted the API of the errors ta change. A much less public API surface of the errors allows changing the error more without breaking the API.

faern requested changes May 2, 2024

View reviewed changes

faern reviewed May 2, 2024

View reviewed changes

RoDmitry force-pushed the error branch from 2fd3ab2 to 6138559 Compare May 2, 2024 09:14

faern requested changes May 2, 2024

View reviewed changes

RoDmitry force-pushed the error branch from 6138559 to e4232bd Compare May 2, 2024 13:41

RoDmitry force-pushed the error branch from e4232bd to 10639dc Compare May 2, 2024 13:49

RoDmitry force-pushed the error branch 2 times, most recently from 399165f to 89a14c7 Compare May 2, 2024 14:11

pronebird reviewed May 3, 2024

View reviewed changes

RoDmitry force-pushed the error branch from 89a14c7 to c347341 Compare May 3, 2024 10:36

Change error-chain to thiserror

c37e98f

RoDmitry force-pushed the error branch from c347341 to c37e98f Compare May 3, 2024 10:37

faern mentioned this pull request May 29, 2024

Stop using error_chain! in examples #102

Merged

faern mentioned this pull request Jul 12, 2024

Remove error-chain and replace with handcrafted errors #107

Merged

faern closed this Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `error-chain` to `thiserror` #88

Change `error-chain` to `thiserror` #88

RoDmitry commented Apr 30, 2024 •

edited by faern

Loading

faern left a comment

faern May 2, 2024

RoDmitry May 2, 2024 •

edited

Loading

faern May 2, 2024

faern May 2, 2024

faern May 2, 2024

RoDmitry May 2, 2024

faern May 3, 2024

faern May 3, 2024

RoDmitry commented May 2, 2024

faern May 2, 2024

faern May 2, 2024

RoDmitry commented May 2, 2024 •

edited

Loading

RoDmitry commented May 2, 2024 •

edited

Loading

faern commented May 3, 2024

RoDmitry commented May 3, 2024

RoDmitry commented May 3, 2024 •

edited

Loading

pronebird May 3, 2024 •

edited

Loading

RoDmitry May 3, 2024 •

edited

Loading

pronebird May 3, 2024

RoDmitry May 3, 2024

pronebird May 3, 2024 •

edited

Loading

RoDmitry May 3, 2024

pronebird May 3, 2024

RoDmitry May 3, 2024

pronebird commented May 3, 2024 •

edited

Loading

faern commented Jul 23, 2024

		scopeguard = "1.0"
		uuid = { version = "1", features = ["v4"] }

Change error-chain to thiserror #88

Change error-chain to thiserror #88

Conversation

RoDmitry commented Apr 30, 2024 • edited by faern Loading

faern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoDmitry May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoDmitry commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoDmitry commented May 2, 2024 • edited Loading

RoDmitry commented May 2, 2024 • edited Loading

faern commented May 3, 2024

RoDmitry commented May 3, 2024

RoDmitry commented May 3, 2024 • edited Loading

pronebird May 3, 2024 • edited Loading

Choose a reason for hiding this comment

RoDmitry May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pronebird May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pronebird commented May 3, 2024 • edited Loading

faern commented Jul 23, 2024

Change `error-chain` to `thiserror` #88

Change `error-chain` to `thiserror` #88

RoDmitry commented Apr 30, 2024 •

edited by faern

Loading

RoDmitry May 2, 2024 •

edited

Loading

RoDmitry commented May 2, 2024 •

edited

Loading

RoDmitry commented May 2, 2024 •

edited

Loading

RoDmitry commented May 3, 2024 •

edited

Loading

pronebird May 3, 2024 •

edited

Loading

RoDmitry May 3, 2024 •

edited

Loading

pronebird May 3, 2024 •

edited

Loading

pronebird commented May 3, 2024 •

edited

Loading