Fix "multiple socket notifiers for same socket"

[FMeinicke]

Some background info:
I have multiple python applications that announce themselves using the python zeroconf implementation. I noticed that when the number of applications increases beyond ~20 I'm starting to get the warning "multiple socket notifiers for same socket XXXX and type Read" in my C++ client application. After some debugging, I found that only the addressNotifier of the Resolver class caused this warning. Looking at

https://github.com/jbagg/QtZeroConf/blob/74402496b07134808504ef5a45ae83d23f07cda9/bonjour.cpp#L182-L185

it seems to me that there are cases where the resolverCallback is called multiple times for the same Resolver. My first try was then just to add something similar that checks if the resolver->addressNotifier is already existing and has the same socket. If that's the case then the QSocketNotifier is also deleted. This works and I'm not getting any warnings anymore.

It's quite interesting to me, though, that the code does not seem to work without this addition even though the effect is essentially the same: QSharedPointer would delete the QSocketNotifier if it already existed in this line

https://github.com/jbagg/QtZeroConf/blob/74402496b07134808504ef5a45ae83d23f07cda9/bonjour.cpp#L193

The same is true for clear() here

            resolver->addressNotifier.clear();

The QSocketNotifier only gets deleted if this QSharedPointer had the last reference to it (which it is in this case).

[jbagg]

I would like to try and reproduce the warning. When you say you have multiple python application that announce themselves, are all those python applications on the same host or one app per host? What OS are you seeing the warning in (iOS or MacOS)?

[FMeinicke]

Yes, these python applications are all on the same host. To reproduce this I had those running on localhost (i.e the same machine that QtZeroConf was running on).
I'm seeing this on Windows.

[jbagg]

it seems to me that there are cases where the resolverCallback is called multiple times for the same Resolver

Yes, if a service's IP address changes, the resolver callback will fire.

It's quite interesting to me, though, that the code does not seem to work without this addition even though the effect is essentially the same: QSharedPointer would delete the QSocketNotifier if it already existed in this line

Yes but the order of operations is different. The current code creates another QSocketNotifier and then assigns that to the QSharedPointer, which then clears the original QSocketNotifier. For a brief moment, there are two QSocketNotifier's monitoring the same socket. Your change changes the order of operations, first clearing the original QSocketNotifier. I don't think it is necessary to have the conditional to clear the QSocketNotifier. Can you amend this pull request or create another so the change only includes the resolver->addressNotifier.clear(); and I will accept it.

[FMeinicke]

For a brief moment, there are two QSocketNotifier's monitoring the same socket. Your change changes the order of operations, first clearing the original QSocketNotifier.

Ahh, right.

I removed the conditional as you requested and force-pushed the branch again.

Thank you for your feedback and for looking into this. :)

[jbagg]

Thanks for putting in the time to investigate this.