Fix the spurious socket notifications under Windows

To handle network events, QEventDispatcherWin32 uses I/O model
based on notifications through the window message queue. Having
successfully posted notification of a particular event to an
application window, no further messages for that network event
will be posted to the application window until the application
makes the function call that implicitly re-enables notification
of that network event. With these semantics, an application need
not read all available data in response to an FD_READ message:
a single recv in response to each FD_READ message is appropriate.
If an application issues multiple recv calls in response to a
single FD_READ, it can receive multiple FD_READ messages
(including spurious).

To solve this issue, this patch always disables the notifier
after getting a notification, and re-enables it only when the
message queue is empty.

Task-number: QTBUG-46552
Change-Id: I05df67032911cd1f5927fa7912f7864bfbf8711e
Reviewed-by: Joerg Bornemann <joerg.bornemann@theqtcompany.com>

src/corelib/kernel/qeventdispatcher_win.cpp

@@ -391,6 +391,8 @@ LRESULT QT_WIN_CALLBACK qt_internal_proc(HWND hwnd, UINT message, WPARAM wp, LPA
 
             QSockNot *sn = dict ? dict->value(wp) : 0;
             if (sn) {
+                d->doWsaAsyncSelect(sn->fd, 0);
+                d->active_fd[sn->fd].selected = false;
                 if (type < 3) {
                     QEvent event(QEvent::SockAct);
                     QCoreApplication::sendEvent(sn->obj, &event);
@@ -633,19 +635,12 @@ void QEventDispatcherWin32Private::sendTimerEvent(int timerId)
     }
 }
 
-void QEventDispatcherWin32Private::doWsaAsyncSelect(int socket)
+void QEventDispatcherWin32Private::doWsaAsyncSelect(int socket, long event)
 {
     Q_ASSERT(internalHwnd);
-    int sn_event = 0;
+    // BoundsChecker may emit a warning for WSAAsyncSelect when event == 0
-    if (sn_read.contains(socket))
-        sn_event |= FD_READ | FD_CLOSE | FD_ACCEPT;
-    if (sn_write.contains(socket))
-        sn_event |= FD_WRITE | FD_CONNECT;
-    if (sn_except.contains(socket))
-        sn_event |= FD_OOB;
-    // BoundsChecker may emit a warning for WSAAsyncSelect when sn_event == 0
     // This is a BoundsChecker bug and not a Qt bug
-    WSAAsyncSelect(socket, internalHwnd, sn_event ? int(WM_QT_SOCKETNOTIFIER) : 0, sn_event);
+    WSAAsyncSelect(socket, internalHwnd, event ? int(WM_QT_SOCKETNOTIFIER) : 0, event);
 }
 
 void QEventDispatcherWin32::createInternalHwnd()
@@ -658,13 +653,6 @@ void QEventDispatcherWin32::createInternalHwnd()
 
     installMessageHook();
 
-    // register all socket notifiers
-    QList<int> sockets = (d->sn_read.keys().toSet()
-                          + d->sn_write.keys().toSet()
-                          + d->sn_except.keys().toSet()).toList();
-    for (int i = 0; i < sockets.count(); ++i)
-        d->doWsaAsyncSelect(sockets.at(i));
-
     // start all normal timers
     for (int i = 0; i < d->timerVec.count(); ++i)
         d->registerTimer(d->timerVec.at(i));
@@ -749,28 +737,40 @@ bool QEventDispatcherWin32::processEvents(QEventLoop::ProcessEventsFlags flags)
                 msg = d->queuedSocketEvents.takeFirst();
             } else {
                 haveMessage = PeekMessage(&msg, 0, 0, 0, PM_REMOVE);
-                if (haveMessage && (flags & QEventLoop::ExcludeUserInputEvents)
+                if (haveMessage) {
-                    && ((msg.message >= WM_KEYFIRST
+                    if ((flags & QEventLoop::ExcludeUserInputEvents)
-                         && msg.message <= WM_KEYLAST)
+                        && ((msg.message >= WM_KEYFIRST
-                        || (msg.message >= WM_MOUSEFIRST
+                             && msg.message <= WM_KEYLAST)
-                            && msg.message <= WM_MOUSELAST)
+                            || (msg.message >= WM_MOUSEFIRST
-                        || msg.message == WM_MOUSEWHEEL
+                                && msg.message <= WM_MOUSELAST)
-                        || msg.message == WM_MOUSEHWHEEL
+                            || msg.message == WM_MOUSEWHEEL
-                        || msg.message == WM_TOUCH
+                            || msg.message == WM_MOUSEHWHEEL
+                            || msg.message == WM_TOUCH
 #ifndef QT_NO_GESTURES
-                        || msg.message == WM_GESTURE
+                            || msg.message == WM_GESTURE
-                        || msg.message == WM_GESTURENOTIFY
+                            || msg.message == WM_GESTURENOTIFY
 #endif
-                        || msg.message == WM_CLOSE)) {
+                            || msg.message == WM_CLOSE)) {
-                    // queue user input events for later processing
+                        // queue user input events for later processing
-                    haveMessage = false;
+                        d->queuedUserInputEvents.append(msg);
-                    d->queuedUserInputEvents.append(msg);
+                        continue;
-                }
+                    }
-                if (haveMessage && (flags & QEventLoop::ExcludeSocketNotifiers)
+                    if ((flags & QEventLoop::ExcludeSocketNotifiers)
-                    && (msg.message == WM_QT_SOCKETNOTIFIER && msg.hwnd == d->internalHwnd)) {
+                        && (msg.message == WM_QT_SOCKETNOTIFIER && msg.hwnd == d->internalHwnd)) {
-                    // queue socket events for later processing
+                        // queue socket events for later processing
-                    haveMessage = false;
+                        d->queuedSocketEvents.append(msg);
-                    d->queuedSocketEvents.append(msg);
+                        continue;
+                    }
+                } else if (!(flags & QEventLoop::ExcludeSocketNotifiers)) {
+                    // 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;
+                        }
+                    }
                 }
             }
             if (!haveMessage) {
@@ -896,8 +896,25 @@ void QEventDispatcherWin32::registerSocketNotifier(QSocketNotifier *notifier)
     sn->fd  = sockfd;
     dict->insert(sn->fd, sn);
 
-    if (d->internalHwnd)
+    long event = 0;
-        d->doWsaAsyncSelect(sockfd);
+    if (d->sn_read.contains(sockfd))
+        event |= FD_READ | FD_CLOSE | FD_ACCEPT;
+    if (d->sn_write.contains(sockfd))
+        event |= FD_WRITE | FD_CONNECT;
+    if (d->sn_except.contains(sockfd))
+        event |= FD_OOB;
+
+    QSFDict::iterator it = d->active_fd.find(sockfd);
+    if (it != d->active_fd.end()) {
+        QSockFd &sd = it.value();
+        if (sd.selected) {
+            d->doWsaAsyncSelect(sockfd, 0);
+            sd.selected = false;
+        }
+        sd.event |= event;
+    } else {
+        d->active_fd.insert(sockfd, QSockFd(event));
+    }
 }
 
 void QEventDispatcherWin32::unregisterSocketNotifier(QSocketNotifier *notifier)
@@ -916,6 +933,19 @@ void QEventDispatcherWin32::unregisterSocketNotifier(QSocketNotifier *notifier)
 #endif
 
     Q_D(QEventDispatcherWin32);
+    QSFDict::iterator it = d->active_fd.find(sockfd);
+    if (it != d->active_fd.end()) {
+        QSockFd &sd = it.value();
+        if (sd.selected)
+            d->doWsaAsyncSelect(sockfd, 0);
+        const long event[3] = { FD_READ | FD_CLOSE | FD_ACCEPT, FD_WRITE | FD_CONNECT, FD_OOB };
+        sd.event ^= event[type];
+        if (sd.event == 0)
+            d->active_fd.erase(it);
+        else
+            sd.selected = false;
+    }
+
     QSNDict *sn_vec[3] = { &d->sn_read, &d->sn_write, &d->sn_except };
     QSNDict *dict = sn_vec[type];
     QSockNot *sn = dict->value(sockfd);
@@ -924,9 +954,6 @@ void QEventDispatcherWin32::unregisterSocketNotifier(QSocketNotifier *notifier)
 
     dict->remove(sockfd);
     delete sn;
-
-    if (d->internalHwnd)
-        d->doWsaAsyncSelect(sockfd);
 }
 
 void QEventDispatcherWin32::registerTimer(int timerId, int interval, Qt::TimerType timerType, QObject *object)
@@ -1165,6 +1192,7 @@ void QEventDispatcherWin32::closingDown()
         unregisterSocketNotifier((*(d->sn_write.begin()))->obj);
     while (!d->sn_except.isEmpty())
         unregisterSocketNotifier((*(d->sn_except.begin()))->obj);
+    Q_ASSERT(d->active_fd.isEmpty());
 
     // clean up any timers
     for (int i = 0; i < d->timerVec.count(); ++i)

src/corelib/kernel/qeventdispatcher_win_p.h

@@ -115,6 +115,14 @@ struct QSockNot {
 };
 typedef QHash<int, QSockNot *> QSNDict;
 
+struct QSockFd {
+    long event;
+    bool selected;
+
+    explicit inline QSockFd(long ev = 0) : event(ev), selected(false) { }
+};
+typedef QHash<int, QSockFd> QSFDict;
+
 struct WinTimerInfo {                           // internal timer info
     QObject *dispatcher;
     int timerId;
@@ -169,7 +177,8 @@ public:
     QSNDict sn_read;
     QSNDict sn_write;
     QSNDict sn_except;
-    void doWsaAsyncSelect(int socket);
+    QSFDict active_fd;
+    void doWsaAsyncSelect(int socket, long event);
 
     QList<QWinEventNotifier *> winEventNotifierList;
     void activateEventNotifier(QWinEventNotifier * wen);

src/corelib/kernel/qsocketnotifier.cpp

@@ -98,42 +98,6 @@ public:
     QTcpSocket and QUdpSocket provide notification through signals, so
     there is normally no need to use a QSocketNotifier on them.
 
-    \section1 Notes for Windows Users
-
-    The socket passed to QSocketNotifier will become non-blocking, even if
-    it was created as a blocking socket.
-    The activated() signal is sometimes triggered by high general activity
-    on the host, even if there is nothing to read. A subsequent read from
-    the socket can then fail, the error indicating that there is no data
-    available (e.g., \c{WSAEWOULDBLOCK}). This is an operating system
-    limitation, and not a bug in QSocketNotifier.
-
-    To ensure that the socket notifier handles read notifications correctly,
-    follow these steps when you receive a notification:
-
-    \list 1
-    \li Disable the notifier.
-    \li Read data from the socket.
-    \li Re-enable the notifier if you are interested in more data (such as after
-       having written a new command to a remote server).
-    \endlist
-
-    To ensure that the socket notifier handles write notifications correctly,
-    follow these steps when you receive a notification:
-
-    \list 1
-    \li Disable the notifier.
-    \li Write as much data as you can (before \c EWOULDBLOCK is returned).
-    \li Re-enable notifier if you have more data to write.
-    \endlist
-
-    \b{Further information:}
-    On Windows, Qt always disables the notifier after getting a notification,
-    and only re-enables it if more data is expected. For example, if data is
-    read from the socket and it can be used to read more, or if reading or
-    writing is not possible because the socket would block, in which case
-    it is necessary to wait before attempting to read or write again.
-
     \sa QFile, QProcess, QTcpSocket, QUdpSocket
 */
 

tests/auto/corelib/kernel/qsocketnotifier/tst_qsocketnotifier.cpp

@@ -40,6 +40,7 @@
 #include <QtCore/QSocketNotifier>
 #include <QtNetwork/QTcpServer>
 #include <QtNetwork/QTcpSocket>
+#include <QtNetwork/QUdpSocket>
 #ifndef Q_OS_WINRT
 #include <private/qnativesocketengine_p.h>
 #else
@@ -66,8 +67,29 @@ private slots:
 #ifdef Q_OS_UNIX
     void posixSockets();
 #endif
+    void asyncMultipleDatagram();
+
+protected slots:
+    void async_readDatagramSlot();
+    void async_writeDatagramSlot();
+
+private:
+    QUdpSocket *m_asyncSender;
+    QUdpSocket *m_asyncReceiver;
 };
 
+static QHostAddress makeNonAny(const QHostAddress &address,
+                               QHostAddress::SpecialAddress preferForAny = QHostAddress::LocalHost)
+{
+    if (address == QHostAddress::Any)
+        return preferForAny;
+    if (address == QHostAddress::AnyIPv4)
+        return QHostAddress::LocalHost;
+    if (address == QHostAddress::AnyIPv6)
+        return QHostAddress::LocalHostIPv6;
+    return address;
+}
+
 class UnexpectedDisconnectTester : public QObject
 {
     Q_OBJECT
@@ -299,5 +321,56 @@ void tst_QSocketNotifier::posixSockets()
 }
 #endif
 
+void tst_QSocketNotifier::async_readDatagramSlot()
+{
+    char buf[1];
+    QVERIFY(m_asyncReceiver->hasPendingDatagrams());
+    do {
+        QCOMPARE(m_asyncReceiver->pendingDatagramSize(), qint64(1));
+        QCOMPARE(m_asyncReceiver->readDatagram(buf, sizeof(buf)), qint64(1));
+        if (buf[0] == '1') {
+            // wait for the second datagram message.
+            QTest::qSleep(100);
+        }
+    } while (m_asyncReceiver->hasPendingDatagrams());
+
+    if (buf[0] == '3')
+        QTestEventLoop::instance().exitLoop();
+}
+
+void tst_QSocketNotifier::async_writeDatagramSlot()
+{
+    m_asyncSender->writeDatagram("3", makeNonAny(m_asyncReceiver->localAddress()),
+                                                 m_asyncReceiver->localPort());
+}
+
+void tst_QSocketNotifier::asyncMultipleDatagram()
+{
+    m_asyncSender = new QUdpSocket;
+    m_asyncReceiver = new QUdpSocket;
+
+    QVERIFY(m_asyncReceiver->bind(QHostAddress(QHostAddress::AnyIPv4), 0));
+    quint16 port = m_asyncReceiver->localPort();
+    QVERIFY(port != 0);
+
+    QSignalSpy spy(m_asyncReceiver, &QIODevice::readyRead);
+    connect(m_asyncReceiver, &QIODevice::readyRead, this,
+            &tst_QSocketNotifier::async_readDatagramSlot);
+    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);
+
+    QTimer::singleShot(500, this, &tst_QSocketNotifier::async_writeDatagramSlot);
+
+    QTestEventLoop::instance().enterLoop(1);
+    QVERIFY(!QTestEventLoop::instance().timeout());
+    QCOMPARE(spy.count(), 2);
+
+    delete m_asyncSender;
+    delete m_asyncReceiver;
+}
+
 QTEST_MAIN(tst_QSocketNotifier)
 #include <tst_qsocketnotifier.moc>