-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
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
Filter replaying unrelated retained MQTT messages when subscribing to share topics #88826
Merged
jbouwh
merged 18 commits into
home-assistant:dev
from
jbouwh:mqtt-block-replay-retained
May 12, 2023
Merged
Filter replaying unrelated retained MQTT messages when subscribing to share topics #88826
jbouwh
merged 18 commits into
home-assistant:dev
from
jbouwh:mqtt-block-replay-retained
May 12, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hey there @emontnemery, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
3b29a88
to
d6348ec
Compare
d6348ec
to
2bf5998
Compare
19 tasks
2bf5998
to
303b78a
Compare
What's preventing this from being moved out of draft? |
5ee75e8
to
275f9e4
Compare
@corporategoth I have reworked the replaying so it will also work for wildcard topics, do you think this will work? |
corporategoth
approved these changes
May 4, 2023
6d78632
to
8448a31
Compare
5 tasks
emontnemery
reviewed
May 12, 2023
emontnemery
approved these changes
May 12, 2023
Co-authored-by: Erik Montnemery <erik@montnemery.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
by-code-owner
cla-signed
code-quality
core
has-tests
integration: mqtt
Quality Scale: gold
small-pr
PRs with less than 30 lines.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed change
The handling of MQTT messages with the
retain
flag has changed with this PR.Each time a new subscription is created to a shared topic, existing subscriptions that already received a retained payload will no longer receive the replayed retained payload except when re-connecting to the broker.
A retain flag is added to a received message when we are (re)subscribing to the broker and (some) retained payload(s) available. The broker will replay these payloads directly after (re)subscribing—also, the same behavior occurs when the connection is lost, and we are reconnecting. When multiple subscriptions subscribe to the same topic, all shared subscriptions receive retained messages. Till this point, the behavior does not change.
When another subscription with
shared
topic(s) is added, we will subscribe to the broker to ensure we receive any retained messages for that new subscription, existing subscriptions (that already received a retained payload and were initialized earlier) will no longer receive the replayed retained payload triggered by the new subscribes.For wildcards this works different, as there might be more than one payload. for the same subscription.
Basically the change effects shared subscriptions that receive a retained payload on a specific topic. When another shared subscription is added, we resubscribe to make sure any retained payloads are replayed. We only want to receive a retained payload once for a subscription per topic, so we filter out retained payloads when a new subscription is added. When reconnecting we allow replaying retained payloads for all subscriptions once again. I think we should filter out retained payloads for existing subscriptions to avoid the unexpected side effect on current subscriptions to receive a retained payload while that is not expected.
This part is communicated via a developers blogpost: home-assistant/developers.home-assistant#1775
The way how MQTT messages with a
retain
flag are handled has changed. Basically a retain flag is added to a received message when we are (re)subscribing to the broker and retained messages are available. The broker will replay these payloads directly after (re)subscribing. Also when the connection was lost, and we are reconnecting, the same behavior is seen. When there are multiple subscriptions that subscribe to the same topic, all shared subscriptions are receiving the retained messages. Up until this point the behavior has not changed.When another subscription with
shared
topic(s) is added, we will resubscribe to the broker to ensure we receive any retained messages for that new subscription. Existing subscriptions (that already received a retained payload and were initialized earlier) will no longer receive the replayed retained payload triggered by the new subscribes. This will improve performance.Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: