Skip to content

Wrap inner errors#748

Open
WhyNotHugo wants to merge 1 commit intoemersion:v2from
WhyNotHugo:wrap-errors
Open

Wrap inner errors#748
WhyNotHugo wants to merge 1 commit intoemersion:v2from
WhyNotHugo:wrap-errors

Conversation

@WhyNotHugo
Copy link
Contributor

The following error (split here into multiple lines for readability):

in response: cannot read tag:
read tcp 192.168.2.3:51198->185.97.174.199:993:
read: connection reset by peer

Does not match the following condition:

errors.Is(err, syscall.ECONNRESET)

Calls to fmt.Errorf use %v, which embeds the inner error's string representation, without wrapping the inner error.

Use %w instead, to wrap the inner error, enabling usages such as the example above.

The following error (split here into multiple lines for readability):

    in response: cannot read tag:
    read tcp 192.168.2.3:51198->185.97.174.199:993:
    read: connection reset by peer

Does not match the following condition:

    errors.Is(err, syscall.ECONNRESET)

Calls to fmt.Errorf use `%v`, which embeds the inner error's string
representation, without wrapping the inner error.

Use `%w` instead, to wrap the inner error, enabling usages such as the
example above.
@emersion
Copy link
Owner

In principle, sounds fine to me, but see the discussion in #685

@WhyNotHugo
Copy link
Contributor Author

I hadn't thought of the fact that the become part of the API.

I believe that StatusResponse is the only public error declared in this library, and exposing that one sounds perfectly reasonable.

Other errors all seem to come from the stdlib and are quite likely to remain stable. E.g. Decoder.Error gets values such as io.ErrUnexpectedEOF which should not change.

I wanted to keep my changes focused, but simply wrapping the error in in response: cannot read tag: %w is insufficient: the error assigned to Decoder.err also wraps another error, and I think a large amount of code paths can yield syscall.ECONNRESET.

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.

2 participants