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

Wrap connection network errors #646

Merged
merged 2 commits into from
Aug 10, 2023
Merged

Wrap connection network errors #646

merged 2 commits into from
Aug 10, 2023

Conversation

adriansmares
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

adriansmares commented Aug 10, 2023

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.

@MattBrittan MattBrittan merged commit 7b759f1 into eclipse-paho:master Aug 10, 2023
@adriansmares adriansmares deleted the feature/network-error-wrapping branch August 11, 2023 18:25
algitbot pushed a commit to alpinelinux/build-server-status that referenced this pull request Sep 16, 2024
This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/eclipse/paho.mqtt.golang](https://github.com/eclipse/paho.mqtt.golang) | require | minor | `v1.4.3` -> `v1.5.0` |

---

### Release Notes

<details>
<summary>eclipse/paho.mqtt.golang (github.com/eclipse/paho.mqtt.golang)</summary>

### [`v1.5.0`](https://github.com/eclipse/paho.mqtt.golang/releases/tag/v1.5.0)

[Compare Source](eclipse-paho/paho.mqtt.golang@v1.4.3...v1.5.0)

In the year since the release of v1.4.3 the majority of changes have been small incremental improvements/fixes. One notable change is that Go v1.20+ is now required (due to MR [#&#8203;646](eclipse-paho/paho.mqtt.golang#646)).

#### What's Changed

-   Wrap connection network errors by [@&#8203;adriansmares](https://github.com/adriansmares) in eclipse-paho/paho.mqtt.golang#646
-   Clarify use of token.WaitTimeout by [@&#8203;MattBrittan](https://github.com/MattBrittan) in eclipse-paho/paho.mqtt.golang#659
-   fix ([#&#8203;661](eclipse-paho/paho.mqtt.golang#661)): Add NewClientOptionsReader for mocking purposes. by [@&#8203;avmunm](https://github.com/avmunm) in eclipse-paho/paho.mqtt.golang#662
-   fix: fix keep-alive timeouts on small intervals by [@&#8203;lefinal](https://github.com/lefinal) in eclipse-paho/paho.mqtt.golang#667
-   Replace the time.After with the timer for efficiency. by [@&#8203;DVasselli](https://github.com/DVasselli) in eclipse-paho/paho.mqtt.golang#671
-   fix: deprecation warnings for ioutil by [@&#8203;vruge](https://github.com/vruge) in eclipse-paho/paho.mqtt.golang#665
-   fix: issue 675:goroutine leak when connectionUp(true) return error by [@&#8203;kiqi007](https://github.com/kiqi007) in eclipse-paho/paho.mqtt.golang#678
-   Update dependencies by [@&#8203;MattBrittan](https://github.com/MattBrittan) in eclipse-paho/paho.mqtt.golang#683

#### New Contributors

-   [@&#8203;adriansmares](https://github.com/adriansmares) made their first contribution in eclipse-paho/paho.mqtt.golang#646
-   [@&#8203;avmunm](https://github.com/avmunm) made their first contribution in eclipse-paho/paho.mqtt.golang#662
-   [@&#8203;lefinal](https://github.com/lefinal) made their first contribution in eclipse-paho/paho.mqtt.golang#667
-   [@&#8203;DVasselli](https://github.com/DVasselli) made their first contribution in eclipse-paho/paho.mqtt.golang#671
-   [@&#8203;vruge](https://github.com/vruge) made their first contribution in eclipse-paho/paho.mqtt.golang#665
-   [@&#8203;kiqi007](https://github.com/kiqi007) made their first contribution in eclipse-paho/paho.mqtt.golang#678

**Full Changelog**: eclipse-paho/paho.mqtt.golang@v1.4.3...v1.5.0

</details>

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

&nbsp;
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44MC4wIiwidXBkYXRlZEluVmVyIjoiMzguODAuMCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciIsImxhYmVscyI6W119-->

See merge request alpine/infra/build-server-status!15
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