WinDebugInterface: Remove blocking connection to main thread

The debugOutput() signal was connected via BlockingQueuedConnection and
blocks the WinDebugInterface thread until the signal is received. If we
signal "buffer ready" afterwards, all OutputDebugString calls (of all
processes of the system) that wait for "buffer ready" also wait for our
internal signal delivery. This can lead to hangs in circumstances where
the event loop is currently unable to deliver the signal.

Remove the blocking queued connection, and introduce an internal
_q_debugOutputReady() signal that is emitted whenever new debug output
is available. This signal is queued-connected to a dispatchDebugOutput
function, which is running in the main thread. That function retrieves
the data from the WinDebugInterfaceThread and emits debugOutput(qint64,
QString). This massively reduces the event queue load without blocking
the WinDebugInterface thread.

Task-number: QTCREATORBUG-20640
Change-Id: I91f8f794af8da2a695c2b897f678844b142a5991
Reviewed-by: David Schulz <david.schulz@qt.io>
Reviewed-by: Alessandro Portale <alessandro.portale@qt.io>
Reviewed-by: Eike Ziller <eike.ziller@qt.io>

3 files changed, 72 insertions, 34 deletions

diff --git a/src/plugins/projectexplorer/applicationlauncher.cpp b/src/plugins/projectexplorer/applicationlauncher.cpp
index d42109d2b4..804954d14e 100644
--- a/src/plugins/projectexplorer/applicationlauncher.cpp
+++ b/src/plugins/projectexplorer/applicationlauncher.cpp
@@ -160,7 +160,7 @@ ApplicationLauncherPrivate::ApplicationLauncherPrivate(ApplicationLauncher *pare
     connect(WinDebugInterface::instance(), &WinDebugInterface::cannotRetrieveDebugOutput,
             this, &ApplicationLauncherPrivate::cannotRetrieveLocalDebugOutput);
     connect(WinDebugInterface::instance(), &WinDebugInterface::debugOutput,
-            this, &ApplicationLauncherPrivate::checkLocalDebugOutput, Qt::BlockingQueuedConnection);
+            this, &ApplicationLauncherPrivate::checkLocalDebugOutput);
 #endif
 }
 
diff --git a/src/plugins/projectexplorer/windebuginterface.cpp b/src/plugins/projectexplorer/windebuginterface.cpp
index 98ece143c8..985fec933f 100644
--- a/src/plugins/projectexplorer/windebuginterface.cpp
+++ b/src/plugins/projectexplorer/windebuginterface.cpp
@@ -27,10 +27,11 @@
 
 #ifdef Q_OS_WIN
 
-#include <windows.h>
+#include <utils/qtcassert.h>
 #include <QCoreApplication>
-#include <QMap>
-#include <QTime>
+#include <qt_windows.h>
+
+#include <algorithm>
 
 /*!
     \class ProjectExplorer::Internal::WinDebugInterface
@@ -67,6 +68,8 @@ WinDebugInterface::WinDebugInterface(QObject *parent) :
     m_instance = this;
     m_creatorPid = QCoreApplication::applicationPid();
     setObjectName(QLatin1String("WinDebugInterfaceThread"));
+    connect(this, &WinDebugInterface::_q_debugOutputReady,
+            this, &WinDebugInterface::dispatchDebugOutput, Qt::QueuedConnection);
 }
 
 WinDebugInterface::~WinDebugInterface()
@@ -131,41 +134,18 @@ bool WinDebugInterface::runLoop()
 
     SetEvent(m_bufferReadyEvent);
 
-    QTime timer; // time since last signal sent
-    timer.start();
-
-    QMap<qint64, QString> delayedMessages;
-
-    auto flushMessages = [this, &delayedMessages, &timer](){
-        auto it = delayedMessages.constBegin();
-        auto end = delayedMessages.constEnd();
-        for (; it != end; ++it)
-            emit debugOutput(it.key(), it.value());
-        delayedMessages.clear();
-        timer.start();
-    };
-
     while (true) {
-        DWORD timeout = INFINITE;
-        if (!delayedMessages.isEmpty()) // if we have delayed message, don't wait forever
-            timeout = qMax(60 - timer.elapsed(), 1);
-        const DWORD ret = WaitForMultipleObjects(HandleCount, m_waitHandles, FALSE, timeout);
-
+        const DWORD ret = WaitForMultipleObjects(HandleCount, m_waitHandles, FALSE, INFINITE);
         if (ret == WAIT_FAILED || ret - WAIT_OBJECT_0 == TerminateEventHandle) {
-            flushMessages();
+            std::lock_guard<std::mutex> guard(m_outputMutex);
+            emitReadySignal();
             break;
         }
-        if (ret == WAIT_TIMEOUT) {
-            flushMessages();
-            SetEvent(m_bufferReadyEvent);
-        } else if (ret - WAIT_OBJECT_0 == DataReadyEventHandle) {
+        if (ret - WAIT_OBJECT_0 == DataReadyEventHandle) {
             if (*processId != m_creatorPid) {
-                if (timer.elapsed() < 60) {
-                    delayedMessages[*processId].append(QString::fromLocal8Bit(message));
-                } else {
-                    delayedMessages[*processId] += QString::fromLocal8Bit(message);
-                    flushMessages();
-                }
+                std::lock_guard<std::mutex> guard(m_outputMutex);
+                m_debugOutput[*processId].push_back(QString::fromLocal8Bit(message));
+                emitReadySignal();
             }
             SetEvent(m_bufferReadyEvent);
         }
@@ -173,6 +153,49 @@ bool WinDebugInterface::runLoop()
     return true;
 }
 
+void WinDebugInterface::emitReadySignal()
+{
+    // This function must be called from the WinDebugInterface thread only.
+    QTC_ASSERT(QThread::currentThread() == this, return);
+
+    if (m_debugOutput.empty() || m_readySignalEmitted)
+        return;
+
+    m_readySignalEmitted = true;
+    emit _q_debugOutputReady();
+}
+
+void WinDebugInterface::dispatchDebugOutput()
+{
+    // Called in the thread this object was created in, not in the WinDebugInterfaceThread.
+    QTC_ASSERT(QThread::currentThread() == QCoreApplication::instance()->thread(), return);
+
+    static size_t maxMessagesToSend = 100;
+    std::vector<std::pair<qint64, QString>> output;
+    bool hasMoreOutput = false;
+
+    m_outputMutex.lock();
+    for (auto &entry : m_debugOutput) {
+        std::vector<QString> &src = entry.second;
+        QString dst;
+        size_t n = std::min(maxMessagesToSend, src.size());
+        for (size_t i = 0; i < n; ++i)
+            dst += src.at(i);
+        src.erase(src.begin(), std::next(src.begin(), n));
+        if (!src.empty())
+            hasMoreOutput = true;
+        output.emplace_back(entry.first, std::move(dst));
+    }
+    if (!hasMoreOutput)
+        m_readySignalEmitted = false;
+    m_outputMutex.unlock();
+
+    for (auto p : output)
+        emit debugOutput(p.first, p.second);
+    if (hasMoreOutput)
+        emit _q_debugOutputReady();
+}
+
 } // namespace Internal
 } // namespace ProjectExplorer
 
@@ -193,6 +216,10 @@ void WinDebugInterface::run() { }
 
 bool WinDebugInterface::runLoop() { return false; }
 
+void WinDebugInterface::emitReadySignal() { }
+
+void WinDebugInterface::dispatchDebugOutput() { }
+
 } // namespace Internal
 } // namespace ProjectExplorer
 
diff --git a/src/plugins/projectexplorer/windebuginterface.h b/src/plugins/projectexplorer/windebuginterface.h
index be3b5e3493..e459394038 100644
--- a/src/plugins/projectexplorer/windebuginterface.h
+++ b/src/plugins/projectexplorer/windebuginterface.h
@@ -25,8 +25,13 @@
 
 #pragma once
 
+#include <QMap>
 #include <QThread>
 
+#include <map>
+#include <mutex>
+#include <vector>
+
 namespace ProjectExplorer {
 namespace Internal {
 
@@ -45,12 +50,15 @@ public:
 signals:
     void debugOutput(qint64 pid, const QString &message);
     void cannotRetrieveDebugOutput();
+    void _q_debugOutputReady();
 
 private:
     enum Handles { DataReadyEventHandle, TerminateEventHandle, HandleCount };
 
     void run() override;
     bool runLoop();
+    void emitReadySignal();
+    void dispatchDebugOutput();
 
     static WinDebugInterface *m_instance;
 
@@ -59,6 +67,9 @@ private:
     Qt::HANDLE m_bufferReadyEvent = nullptr;
     Qt::HANDLE m_sharedFile = nullptr;
     void *m_sharedMem = nullptr;
+    std::mutex m_outputMutex;
+    bool m_readySignalEmitted = false;
+    std::map<qint64, std::vector<QString>> m_debugOutput;
 };
 
 } // namespace Internal