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.

Sign up for GitHub

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

Jump to bottom

[mqtt] Discovery services shall not unsubscribe unless they have already subscribed #10566

Merged
merged 5 commits into from
Aug 8, 2021
Merged

[mqtt] Discovery services shall not unsubscribe unless they have already subscribed #10566

merged 5 commits into from
Aug 8, 2021

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Apr 23, 2021 •
edited
Loading

The MQTT AbstractBrokerHandler produces erroneous logger warnings when OH is being shutdown for the EspMilight, HomeAssistant, and Homie, discovery services. See #10562

I think that the problem is because of some or all of the following issues:

  1. The three respective discovery participant classes extend AbstractMQTTDiscovery, and that class may be calling getDiscoveryService().unsubscribe() although it might not already have called getDiscoveryService().subscribe().
  2. The MqttBrokerHandlerFactory discoveryTopics Map allows duplicate entries of the same listener in each topic's listenerList.
  3. The MqttBrokerHandlerFactory subscribe method would re-subscribe the given listener/topic even if already subscribed.
  4. The MqttBrokerHandlerFactory unsubscribe method would unsubscribe ALL the topics (even if the listener was not in the listenerList for that topic) -- this IMHO is the most critical error in the current code.

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

@andrewfg
[mqqt] do not allow unsubscribe unless already subscribed
6d4b890 
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg added bug An unexpected problem or unintended behavior of an add-on help wanted additional testing preferred The change works for the pull request author. A test from someone else is preferred though. labels Apr 23, 2021
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/mqtt-strange-logger-warnings-on-oh-shutdown/121228/17

@andrewfg andrewfg linked an issue Apr 23, 2021 that may be closed by this pull request
[mqtt] Erroneous Shutdown Warnings for EspMilight, HomeAssistant, Homie Discovery Services #10562
Closed
@wborn wborn changed the title [mqqt] Don't allow discovery services to unsubscribe unless they have already subscribed [mqtt] Don't allow discovery services to unsubscribe unless they have already subscribed Apr 24, 2021
@andrewfg
[mqtt] avoid duplicates in listenerList; only (un)register topicd if …
c0d00c9 
…listener in listenerList

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg requested a review from a team April 25, 2021 14:11
@andrewfg
Copy link
Contributor Author

I found a way to install all the dependencies so I was able to test it :)
=> Removing the "additional testing required" flag

@andrewfg andrewfg removed additional testing preferred The change works for the pull request author. A test from someone else is preferred though. help wanted labels Apr 25, 2021
@andrewfg
[mqtt] rename "listenerList" local variables to "listeners"
1abceca 
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

shouldn't we rename the variable to 'listenerSet'

@jochen314 done.

@andrewfg andrewfg added this to the 3.1 milestone Apr 26, 2021
@andrewfg andrewfg changed the title [mqtt] Don't allow discovery services to unsubscribe unless they have already subscribed [mqtt] Discovery services will not unsubscribe unless they have already subscribed Apr 26, 2021
@andrewfg andrewfg changed the title [mqtt] Discovery services will not unsubscribe unless they have already subscribed [mqtt] Discovery services shall not unsubscribe unless they have already subscribed Apr 26, 2021
@andrewfg
[mqtt] added java docs
1aaa0bc 
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg mentioned this pull request Apr 28, 2021
Closed
jochen314
jochen314 reviewed May 2, 2021
View reviewed changes
Copy link
Contributor

@jochen314 jochen314 left a comment

Choose a reason for hiding this comment

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

If we now improve the thread saftey, it will look good to me

...hab.binding.mqtt/src/main/java/org/openhab/binding/mqtt/discovery/AbstractMQTTDiscovery.java Outdated Show resolved Hide resolved
...hab.binding.mqtt/src/main/java/org/openhab/binding/mqtt/discovery/AbstractMQTTDiscovery.java Outdated Show resolved Hide resolved
...hab.binding.mqtt/src/main/java/org/openhab/binding/mqtt/discovery/AbstractMQTTDiscovery.java Outdated Show resolved Hide resolved
...b.binding.mqtt/src/main/java/org/openhab/binding/mqtt/internal/MqttBrokerHandlerFactory.java Outdated Show resolved Hide resolved
...b.binding.mqtt/src/main/java/org/openhab/binding/mqtt/internal/MqttBrokerHandlerFactory.java Outdated Show resolved Hide resolved
@andrewfg
[mqtt] reviewer requested fixes to concurrency issues
d394f2f 
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
jochen314
jochen314 approved these changes May 2, 2021
View reviewed changes
Copy link
Contributor

@jochen314 jochen314 left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewfg andrewfg added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jun 7, 2021
@Skinah Skinah removed the rebuild Triggers Jenkins PR build label Jun 15, 2021
@andrewfg
Copy link
Contributor Author

PING!

@wborn wborn removed this from the 3.1 milestone Jun 28, 2021
@andrewfg
Copy link
Contributor Author

PING!!

@andrewfg andrewfg added this to the 3.2 milestone Jul 20, 2021
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh3-mqtt-somtimes-issues-after-starting/113104/19

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/broken-homeassistant-mqtt-discovery/125102/9

@Skinah
Copy link
Contributor

Skinah commented Aug 8, 2021

LGTM

As Code owner for mqtt.espmilighthub this has been a minor annoyance in the logs but have not seen any issues. From the description of this, it may be increasing system load if a listener can get subscribed multiple times, so thanks for making the changes.

kaikreuzer
kaikreuzer approved these changes Aug 8, 2021
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks @andrewfg!

@kaikreuzer kaikreuzer merged commit 2e8700e into openhab:main Aug 8, 2021
frederictobiasc pushed a commit to frederictobiasc/openhab-addons that referenced this pull request Oct 26, 2021
@andrewfg @frederictobiasc
[mqtt] Discovery services shall not unsubscribe unless they have alre…
5402df7 
…ady subscribed (openhab#10566)

* [mqqt] do not allow unsubscribe unless already subscribed

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
@andrewfg @thinkingstone
[mqtt] Discovery services shall not unsubscribe unless they have alre…
0e56dba 
…ady subscribed (openhab#10566)

* [mqqt] do not allow unsubscribe unless already subscribed

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
dschoepel pushed a commit to dschoepel/openhab-addons that referenced this pull request Nov 9, 2021
@andrewfg @dschoepel
[mqtt] Discovery services shall not unsubscribe unless they have alre…
7356585 
…ady subscribed (openhab#10566)

* [mqqt] do not allow unsubscribe unless already subscribed

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Dave J Schoepel <dave@theschoepels.com>
@andrewfg andrewfg deleted the mqtt-unsubscribe branch February 7, 2022 19:27
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
@andrewfg
[mqtt] Discovery services shall not unsubscribe unless they have alre…
6715e7a 
…ady subscribed (openhab#10566)

* [mqqt] do not allow unsubscribe unless already subscribed

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaikreuzer kaikreuzer kaikreuzer approved these changes

@jochen314 jochen314 jochen314 approved these changes

@davidgraeff davidgraeff Awaiting requested review from davidgraeff

Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Milestone
3.2
Development

Successfully merging this pull request may close these issues.

[mqtt] Erroneous Shutdown Warnings for EspMilight, HomeAssistant, Homie Discovery Services
6 participants
@andrewfg @openhab-bot @Skinah @kaikreuzer @jochen314 @wborn