search

icecc / icemon

Icecream GUI Monitor
http://kfunk.org/tag/icemon/
GNU General Public License v2.0
92 stars 35 forks source link

Explicitly disable socket notifiers before calling deleteLater #47

Closed alcroito closed 5 years ago

alcroito commented 5 years ago

This fixes the IcecreamMonitor to work on macOS with Qt 5.12.

Because registerNotify() gets called a second time with the same fd number, and tries to create a new QSocketNotifier before deleting / disabling the existing notifier, some macOS specific socket internals gets messed up, and no notifications go through.

This can be confirmed by using a debug build of Qt, which asserts in QCFSocketNotifier::registerSocketNotifier because there is an already existing notifier on the given file descriptor.

ossilator commented 5 years ago

the commit message makes it sound as if this was a qt bug (in which case you submitting a workaround would definitely have a significant irony factor ;) ), but i'm not convinced it is - we might have just been lucky on linux. @thiagomacieira or @ediosyncratic, opinions?

thiagomacieira commented 5 years ago

The patch actually looks correct. You're not allowed to create more than on QSocketNotifier per fd, per type. If you were deleting the QSocketNotifier directly, there would be no trouble. But since you use deleteLater(), you're probably creating the new one before the old one gets destroyed.

ossilator commented 5 years ago

thought so. but by extension that means that things would go boom later again when we apply the optimization we previously pondered so the low-level watcher is not discarded each time the notifier is disabled. it seems that instead the code should explicitly check whether the fd is the same as previously and recycle the notifier if it is.

thiagomacieira commented 5 years ago

You've got a point: you should not create a new QSocketNotifier for the same fd & type until the previous one is completely destroyed, whether it was enabled or not.

krf commented 5 years ago

Note: Icemon is looking for a new maintainer, please check if you'd like to be the person in charge: https://groups.google.com/forum/#!topic/icecream-users/pioozZ-vB0c

torarnv commented 5 years ago

Looks good, ready to land @krf?

krf commented 5 years ago

You've got a point: you should not create a new QSocketNotifier for the same fd & type until the previous one is completely destroyed, whether it was enabled or not.

@torarnv Saw Thiago's comment ^?

torarnv commented 5 years ago

Ah :/

llunak commented 5 years ago

Fixed by my commit.