CMakeProcess: Fix stopping the CMake process

Calling QtcProcess::terminate() doesn't guarantee that
process will be terminated, especially on Windows we may
ofter wait for it forever. Replace the call to terminate()
with close(). After calling close(), the process will sooner
or later be terminated (or finally killed) - that's the job
for ProcessReaper.

Since the current code relies on receiving the finished() signal
after calling a stopping method, we need to call the expected
handler (handleProcessDone()) after calling the QtcProcess::close(),
as afterwards we can't expect receiving any signal for the possibly
running process anymore.

Refactor the code so that we connect do QtcProcess::done() signal
instead of connecting to QtcProcess::finished() signal. This
guarantees that the handler will also be called when process
failed to start. Handle this case in done() handler, too.

Rename CMakeProcess::terminate() into CMakeProcess::stop()
in order to avoid confusion on what method we have chosen to
stop the process in fact. That's the implementation detail.

Get rid of some QFuture related handlings from public API.
Use them directly from inside CMakeProcess d'tor.

Get rid of public state() method, as it seems it's unused.

Increase the responsiveness of small [x] icon of the running
cmake task (reduce the timeout of 500 ms into 50 ms).

Fixes: QTCREATORBUG-27518
Change-Id: I15cac3990079ad1cae0bbe22ac2a6e64cfb659a0
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Cristian Adam <cristian.adam@qt.io>

src/plugins/cmakeprojectmanager/cmakeprocess.cpp

@@ -32,9 +32,12 @@
 #include <projectexplorer/projectexplorerconstants.h>
 #include <projectexplorer/taskhub.h>
 
+#include <utils/processinterface.h>
 #include <utils/qtcprocess.h>
 #include <utils/stringutils.h>
 
+#include <QFutureInterface>
+
 using namespace Utils;
 
 namespace CMakeProjectManager {
@@ -52,7 +55,7 @@ static QString stripTrailingNewline(QString str)
 CMakeProcess::CMakeProcess()
 {
     connect(&m_cancelTimer, &QTimer::timeout, this, &CMakeProcess::checkForCancelled);
-    m_cancelTimer.setInterval(500);
+    m_cancelTimer.setInterval(50);
 }
 
 CMakeProcess::~CMakeProcess()
@@ -61,8 +64,9 @@ CMakeProcess::~CMakeProcess()
     m_parser.flush();
 
     if (m_future) {
-        reportCanceled();
+        // None of the progress related functions will work after this!
-        reportFinished();
+        m_future->reportCanceled();
+        m_future->reportFinished();
     }
 }
 
@@ -104,25 +108,26 @@ void CMakeProcess::run(const BuildDirParameters &parameters, const QStringList &
     // Always use the sourceDir: If we are triggered because the build directory is getting deleted
     // then we are racing against CMakeCache.txt also getting deleted.
 
-    auto process = std::make_unique<QtcProcess>();
+    m_process.reset(new QtcProcess);
     m_processWasCanceled = false;
 
     m_cancelTimer.start();
 
-    process->setWorkingDirectory(buildDirectory);
+    m_process->setWorkingDirectory(buildDirectory);
-    process->setEnvironment(parameters.environment);
+    m_process->setEnvironment(parameters.environment);
 
-    process->setStdOutLineCallback([](const QString &s) {
+    m_process->setStdOutLineCallback([](const QString &s) {
         BuildSystem::appendBuildSystemOutput(stripTrailingNewline(s));
     });
 
-    process->setStdErrLineCallback([this](const QString &s) {
+    m_process->setStdErrLineCallback([this](const QString &s) {
         m_parser.appendMessage(s, StdErrFormat);
         BuildSystem::appendBuildSystemOutput(stripTrailingNewline(s));
     });
 
-    connect(process.get(), &QtcProcess::finished,
+    connect(m_process.get(), &QtcProcess::done, this, [this] {
-            this, &CMakeProcess::handleProcessFinished);
+        handleProcessDone(m_process->resultData());
+    });
 
     CommandLine commandLine(cmakeExecutable);
     commandLine.addArgs({"-S", sourceDirectory.path(), "-B", buildDirectory.path()});
@@ -133,72 +138,44 @@ void CMakeProcess::run(const BuildDirParameters &parameters, const QStringList &
     BuildSystem::startNewBuildSystemOutput(
         tr("Running %1 in %2.").arg(commandLine.toUserOutput()).arg(buildDirectory.toUserOutput()));
 
-    auto future = std::make_unique<QFutureInterface<void>>();
+    m_future.reset(new QFutureInterface<void>);
-    future->setProgressRange(0, 1);
+    m_future->setProgressRange(0, 1);
-    Core::ProgressManager::addTimedTask(*future.get(),
+    Core::ProgressManager::addTimedTask(*m_future.get(),
                                         tr("Configuring \"%1\"").arg(parameters.projectName),
                                         "CMake.Configure",
                                         10);
 
-    process->setUseCtrlCStub(true);
+    m_process->setCommand(commandLine);
-    process->setCommand(commandLine);
     emit started();
     m_elapsed.start();
-    process->start();
+    m_process->start();
-
-    m_process = std::move(process);
-    m_future = std::move(future);
 }
 
-void CMakeProcess::terminate()
+void CMakeProcess::stop()
 {
     if (m_process) {
         m_processWasCanceled = true;
-        m_process->terminate();
+        m_process->close();
+        handleProcessDone({15, QProcess::CrashExit, QProcess::Crashed, {}});
     }
 }
 
-QProcess::ProcessState CMakeProcess::state() const
+void CMakeProcess::handleProcessDone(const Utils::ProcessResultData &resultData)
-{
-    if (m_process)
-        return m_process->state();
-    return QProcess::NotRunning;
-}
-
-void CMakeProcess::reportCanceled()
 {
     QTC_ASSERT(m_future, return);
-    m_future->reportCanceled();
-}
-
-void CMakeProcess::reportFinished()
-{
-    QTC_ASSERT(m_future, return);
-    m_future->reportFinished();
-    m_future.reset();
-}
-
-void CMakeProcess::setProgressValue(int p)
-{
-    QTC_ASSERT(m_future, return);
-    m_future->setProgressValue(p);
-}
-
-void CMakeProcess::handleProcessFinished()
-{
-    QTC_ASSERT(m_process && m_future, return);
 
     m_cancelTimer.stop();
 
-    const int code = m_process->exitCode();
+    const int code = resultData.m_exitCode;
 
     QString msg;
-    if (m_process->exitStatus() != QProcess::NormalExit) {
+    if (resultData.m_error == QProcess::FailedToStart) {
-        if (m_processWasCanceled) {
+        msg = tr("CMake process failed to start.");
+    } else if (resultData.m_exitStatus != QProcess::NormalExit) {
+        if (m_processWasCanceled)
             msg = tr("CMake process was canceled by the user.");
-        } else {
+         else
             msg = tr("CMake process crashed.");
-        }
     } else if (code != 0) {
         msg = tr("CMake process exited with exit code %1.").arg(code);
     }
@@ -227,8 +204,7 @@ void CMakeProcess::checkForCancelled()
 
     if (m_future->isCanceled()) {
         m_cancelTimer.stop();
-        m_processWasCanceled = true;
+        stop();
-        m_process->close();
     }
 }
 

src/plugins/cmakeprojectmanager/cmakeprocess.h

@@ -30,15 +30,21 @@
 #include <utils/outputformatter.h>
 
 #include <QElapsedTimer>
-#include <QFutureInterface>
 #include <QObject>
-#include <QProcess>
 #include <QStringList>
 #include <QTimer>
 
 #include <memory>
 
-namespace Utils { class QtcProcess; }
+QT_BEGIN_NAMESPACE
+template<class T>
+class QFutureInterface;
+QT_END_NAMESPACE
+
+namespace Utils {
+class ProcessResultData;
+class QtcProcess;
+}
 
 namespace CMakeProjectManager {
 namespace Internal {
@@ -49,18 +55,10 @@ class CMakeProcess : public QObject
 
 public:
     CMakeProcess();
-    CMakeProcess(const CMakeProcess&) = delete;
     ~CMakeProcess();
 
     void run(const BuildDirParameters &parameters, const QStringList &arguments);
-    void terminate();
+    void stop();
-
-    QProcess::ProcessState state() const;
-
-    // Update progress information:
-    void reportCanceled();
-    void reportFinished(); // None of the progress related functions will work after this!
-    void setProgressValue(int p);
 
     int lastExitCode() const { return m_lastExitCode; }
 
@@ -69,7 +67,7 @@ signals:
     void finished();
 
 private:
-    void handleProcessFinished();
+    void handleProcessDone(const Utils::ProcessResultData &resultData);
     void checkForCancelled();
 
     std::unique_ptr<Utils::QtcProcess> m_process;

src/plugins/cmakeprojectmanager/fileapireader.cpp

@@ -175,7 +175,7 @@ void FileApiReader::stop()
 void FileApiReader::stopCMakeRun()
 {
     if (m_cmakeProcess)
-        m_cmakeProcess->terminate();
+        m_cmakeProcess->stop();
 }
 
 bool FileApiReader::isParsing() const