Fix race condition with for reinitializing after MQTT reconnect #17891
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
|
I now see that this was specifically for a user who doesn't want repeat state updates So I am kind of unsure what the best option is here... You may be better equipped to understand the pros/cons of all of the attached issues and feedback and decide would be best |
|
Can you try with just the following change: logger.info('Connected to MQTT server');
if (this.initialConnect) {
await this.publishStateOnline();
} else {
this.republishRetainedTimer = setTimeout(() => {
// Republish retained messages in case MQTT broker does not persist them.
// https://github.com/Koenkk/zigbee2mqtt/issues/9629
Object.values(this.retainedMessages).forEach((e) =>
this.publish(e.topic, e.payload, e.options, e.base, e.skipLog, e.skipReceive));
}, 2000);
}
this.initialConnect = false; |
Is that intended to be a direct revert of 6314b86 ? It works for me and I will leave it running, but I thought this causes other problems that I saw in the related issue threads. Maybe those were unrelated problems/fixes though related to mqtt stability |
… after a reconnect to mqtt
|
@Koenkk I am leaving the tests as failures for now so you can assess the repurcussions of the change Part of me thinks a config option for "republish_retained_messages_on_reconnect" with a default value of true might be a good idea, so then some people could disable it if they have persistent storage on their broker AND dont want duplicate messages |
|
I don't want to introduce this as an option since this goes quite deep into the working of Z2M/HA/MQTT. I think we should go the safe route and always republish, if it works for you the following is an acceptable solution for me (feel free to update the PR if it works and I will merge it). logger.info('Connected to MQTT server');
// Re-publish retained messages as broker might not persist them
// https://github.com/Koenkk/zigbee2mqtt/issues/9629
Object.values(this.retainedMessages).forEach((e) =>
this.publish(e.topic, e.payload, e.options, e.base, e.skipLog, e.skipReceive));
this.subscribe(`${settings.get().mqtt.base_topic}/#`); |
It works for me, but what about someone like this #9629 (comment) |
|
Ok, sounds good @Koenkk, updated the tests that go with that commit/change. Let me know if you want me to do further testing as well or if you want to take the reins/ownership on this. I am at least setup to do further testing and make any changes necessary but you have a better idea of all of the constraints |
|
@jeroen85 please check if #13382 is back with the latest dev branch, if yes provide me the debug logging. See https://www.zigbee2mqtt.io/guide/usage/debug.html on how to enable debug logging. |
|
Unfortunately, the issue is back in the dev release. As a double check; the current stable release works as expected. |
|
@jeroen85 how are you running z2m? (I want you to make some local changes to the code) |
|
Docker container on my Synology NAS |
|
Edit: I mixed up users |
|
@jeroen85 can you do the following:
this.republishRetainedTimer = setTimeout(() => {
// Republish retained messages in case MQTT broker does not persist them.
// https://github.com/Koenkk/zigbee2mqtt/issues/9629
logger_1.default.error('DEBUG: PUBLISHING RETAINED MESSAGES: ' + this.retainedMessages['zigbee2mqtt/bridge/state']); // <-- add this line
Object.values(this.retainedMessages).forEach((e) => this.publish(e.topic, e.payload, e.options, e.base, e.skipLog, e.skipReceive));
}, 2000);if (this.republishRetainedTimer && topic == `${settings.get().mqtt.base_topic}/bridge/state`) {
logger_1.default.error('DEBUG: CLEARING RETAINED MESSAGES TIMER ' + topic + ' ' + message); // <-- add this line
clearTimeout(this.republishRetainedTimer);
this.republishRetainedTimer = null;
}
|
|
Edit: I mixed up users |
|
@xconverge from my understanding, the problem that @jeroen85 is having is that the entities in HA stay unavailable after MQTT reconnect, this is because |
|
Yea sorry I got the user mixed up. I will bow out now sorry for the confusion |
|
Terrible program (vi), but I managed to make the changes. Only |
|
Is the issue fixed when removing 'clearTimeout(this.republishRetainedTimer);'? |
|
Yes |
|
I updated #17891 (comment) , can you provide the log with those changes? |
|
I kept the 'clearTimeout(this.republishRetainedTimer);' out. |
|
@xconverge could you also apply these changes and provide me the debug logging for your case? |
|
@jeroen85 just a hint but I prefer to do this instead of use vi in the container itself for my own sanity 😆 I see both messages, I am guessing the 2 seconds is not sufficient and maybe @jeroen85 has more going on from a cpu standpoint? Mine get published right away though |
Koenkk
added a commit
that referenced
this pull request
Jun 7, 2023
|
@xconverge can you try the fix from #17951 ? (@jeroen85 for you there is no need to test this one) |
|
Works for me |
Koenkk
added a commit
that referenced
this pull request
Jun 8, 2023
|
Great! @jeroen85 just to be sure, can you check if the latest-dev works fine for you? |
|
Checked, it works as expected :-) Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #9629
First related commit: 343ef7f
Second related commit that introduces the problem: 6314b86