Skip to content

Conversation

@lefinal
Copy link
Contributor

@lefinal lefinal commented Feb 9, 2024

The check interval calculation in the ping.go file has been adjusted for when the keep-alive time is less than or equal to 10. Instead of dividing by 2, the check interval now is calculated with a division by 4, ensuring a more frequent ping check. This is required as MQTT brokers typically detect a timeout if the client did not send a ping within the specified keep-alive interval times 1.5. If the client only checks for due pings every half-interval, this commonly leads to timeouts. An example: a 5 second interval means that every 2.5 seconds checks are made for due pings. However, if checks have occurred at 2.49 seconds since the last ping and 4.99 seconds, the next one might happen slightly after 7.5 seconds, making the broker detect a timeout. Therefore, we increase the frequency of ping checks by reducing the check interval.

@lefinal lefinal force-pushed the fix-keep-alive-timeouts branch from 2d8a8e6 to dd9e000 Compare February 9, 2024 16:14
The check interval calculation in the `ping.go` file has been adjusted for when the keep-alive time is less than or equal to 10. Instead of dividing by 2, the check interval now is calculated with a division by 4, ensuring a more frequent ping check. This is required as MQTT brokers typically detect a timeout if the client did not send a ping within the specified keep-alive interval times 1.5. If the client only checks for due pings every half-interval, this commonly leads to timeouts. An example: a 5 second interval means that every 2.5 seconds checks are made for due pings. However, if checks have occurred at 2.49 seconds since the last ping and 4.99 seconds, the next one might happen slightly after 7.5 seconds, making the broker detect a timeout. Therefore, we increase the frequency of ping checks by reducing the check interval.
@lefinal lefinal force-pushed the fix-keep-alive-timeouts branch from dd9e000 to 08d0637 Compare February 9, 2024 16:16
@lefinal
Copy link
Contributor Author

lefinal commented Feb 9, 2024

Oh and in case this isn't already clear: This might be considered a critical bug as the library is nearly unusable with keep-alive timeouts below 10-11 seconds. So I'd be very happy if this is released as soon as possible (: Thanks guys!

@MattBrittan MattBrittan merged commit f21bdb1 into eclipse-paho:master Feb 9, 2024
@MattBrittan
Copy link
Contributor

Thanks for the fix.

MQTT brokers typically detect a timeout if the client did not send a ping within the specified keep-alive interval times 1.5.

This is mandated in the spec "...within one and a half times the Keep Alive time period, it MUST disconnect...".

An example: a 5 second interval means that every 2.5 seconds checks are made for due pings. However, if checks have occurred at 2.49 seconds since the last ping and 4.99 seconds, the next one might happen slightly after 7.5 seconds, making the broker detect a timeout. Therefore, we increase the frequency of ping checks by reducing the check interval.

This is likely to be fairly common given that pings are sent/checked on the same ticker i.e.

0.00 - timer fired
0.01 - ping sent (triggered by same timer so generally happens quickly)
2.50 - timer fired (no ping due; assume response received)
5.00 - timer fired (no ping due - 5.00 - 0.01 = 4.99)
7.50 - Timer fired (ping due; but this is now 7.49s after the last ping, so quite likely broker will drop connection)

This might be considered a critical bug

The algorithm (with minor changes) has been in place since 2017, so this has obviously not been too much of an issue (I suspect that very few users require such frequent pings). As such, I'm accepting the PR but don't intend to issue a release because of it (you can just use go get github.com/eclipse/paho.mqtt.golang@master for the time being).

Note that the v5 client uses a different approach ,that I believe will result in more accurate checks. It might be worth considering moving to that model here (but the code there is pretty new so no where near as well tested as this).

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