ProcessReaper: Move reaping into a separate thread

When reaping a process: do one call to terminate() and after
500 ms timeout (default) do one call to kill() if the process is
still running. When the process finishes, end reaping by
deleting the reaper and its process. Ensure that the state of
the process is NotRunning before deleting it.

Eliminate the need to process events in ProcessReaper's destructor.

This change removes the emergency counter, which could caused
issues when the process was still running and the emergency
conter was above 5. Before, we were forcefully deleting the
running process. Now we wait as long as it's needed for process
to finish.

Since the reaper was operating in the main thread before,
it could happen that ProcessReaper inside the process launcher
could have blocked the launcher's main thread considerably, when
emergency counter exceeded the value of 5 and the destructor
of QProcess was run in turn. In this case the process launcher couldn't
operate and couldn't handle other running processes. This could cause
that corresponding running QtcProcesses on Creator side
timed out. Moving the reaper to the separate thread eliminates
this issue.

Change-Id: Id78953a2aec5cb08dc869621386b6a61a182e81c
Reviewed-by: hjk <hjk@qt.io>

src/libs/utils/processreaper.cpp

@@ -27,149 +27,234 @@
 #include "qtcassert.h"
 
 #include <QCoreApplication>
+#include <QElapsedTimer>
+#include <QHash>
 #include <QProcess>
-#include <QThread>
 #include <QTimer>
-
+#include <QWaitCondition>
-#ifdef Q_OS_WIN
-#ifdef QTCREATOR_PCH_H
-#define CALLBACK WINAPI
-#endif
-#include <qt_windows.h>
-#endif
 
 using namespace Utils;
 
 namespace Utils {
 namespace Internal {
 
-class Reaper final : public QObject
+/*
+
+Observations on how QProcess::terminate() behaves on different platforms when called for
+never ending running process:
+
+1. Windows:
+
+   The issue in QTCREATORBUG-27118 with adb.exe is most probably a special
+   Windows only case described in docs of QProcess::terminate():
+
+   "Console applications on Windows that do not run an event loop, or whose event loop does
+   not handle the WM_CLOSE message, can only be terminated by calling kill()."
+
+   It looks like when you call terminate() for the adb.exe, it won't stop, never, even after
+   default 30 seconds timeout. The same happens for blocking processes tested in
+   tst_QtcProcess::killBlockingProcess(). It's hard to say whether any process on Windows can
+   be finished by a call to terminate(). Until now, no such a process has been found.
+
+   Further call to kill() (after a call to terminate()) finishes the process quickly.
+
+2. Linux:
+
+   It looks like a call to terminate() finishes the running process after a long wait
+   (between 10-15 seconds). After calling terminate(), further calls to kill() doesn't
+   make the process to finish soon (are no-op). On the other hand, when we start reaping the
+   running process from a call to kill() without a prior call to terminate(), the process
+   finishes quickly.
+
+3. Mac:
+
+   It looks like the process finishes quickly after a call to terminate().
+
+*/
+
+static const int s_timeoutThreshold = 10000; // 10 seconds
+
+static QString execWithArguments(QProcess *process)
 {
-public:
+    QStringList commandLine;
-    Reaper(QProcess *p, int timeoutMs);
+    commandLine.append(process->program());
-    ~Reaper() final;
+    commandLine.append(process->arguments());
+    return commandLine.join(QChar::Space);
+}
 
-    int timeoutMs() const;
+struct ReaperSetup
-    bool isFinished() const;
+{
-    void nextIteration();
-
-private:
-    mutable QTimer m_iterationTimer;
     QProcess *m_process = nullptr;
-    int m_emergencyCounter = 0;
+    int m_timeoutMs;
-    QProcess::ProcessState m_lastState = QProcess::NotRunning;
 };
 
-static QList<Reaper *> g_reapers;
+class Reaper : public QObject
-
-Reaper::Reaper(QProcess *p, int timeoutMs) : m_process(p)
 {
-    g_reapers.append(this);
+    Q_OBJECT
 
-    m_iterationTimer.setInterval(timeoutMs);
+public:
-    m_iterationTimer.setSingleShot(true);
+    Reaper(const ReaperSetup &reaperSetup) : m_reaperSetup(reaperSetup) {}
-    connect(&m_iterationTimer, &QTimer::timeout, this, &Reaper::nextIteration);
 
-    QMetaObject::invokeMethod(this, &Reaper::nextIteration, Qt::QueuedConnection);
+    void reap()
-}
+    {
+        m_timer.start();
 
-Reaper::~Reaper()
+        connect(m_reaperSetup.m_process, &QProcess::finished,
-{
+                this, &Reaper::handleFinished);
-    g_reapers.removeOne(this);
-}
 
-int Reaper::timeoutMs() const
+        if (emitFinished())
-{
-    const int remaining = m_iterationTimer.remainingTime();
-    if (remaining < 0)
-        return m_iterationTimer.interval();
-    m_iterationTimer.stop();
-    return remaining;
-}
-
-bool Reaper::isFinished() const
-{
-    return !m_process;
-}
-
-#ifdef Q_OS_WIN
-static BOOL sendMessage(UINT message, HWND hwnd, LPARAM lParam)
-{
-    DWORD dwProcessID;
-    GetWindowThreadProcessId(hwnd, &dwProcessID);
-    if ((DWORD)lParam == dwProcessID) {
-        SendNotifyMessage(hwnd, message, 0, 0);
-        return FALSE;
-    }
-    return TRUE;
-}
-
-BOOL CALLBACK sendShutDownMessageToAllWindowsOfProcess_enumWnd(HWND hwnd, LPARAM lParam)
-{
-    static UINT uiShutDownMessage = RegisterWindowMessage(L"qtcctrlcstub_shutdown");
-    return sendMessage(uiShutDownMessage, hwnd, lParam);
-}
-
-#endif
-
-void Reaper::nextIteration()
-{
-    QProcess::ProcessState state = m_process ? m_process->state() : QProcess::NotRunning;
-    if (state == QProcess::NotRunning || m_emergencyCounter > 5) {
-        delete m_process;
-        m_process = nullptr;
             return;
+
+        terminate();
     }
 
-    if (state == QProcess::Starting) {
+signals:
-        if (m_lastState == QProcess::Starting)
+    void finished();
-            m_process->kill();
+
-    } else if (state == QProcess::Running) {
+private:
-        if (m_lastState == QProcess::Running) {
+    void terminate()
-            m_process->kill();
+    {
-        } else if (m_process->program().endsWith(QLatin1String("qtcreator_ctrlc_stub.exe"))) {
+        // TODO: do a custom terminate here for ctrlCStub
-#ifdef Q_OS_WIN
+        m_reaperSetup.m_process->terminate();
-            EnumWindows(sendShutDownMessageToAllWindowsOfProcess_enumWnd, m_process->processId());
+        QTimer::singleShot(m_reaperSetup.m_timeoutMs, this, &Reaper::handleTerminateTimeout);
-#endif
+    }
-        } else {
+
-            m_process->terminate();
+    void kill() { m_reaperSetup.m_process->kill(); }
+
+    bool emitFinished()
+    {
+        if (m_reaperSetup.m_process->state() != QProcess::NotRunning)
+            return false;
+
+        if (!m_finished) {
+            const int timeout = m_timer.elapsed();
+            if (timeout > s_timeoutThreshold) {
+                qWarning() << "Finished parallel reaping of" << execWithArguments(m_reaperSetup.m_process)
+                           << "in" << (timeout / 1000.0) << "seconds.";
+            }
+
+            m_finished = true;
+            emit finished();
+        }
+        return true;
+    }
+
+    void handleFinished()
+    {
+        // In case the process is still running - wait until it has finished
+        QTC_ASSERT(emitFinished(), QTimer::singleShot(m_reaperSetup.m_timeoutMs,
+                                                      this, &Reaper::handleFinished));
+    }
+
+    void handleTerminateTimeout()
+    {
+        if (emitFinished())
+            return;
+
+        kill();
+    }
+
+    bool m_finished = false;
+    QElapsedTimer m_timer;
+    const ReaperSetup m_reaperSetup;
+};
+
+class ProcessReaperPrivate : public QObject
+{
+    Q_OBJECT
+
+public:
+    // Called from non-reaper's thread
+    void scheduleReap(const ReaperSetup &reaperSetup)
+    {
+        QTC_CHECK(QThread::currentThread() != thread());
+        QMutexLocker locker(&m_mutex);
+        m_reaperSetupList.append(reaperSetup);
+        QMetaObject::invokeMethod(this, &ProcessReaperPrivate::flush);
+    }
+
+    // Called from non-reaper's thread
+    void waitForFinished()
+    {
+        QTC_CHECK(QThread::currentThread() != thread());
+        QMetaObject::invokeMethod(this, &ProcessReaperPrivate::flush,
+                                  Qt::BlockingQueuedConnection);
+        QMutexLocker locker(&m_mutex);
+        if (m_reaperList.isEmpty())
+            return;
+
+        m_waitCondition.wait(&m_mutex);
+    }
+
+private:
+    // All the private methods are called from the reaper's thread
+    QList<ReaperSetup> takeReaperSetupList()
+    {
+        QMutexLocker locker(&m_mutex);
+        const QList<ReaperSetup> reaperSetupList = m_reaperSetupList;
+        m_reaperSetupList.clear();
+        return reaperSetupList;
+    }
+
+    void flush()
+    {
+        while (true) {
+            const QList<ReaperSetup> reaperSetupList = takeReaperSetupList();
+            if (reaperSetupList.isEmpty())
+                return;
+            for (const ReaperSetup &reaperSetup : reaperSetupList)
+                reap(reaperSetup);
         }
     }
 
-    m_lastState = state;
+    void reap(const ReaperSetup &reaperSetup)
-    m_iterationTimer.start();
+    {
+        Reaper *reaper = new Reaper(reaperSetup);
+        connect(reaper, &Reaper::finished, this, [this, reaper, process = reaperSetup.m_process] {
+            QMutexLocker locker(&m_mutex);
+            QTC_CHECK(m_reaperList.removeOne(reaper));
+            delete reaper;
+            delete process;
+            if (m_reaperList.isEmpty())
+                m_waitCondition.wakeOne();
+        }, Qt::QueuedConnection);
 
-    ++m_emergencyCounter;
+        {
-}
+            QMutexLocker locker(&m_mutex);
+            m_reaperList.append(reaper);
+        }
+
+        reaper->reap();
+    }
+
+    QMutex m_mutex;
+    QWaitCondition m_waitCondition;
+    QList<ReaperSetup> m_reaperSetupList;
+    QList<Reaper *> m_reaperList;
+};
 
 } // namespace Internal
 
+using namespace Utils::Internal;
+
+static QMutex s_instanceMutex;
+
+ProcessReaper::ProcessReaper()
+    : m_private(new ProcessReaperPrivate())
+{
+    m_private->moveToThread(&m_thread);
+    QObject::connect(&m_thread, &QThread::finished, m_private, &QObject::deleteLater);
+    m_thread.start();
+    m_thread.moveToThread(qApp->thread());
+}
+
 ProcessReaper::~ProcessReaper()
 {
-    while (!Internal::g_reapers.isEmpty()) {
+    QTC_CHECK(QThread::currentThread() == qApp->thread());
-        int alreadyWaited = 0;
+    QMutexLocker locker(&s_instanceMutex);
-        QList<Internal::Reaper *> toDelete;
+    instance()->m_private->waitForFinished();
-
+    m_thread.quit();
-        // push reapers along:
+    m_thread.wait();
-        for (Internal::Reaper *pr : qAsConst(Internal::g_reapers)) {
-            const int timeoutMs = pr->timeoutMs();
-            if (alreadyWaited < timeoutMs) {
-                const unsigned long toSleep = static_cast<unsigned long>(timeoutMs - alreadyWaited);
-                QThread::msleep(toSleep);
-                QCoreApplication::processEvents(QEventLoop::ExcludeUserInputEvents);
-                alreadyWaited += toSleep;
-            }
-
-            pr->nextIteration();
-
-            if (pr->isFinished())
-                toDelete.append(pr);
-        }
-
-        // Clean out reapers that finished in the meantime
-        qDeleteAll(toDelete);
-        toDelete.clear();
-    }
 }
 
 void ProcessReaper::reap(QProcess *process, int timeoutMs)
@@ -177,27 +262,26 @@ void ProcessReaper::reap(QProcess *process, int timeoutMs)
     if (!process)
         return;
 
-    QTC_ASSERT(QThread::currentThread() == process->thread(), return );
+    QTC_ASSERT(QThread::currentThread() == process->thread(), return);
 
     process->disconnect();
     if (process->state() == QProcess::NotRunning) {
         process->deleteLater();
         return;
     }
+
     // Neither can move object with a parent into a different thread
     // nor reaping the process with a parent makes any sense.
     process->setParent(nullptr);
-    if (process->thread() != QCoreApplication::instance()->thread()) {
-        process->moveToThread(QCoreApplication::instance()->thread());
-        QMetaObject::invokeMethod(process, [process, timeoutMs] {
-            reap(process, timeoutMs);
-        }); // will be queued
-        return;
-    }
 
-    ProcessReaper::instance();
+    QMutexLocker locker(&s_instanceMutex);
-    new Internal::Reaper(process, timeoutMs);
+    ProcessReaperPrivate *priv = instance()->m_private;
+
+    process->moveToThread(priv->thread());
+    ReaperSetup reaperSetup {process, timeoutMs};
+    priv->scheduleReap(reaperSetup);
 }
 
 } // namespace Utils
 
+#include "processreaper.moc"

src/libs/utils/processreaper.h

@@ -29,11 +29,14 @@
 
 #include "singleton.h"
 
+#include <QThread>
+
 QT_BEGIN_NAMESPACE
 class QProcess;
 QT_END_NAMESPACE
 
 namespace Utils {
+namespace Internal { class ProcessReaperPrivate; }
 
 class QTCREATOR_UTILS_EXPORT ProcessReaper final
         : public SingletonWithOptionalDependencies<ProcessReaper>
@@ -42,8 +45,11 @@ public:
     static void reap(QProcess *process, int timeoutMs = 500);
 
 private:
-    ProcessReaper() = default;
+    ProcessReaper();
     ~ProcessReaper();
+
+    QThread m_thread;
+    Internal::ProcessReaperPrivate *m_private;
     friend class SingletonWithOptionalDependencies<ProcessReaper>;
 };