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

Improve Pub/Sub MQTT driver context propagation #3343

Merged
merged 5 commits into from Oct 19, 2020

Conversation

adriansmares
Copy link
Contributor

@adriansmares adriansmares commented Oct 9, 2020

Summary

Closes #3338

Changes

  • Upgrade Paho to ba85050a1f23
  • Remove connection / publishing / subscription timeouts. Use context cancellations instead, as Paho recently added support for it (only await, no cancellation for started operations unfortunately).
  • Always disconnect the client if the connection attempt fails
  • Linking and Pub/Sub tasks no longer stop on registry errors (except NotFound errors) - we've actually observed this in production when the link to Redis stops, and the task stops
  • Do not cancel contexts with nil error. This makes code that uses ctx.Done and ctx.Err to miss the cancellation
  • Add keep-alive to MQTT connections

Testing

Unit testing covers this.

Regressions

As this touches the MQTT driver timeouts, and a bit of the linking/pub/sub startup logic, regressions can occur there. However, the changes actually make us more lenient in task restarts (since we now actually propagate the errors better).

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@adriansmares adriansmares added bug Something isn't working prio/high c/application server This is related to the Application Server labels Oct 9, 2020
@adriansmares adriansmares added this to the October 2020 milestone Oct 9, 2020
@adriansmares adriansmares self-assigned this Oct 9, 2020
@github-actions github-actions bot added dependencies Pull requests that update a dependency file documentation This involves writing user documentation labels Oct 9, 2020
Comment on lines -19 to -20
// Remove this once https://github.com/eclipse/paho.mqtt.golang/pull/451 is merged.
replace github.com/eclipse/paho.mqtt.golang => github.com/johanstokking/paho.mqtt.golang v1.2.1-0.20200917115254-cf2daad9593e
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably smart to keep a replacement to the latest master, otherwise a dependency upgrade may accidentally revert it to v1.2.0.

@adriansmares adriansmares merged commit 56474a2 into v3.10 Oct 19, 2020
@adriansmares adriansmares deleted the fix/3338-paho-wait-timeout branch October 19, 2020 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working c/application server This is related to the Application Server dependencies Pull requests that update a dependency file documentation This involves writing user documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Paho MQTT WaitTimeout usage in Pub/Sub
3 participants