openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
911 stars 420 forks source link

[MDNSDiscoveryService] potential bug deleting items from Inbox?? #1835

Closed andrewfg closed 2 years ago

andrewfg commented 3 years ago

I have been developing a binding that implements a MDNSDiscoveryParticipant to discover the bridge. One user reports that every time the participant add such a bridge to the Inbox, the core then deletes it again. I think this could be a bug in the MDNSDiscoveryService class..

public class MDNSDiscoveryService extends AbstractDiscoveryService implements ServiceListener {
..
    protected synchronized void stopScan() {
        removeOlderResults(getTimestampOfLastScan());
        super.stopScan();
    }
lolodomo commented 3 years ago

timestampOfLastScan is set by the abstract class just before calling the startScan method. removeOlderResults(getTimestampOfLastScan()); is here to delete any discovery that was not discovered by the last scan. So if the inbox entry is suppressed, that would mean that it is not seen by the scan. IMHO there is no problem with the content of this stopScan method.

andrewfg commented 3 years ago

Note: #2144 would resolve this

lolodomo commented 3 years ago

How something related to UPnP discovery could change something related to mDNS discovery?

andrewfg commented 3 years ago

I'm thinking the same logic about grace periods..

jlaur commented 2 years ago

Same problem for the Miele binding every two minutes:

2021-12-19 19:58:17.078 [INFO ] [openhab.event.InboxRemovedEvent     ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been removed.
2021-12-19 19:58:21.923 [INFO ] [g.discovery.internal.PersistentInbox] - Added new thing 'miele:xgw3000:Miele_XGW3000' to inbox.
2021-12-19 19:58:21.923 [INFO ] [openhab.event.InboxAddedEvent       ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been added.
2021-12-19 20:00:17.072 [INFO ] [openhab.event.InboxRemovedEvent     ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been removed.
2021-12-19 20:00:21.753 [INFO ] [openhab.event.InboxAddedEvent       ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been added.
2021-12-19 20:00:21.753 [INFO ] [g.discovery.internal.PersistentInbox] - Added new thing 'miele:xgw3000:Miele_XGW3000' to inbox.

Using the app Service Browser by Andriy Druk for Android I can see the service disappearing for a short moment every two minutes, completely synchronized with when it's removed and readded to the inbox in openHAB.

lolodomo commented 2 years ago

For UPnP, there was a possible grace period added.

Maybe something similar could be implemented ? Of course if the problem is similar. https://github.com/openhab/openhab-core/pull/2144

andrewfg commented 2 years ago

@lolodomo you beat me to it! I was also going to mention https://github.com/openhab/openhab-core/pull/2144

jlaur commented 2 years ago

@lolodomo, @andrewfg - thanks, I was actually reading that issue before I found this. I also came to the conclusion that this issue could probably be solved for mDNS in a similar way. Just wanted to mention that the Miele binding also suffers from this problem. I've been trying to hunt down the problem in MieleMDNSDiscoveryParticipant but finally gave up and realized that it can't be fixed within the binding. Unfortunately I don't feel quite ready yet to work on openhab-core, but will be happy to update and test the Miele binding accordingly if someone will pick up this issue.

andrewfg commented 2 years ago

don't feel quite ready yet to work on openhab-core,

Ok. I will look at it tomorrow. If I can rely on you to test it.

jlaur commented 2 years ago

@andrewfg - cool, thanks. I'll test it as soon as I possible can, Christmas holiday is within reach now. :-) I guess I better start figuring out how to build core and install new system in Windows or Docker...

andrewfg commented 2 years ago

@jlaur the final release of OH v3.2.x means that the maintainers updated the build system to OH v3.3.x so if you want to test things, you will need a spare PC running v3.3.x-SNAPSHOT

If you have such a system then HERE you will find two JAR files as follows..

You need a system running OH v3.3.x and you need to add those two JARs to the 'addons' folder. And you may also need to open the openhab console and bundle:uninstall the prior org.openhab.core.config.discovery.mdns jar resp. bundle:install the new version..

jlaur commented 2 years ago

@andrewfg - wow, that was fast. I'll try to have a go at it as soon as possible. Will probably try Windows version when 3.3 snapshot will become available (I checked, not there yet). Might also be an option to try on my production 3.2 system, if I can go back after. Do you know if the 3.3 version number will be a problem?

jlaur commented 2 years ago

@andrewfg - it was just a bad link at https://www.openhab.org/download/ pointing to non-existant 3.2 snapshot file. I have a new instance running now with your changes, will let you know in a moment how it turns out.

jlaur commented 2 years ago

@andrewfg - it works!

Restarted openHAB, manually removed thing from inbox: 2021-12-20 22:22:11.068 [INFO ] [openhab.event.InboxRemovedEvent ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been removed.

Was discovered again: 2021-12-20 22:22:20.934 [INFO ] [openhab.event.InboxAddedEvent ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been added.

Then 5-6 minutes of silence (normally it's removed and readded every two minutes). Unplugged network cable from gateway: 2021-12-20 22:29:39.937 [INFO ] [openhab.event.InboxRemovedEvent ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been removed.

And plugged in again: 2021-12-20 22:30:52.140 [INFO ] [openhab.event.InboxAddedEvent ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been added.

I believe it took a bit more than 30 seconds to have it removed, but I checked the code, and it looks correct. For sure it was removed within a few minutes.

andrewfg commented 2 years ago

@jlaur if you are happy with this, then I will open a PR (actually two, one for the core, and one for the binding)..

jlaur commented 2 years ago

@andrewfg - excellent, thanks again.