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

fix: issue 675:goroutine leak when connectionUp(true) return error #678

Merged
merged 2 commits into from
May 20, 2024

Conversation

kiqi007
Copy link
Contributor

@kiqi007 kiqi007 commented May 19, 2024

bug fix: #675

  • Skip the check for Disconnect when the connection is being established.
  • Treat this scenario (Disconnect during connection) as if the Disconnect occurred after connectionUp(true) completed.

client.go Outdated
* as if a Disconnect event occurred immediately after connectionUp(true) completed.
*/

//close(c.stop) // Tidy up anything we have already started
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in summary, this PR will change things so that:

if a connection is in progress when Disconnect is called then the connection attempt will be completed (including calling OnConnect and spinning up goroutines) and then the Disconnect operation will proceed after this (so, effectively a normal disconnect). In this case connectionStatus.Disconnecting() will wait until the connection process has completed before returning (and allowing the disconnection process to proceed). startCommsWorkers maintains a lock on connMu throughout the process, so stopCommsWorkers will be blocked until it completes.

My feeling is that this should work (but it's quite difficult to trace all possibilities). However I'd prefer to see the old code removed rather than being commented out (it will be available in the git history) and would like to see a comment something like the above (i.e. we will allow the connection to complete before the Disconnect is allowed to proceed).

I think I'm OK to accept this but if you are able to add something like the following I'd appreciate it. Adding a test for this would be absolutely fantastic (but I'm thinking it might be quite difficult to write).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, I have followed your suggestions.
For testing, I simulated this scenario locally using the method I previously used to debug the issue, and it works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

For testing, I simulated this scenario locally using the method I previously used to debug the issue, and it works.

This comment was more about adding a test (that does not pass with the old code but does with the new). The main purpose of this is to help prevent regressions (the state management stuff is pretty complex and Disconnect is not used all that often so it would be east to reintroduce the issue). Anyway I'll accept this PR now and if you do have a test that could be added later.

@MattBrittan MattBrittan merged commit 6801721 into eclipse-paho:master May 20, 2024
1 check passed
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.

goroutine leak when connectionUp(true) return error
2 participants