eclipse / paho.mqtt.golang

Other
2.73k stars 533 forks source link

Wrap connection network errors #646

Closed adriansmares closed 1 year ago

adriansmares commented 1 year ago

This PR implements error wrapping for the connection network errors, in order to allow them to be asserted using the errors package, specifically errors.Is.

Pretty much all of the boiler plate proposed in this PR can be handled using Go 1.20 multiple error wrapping, but since that's a 1.20+ feature, we manually handle this instead. When Go 1.20 becomes the minimum supported version, most of this PR can be reduced to fmt.Errorf("%w : %w", packets.ConnErrors[rc], err).

Some formatting is changed due to the fact that I automatically format files using gofumpt - please let me know if you would like for these changes to be left out.

MattBrittan commented 1 year ago

Was concerned that this is a breaking change because ErrorNetworkError is public. However reviewing the code we never return an error of that type due to the check:

// Maintain same error format as used previously
if rc != packets.ErrNetworkError { // mqtt error
    err = packets.ConnErrors[rc]
} else { // network error (if this occurred in ConnectMQTT then err will be nil)
    err = fmt.Errorf("%s : %s", packets.ConnErrors[rc], err)
}

As a result I think we are OK with this.

However the change does add quite a bit of code which, as you say, is not required if we drop support for earlier versions of Go. With the release of 1.21 I don't have a problem with following the Go team policy and only supporting 1.20 and 1.21. As such might I suggest that we go with the simpler version and Go 1.20? (I would accept into master but not push out a new release for a month or so to provide time for people to migrate off 1.18/1.19; I think most go devs update relatively quickly).

adriansmares commented 1 year ago

As such might I suggest that we go with the simpler version and Go 1.20?

I have converted this PR to the Go 1.20 multi wrap format, and updated the minimum Go version to 1.20, along with the dependencies.

I have tested if everything still works fine in case the err is nil - you can see this experiment here. I don't really like the output to be honest - I would prefer that we wrap only if err != nil, but let me know your thoughts on this.

I would argue that this error was generally just a string, not really comparable, and we can break the format and only wrap when err != nil - this would only break clients that compare the error as a string, which maybe is acceptable.