From 124b9a6ff89da8be83a256135ec6c4d0603e9a6f Mon Sep 17 00:00:00 2001 From: Alex Trotsenko Date: Sat, 21 Jan 2017 18:09:11 +0200 Subject: [PATCH] Improve suppression of spurious socket notifications under Windows There were still two cases where spurious notifications would be possible: - user calls hasPendingDatagrams()/pendingDatagramSize() on UDP socket somewhere outside the slot connected to readyRead() signal (::WSARecvFrom posts FD_READ notification, even if a notification for incoming datagram already exists in the message queue); - a socket was registered to receive several types of event and WM_QT_ACTIVATENOTIFIERS message is located between the different events for this socket in the queue. Provided patch ensures that the message queue is synchronized with the Qt event processing mechanism and adds a way to detect spurious notifications inside the window procedure. Task-number: QTBUG-58214 Change-Id: I49609dace601f300de09875ff1653617efabd72f Reviewed-by: Oswald Buddenhagen Reviewed-by: Lars Knoll Reviewed-by: Oliver Wolff Reviewed-by: Edward Welbourne Reviewed-by: Peter Seiderer --- src/corelib/kernel/qeventdispatcher_win.cpp | 52 +++++++++++++------ src/corelib/kernel/qeventdispatcher_win_p.h | 3 +- .../qsocketnotifier/tst_qsocketnotifier.cpp | 5 ++ 3 files changed, 43 insertions(+), 17 deletions(-) diff --git a/src/corelib/kernel/qeventdispatcher_win.cpp b/src/corelib/kernel/qeventdispatcher_win.cpp index 74fa2d8d500..40db5020abd 100644 --- a/src/corelib/kernel/qeventdispatcher_win.cpp +++ b/src/corelib/kernel/qeventdispatcher_win.cpp @@ -177,15 +177,24 @@ LRESULT QT_WIN_CALLBACK qt_internal_proc(HWND hwnd, UINT message, WPARAM wp, LPA QSNDict *dict = sn_vec[type]; QSockNot *sn = dict ? dict->value(wp) : 0; - if (sn) { - d->doWsaAsyncSelect(sn->fd, 0); - d->active_fd[sn->fd].selected = false; + if (sn == nullptr) { d->postActivateSocketNotifiers(); - if (type < 3) { - QEvent event(QEvent::SockAct); - QCoreApplication::sendEvent(sn->obj, &event); - } else { - QEvent event(QEvent::SockClose); + } else { + Q_ASSERT(d->active_fd.contains(sn->fd)); + QSockFd &sd = d->active_fd[sn->fd]; + if (sd.selected) { + Q_ASSERT(sd.mask == 0); + d->doWsaAsyncSelect(sn->fd, 0); + sd.selected = false; + } + d->postActivateSocketNotifiers(); + + // Ignore the message if a notification with the same type was + // received previously. Suppressed message is definitely spurious. + const long eventCode = WSAGETSELECTEVENT(lp); + if ((sd.mask & eventCode) != eventCode) { + sd.mask |= eventCode; + QEvent event(type < 3 ? QEvent::SockAct : QEvent::SockClose); QCoreApplication::sendEvent(sn->obj, &event); } } @@ -194,13 +203,22 @@ LRESULT QT_WIN_CALLBACK qt_internal_proc(HWND hwnd, UINT message, WPARAM wp, LPA } else if (message == WM_QT_ACTIVATENOTIFIERS) { Q_ASSERT(d != 0); - // register all socket notifiers - for (QSFDict::iterator it = d->active_fd.begin(), end = d->active_fd.end(); - it != end; ++it) { - QSockFd &sd = it.value(); - if (!sd.selected) { - d->doWsaAsyncSelect(it.key(), sd.event); - sd.selected = true; + // Postpone activation if we have unhandled socket notifier messages + // in the queue. WM_QT_ACTIVATENOTIFIERS will be posted again as a result of + // event processing. + MSG msg; + if (!PeekMessage(&msg, 0, WM_QT_SOCKETNOTIFIER, WM_QT_SOCKETNOTIFIER, PM_NOREMOVE) + && d->queuedSocketEvents.isEmpty()) { + // register all socket notifiers + for (QSFDict::iterator it = d->active_fd.begin(), end = d->active_fd.end(); + it != end; ++it) { + QSockFd &sd = it.value(); + if (!sd.selected) { + d->doWsaAsyncSelect(it.key(), sd.event); + // allow any event to be accepted + sd.mask = 0; + sd.selected = true; + } } } d->activateNotifiersPosted = false; @@ -706,7 +724,9 @@ void QEventDispatcherWin32::registerSocketNotifier(QSocketNotifier *notifier) } sd.event |= event; } else { - d->active_fd.insert(sockfd, QSockFd(event)); + // Disable the events which could be implicitly re-enabled. Next activation + // of socket notifiers will reset the mask. + d->active_fd.insert(sockfd, QSockFd(event, FD_READ | FD_ACCEPT | FD_WRITE | FD_OOB)); } d->postActivateSocketNotifiers(); diff --git a/src/corelib/kernel/qeventdispatcher_win_p.h b/src/corelib/kernel/qeventdispatcher_win_p.h index 423dc5b1697..f6d1bffdf54 100644 --- a/src/corelib/kernel/qeventdispatcher_win_p.h +++ b/src/corelib/kernel/qeventdispatcher_win_p.h @@ -126,9 +126,10 @@ typedef QHash QSNDict; struct QSockFd { long event; + long mask; bool selected; - explicit inline QSockFd(long ev = 0) : event(ev), selected(false) { } + explicit inline QSockFd(long ev = 0, long ma = 0) : event(ev), mask(ma), selected(false) { } }; typedef QHash QSFDict; diff --git a/tests/auto/corelib/kernel/qsocketnotifier/tst_qsocketnotifier.cpp b/tests/auto/corelib/kernel/qsocketnotifier/tst_qsocketnotifier.cpp index 1598382959f..a52b80170fb 100644 --- a/tests/auto/corelib/kernel/qsocketnotifier/tst_qsocketnotifier.cpp +++ b/tests/auto/corelib/kernel/qsocketnotifier/tst_qsocketnotifier.cpp @@ -360,11 +360,16 @@ void tst_QSocketNotifier::asyncMultipleDatagram() QSignalSpy spy(m_asyncReceiver, &QIODevice::readyRead); connect(m_asyncReceiver, &QIODevice::readyRead, this, &tst_QSocketNotifier::async_readDatagramSlot); + + // activate socket notifiers + QTestEventLoop::instance().enterLoopMSecs(100); + m_asyncSender->writeDatagram("1", makeNonAny(m_asyncReceiver->localAddress()), port); m_asyncSender->writeDatagram("2", makeNonAny(m_asyncReceiver->localAddress()), port); // wait a little to ensure that the datagrams we've just sent // will be delivered on receiver side. QTest::qSleep(100); + QVERIFY(m_asyncReceiver->hasPendingDatagrams()); QTimer::singleShot(500, this, &tst_QSocketNotifier::async_writeDatagramSlot);