Process: Refactor timeout handling

Rename ProcessResult::Hang into Canceled.

Change the behavior of the Process:
Whenever the terminate() or kill() is called, including
indirect calls through stop() or runBlocking()'s timeout,
mark the process result as Canceled.
In this way the Process running by a call to start()
may also finish with Canceled state. Before it was only
happening for processes started via runBlocking().

Adapt the runBlockingStdOut_data() test accordingly.

Get rid of ProcessInterface::m_canceledByUser field.
Use ProcessResult::Canceled state instead.

Fix existing 3 usages of m_canceledByUser field.
Use standarized exitMessage() method for them.

Add automatic measurement of process execution.
Introduce processDuration() getter.
Use it for reporting exitMessage() instead of timeoutS().

Change-Id: I1a68559ce844caef863a97a6b0577b0238011f70
Reviewed-by: Orgad Shaneh <orgads@gmail.com>

src/libs/utils/process.cpp

@@ -45,6 +45,8 @@
 
 using namespace Utils::Internal;
 
+using namespace std::chrono;
+
 namespace Utils {
 namespace Internal {
 
@@ -850,6 +852,8 @@ public:
     ChannelBuffer m_stdOut;
     ChannelBuffer m_stdErr;
 
+    time_point<system_clock, nanoseconds> m_startTimestamp = {};
+    time_point<system_clock, nanoseconds> m_doneTimestamp = {};
     int m_timeoutInSeconds = 10;
     bool m_timeOutMessageBoxEnabled = false;
 
@@ -1087,8 +1091,10 @@ void ProcessPrivate::sendControlSignal(ControlSignal controlSignal)
     if (!m_process || (m_state == QProcess::NotRunning))
         return;
 
-    if (controlSignal == ControlSignal::Terminate || controlSignal == ControlSignal::Kill)
+    if (controlSignal == ControlSignal::Terminate || controlSignal == ControlSignal::Kill) {
-        m_resultData.m_canceledByUser = true;
+        m_doneTimestamp = system_clock::now();
+        m_result = ProcessResult::Canceled;
+    }
 
     QMetaObject::invokeMethod(m_process.get(), [this, controlSignal] {
         m_process->sendControlSignal(controlSignal);
@@ -1102,6 +1108,8 @@ void ProcessPrivate::clearForRun()
     m_stdErr.clearForRun();
     m_stdErr.codec = m_codec;
     m_result = ProcessResult::StartFailed;
+    m_startTimestamp = {};
+    m_doneTimestamp = {};
 
     m_killTimer.stop();
     m_state = QProcess::NotRunning;
@@ -1272,6 +1280,7 @@ void Process::start()
     d->setProcessInterface(processImpl);
     d->m_state = QProcess::Starting;
     d->m_process->m_setup = d->m_setup;
+    d->m_startTimestamp = system_clock::now();
     d->emitGuardedSignal(&Process::starting);
     d->m_process->start();
 }
@@ -1652,8 +1661,8 @@ QString Process::exitMessage(const CommandLine &command, ProcessResult result, i
         return Tr::tr("The command \"%1\" terminated abnormally.").arg(cmd);
     case ProcessResult::StartFailed:
         return Tr::tr("The command \"%1\" could not be started.").arg(cmd);
-    case ProcessResult::Hang:
+    case ProcessResult::Canceled:
-        return Tr::tr("The command \"%1\" did not respond within the timeout limit (%2 s).")
+        return Tr::tr("The command \"%1\" was canceled after (%2 s).")
             .arg(cmd).arg(timeoutInSeconds);
     }
     return {};
@@ -1661,7 +1670,17 @@ QString Process::exitMessage(const CommandLine &command, ProcessResult result, i
 
 QString Process::exitMessage() const
 {
-    return exitMessage(commandLine(), result(), exitCode(), d->m_timeoutInSeconds);
+    return exitMessage(commandLine(), result(), exitCode(),
+                       duration_cast<seconds>(processDuration()).count());
+}
+
+milliseconds Process::processDuration() const
+{
+    if (d->m_startTimestamp == time_point<system_clock, nanoseconds>())
+        return {};
+    const auto end = (d->m_doneTimestamp == time_point<system_clock, nanoseconds>())
+                         ? system_clock::now() : d->m_doneTimestamp;
+    return duration_cast<milliseconds>(end - d->m_startTimestamp);
 }
 
 QByteArray Process::allRawOutput() const
@@ -1874,7 +1893,6 @@ void Process::runBlocking(EventLoopMode eventLoopMode)
             return;
         stop();
         QTC_CHECK(waitForFinished(2000));
-        d->m_result = ProcessResult::Hang;
     };
 
     if (eventLoopMode == EventLoopMode::On) {
@@ -2028,10 +2046,10 @@ void ProcessPrivate::handleReadyRead(const QByteArray &outputData, const QByteAr
 
 void ProcessPrivate::handleDone(const ProcessResultData &data)
 {
+    if (m_result != ProcessResult::Canceled)
+        m_doneTimestamp = system_clock::now();
     m_killTimer.stop();
-    const bool wasCanceled = m_resultData.m_canceledByUser;
     m_resultData = data;
-    m_resultData.m_canceledByUser = wasCanceled;
 
     switch (m_state) {
     case QProcess::NotRunning:
@@ -2053,19 +2071,15 @@ void ProcessPrivate::handleDone(const ProcessResultData &data)
     // HACK: See QIODevice::errorString() implementation.
     if (m_resultData.m_error == QProcess::UnknownError)
         m_resultData.m_errorString.clear();
-    else if (m_result != ProcessResult::Hang)
-        m_result = ProcessResult::StartFailed;
 
-    if (m_resultData.m_error != QProcess::FailedToStart) {
+    if (m_result != ProcessResult::Canceled && m_resultData.m_error != QProcess::FailedToStart) {
         switch (m_resultData.m_exitStatus) {
         case QProcess::NormalExit:
             m_result = m_resultData.m_exitCode ? ProcessResult::FinishedWithError
                                                : ProcessResult::FinishedWithSuccess;
             break;
         case QProcess::CrashExit:
-            // Was hang detected before and killed?
+            m_result = ProcessResult::TerminatedAbnormally;
-            if (m_result != ProcessResult::Hang)
-                m_result = ProcessResult::TerminatedAbnormally;
             break;
         }
     }
@@ -2093,7 +2107,6 @@ void ProcessPrivate::setupDebugLog()
         return;
 
     auto now = [] {
-        using namespace std::chrono;
         return duration_cast<milliseconds>(system_clock::now().time_since_epoch()).count();
     };
 

src/libs/utils/process.h

@@ -184,6 +184,7 @@ public:
     static QString exitMessage(const CommandLine &command, ProcessResult result, int exitCode,
                                int timeoutInSeconds);
     QString exitMessage() const;
+    std::chrono::milliseconds processDuration() const;
 
     QString toStandaloneCommandLine() const;
 

src/libs/utils/processenums.h

@@ -58,12 +58,13 @@ enum class ProcessResult {
     // Finished unsuccessfully. Unless an ExitCodeInterpreter is set
     // this corresponds to a return code different from 0.
     FinishedWithError,
-    // Process terminated abnormally (kill)
+    // Process terminated abnormally (crash)
     TerminatedAbnormally,
     // Executable could not be started
     StartFailed,
-    // Hang, no output after time out
+    // Canceled due to a call to terminate() or kill(),
-    Hang
+    // This includes a call to stop() or timeout has triggered for runBlocking().
+    Canceled
 };
 
 using TextChannelCallback = std::function<void(const QString & /*text*/)>;

src/libs/utils/processinterface.h

@@ -102,7 +102,6 @@ public:
     QProcess::ExitStatus m_exitStatus = QProcess::NormalExit;
     QProcess::ProcessError m_error = QProcess::UnknownError;
     QString m_errorString;
-    bool m_canceledByUser = false;
 };
 
 enum class ControlSignal {

src/plugins/cmakeprojectmanager/cmakeprocess.cpp

@@ -131,7 +131,16 @@ void CMakeProcess::run(const BuildDirParameters &parameters, const QStringList &
     });
 
     connect(m_process.get(), &Process::done, this, [this] {
-        handleProcessDone(m_process->resultData());
+        if (m_process->result() != ProcessResult::FinishedWithSuccess) {
+            const QString message = m_process->exitMessage();
+            BuildSystem::appendBuildSystemOutput(addCMakePrefix({{}, message}).join('\n'));
+            TaskHub::addTask(BuildSystemTask(Task::Error, message));
+        }
+
+        emit finished(m_process->exitCode());
+
+        const QString elapsedTime = Utils::formatElapsedTime(m_elapsed.elapsed());
+        BuildSystem::appendBuildSystemOutput(addCMakePrefix({{}, elapsedTime}).join('\n'));
     });
 
     CommandLine commandLine(cmakeExecutable);
@@ -158,32 +167,6 @@ void CMakeProcess::stop()
         m_process->stop();
 }
 
-void CMakeProcess::handleProcessDone(const Utils::ProcessResultData &resultData)
-{
-    const int code = resultData.m_exitCode;
-    QString msg;
-    if (resultData.m_error == QProcess::FailedToStart) {
-        msg = ::CMakeProjectManager::Tr::tr("CMake process failed to start.");
-    } else if (resultData.m_exitStatus != QProcess::NormalExit) {
-        if (resultData.m_canceledByUser)
-            msg = ::CMakeProjectManager::Tr::tr("CMake process was canceled by the user.");
-        else
-            msg = ::CMakeProjectManager::Tr::tr("CMake process crashed.");
-    } else if (code != 0) {
-        msg = ::CMakeProjectManager::Tr::tr("CMake process exited with exit code %1.").arg(code);
-    }
-
-    if (!msg.isEmpty()) {
-        BuildSystem::appendBuildSystemOutput(addCMakePrefix({QString(), msg}).join('\n'));
-        TaskHub::addTask(BuildSystemTask(Task::Error, msg));
-    }
-
-    emit finished(code);
-
-    const QString elapsedTime = Utils::formatElapsedTime(m_elapsed.elapsed());
-    BuildSystem::appendBuildSystemOutput(addCMakePrefix({QString(), elapsedTime}).join('\n'));
-}
-
 QString addCMakePrefix(const QString &str)
 {
     auto qColorToAnsiCode = [] (const QColor &color) {

src/plugins/cmakeprojectmanager/cmakeprocess.h

@@ -36,8 +36,6 @@ signals:
     void stdOutReady(const QString &s);
 
 private:
-    void handleProcessDone(const Utils::ProcessResultData &resultData);
-
     std::unique_ptr<Utils::Process> m_process;
     Utils::OutputFormatter m_parser;
     QElapsedTimer m_elapsed;

src/plugins/git/gerrit/gerritmodel.cpp

@@ -225,8 +225,6 @@ private:
     void processDone();
     void timeout();
 
-    void errorTermination(const QString &msg);
-
     Process m_process;
     QTimer m_timer;
     FilePath m_binary;
@@ -293,12 +291,6 @@ void QueryContext::start()
     m_process.start();
 }
 
-void QueryContext::errorTermination(const QString &msg)
-{
-    if (!m_process.resultData().m_canceledByUser)
-        VcsOutputWindow::appendError(msg);
-}
-
 void QueryContext::terminate()
 {
     m_process.stop();
@@ -313,14 +305,10 @@ void QueryContext::processDone()
     if (!m_error.isEmpty())
         emit errorText(m_error);
 
-    if (m_process.exitStatus() == QProcess::CrashExit)
+    if (m_process.result() == ProcessResult::FinishedWithSuccess)
-        errorTermination(Git::Tr::tr("%1 crashed.").arg(m_binary.toUserOutput()));
-    else if (m_process.exitCode())
-        errorTermination(Git::Tr::tr("%1 returned %2.").arg(m_binary.toUserOutput()).arg(m_process.exitCode()));
-    else if (m_process.result() != ProcessResult::FinishedWithSuccess)
-        errorTermination(Git::Tr::tr("Error running %1: %2").arg(m_binary.toUserOutput(), m_process.errorString()));
-    else
         emit resultRetrieved(m_output);
+    else if (m_process.result() != ProcessResult::Canceled)
+        VcsOutputWindow::appendError(m_process.exitMessage());
 
     emit finished();
 }

src/plugins/git/gerrit/gerritplugin.cpp

@@ -119,7 +119,7 @@ void FetchContext::processDone()
     deleteLater();
 
     if (m_process.result() != ProcessResult::FinishedWithSuccess) {
-        if (!m_process.resultData().m_canceledByUser)
+        if (m_process.result() != ProcessResult::Canceled)
             VcsBase::VcsOutputWindow::appendError(m_process.exitMessage());
         return;
     }

tests/auto/utils/process/tst_process.cpp

@@ -946,13 +946,11 @@ void tst_Process::runBlockingStdOut_data()
     QTest::addColumn<int>("timeOutS");
     QTest::addColumn<ProcessResult>("expectedResult");
 
-    // TerminatedAbnormally, since it didn't time out and callback stopped the process forcefully.
+    // Canceled, since the process is killed (canceled) from the callback.
-    QTest::newRow("Short timeout with end of line")
+    QTest::newRow("Short timeout with end of line") << true << 2 << ProcessResult::Canceled;
-            << true << 2 << ProcessResult::TerminatedAbnormally;
 
-    // Hang, since it times out, calls the callback handler and stops the process forcefully.
+    // Canceled, since it times out.
-    QTest::newRow("Short timeout without end of line")
+    QTest::newRow("Short timeout without end of line") << false << 2 << ProcessResult::Canceled;
-            << false << 2 << ProcessResult::Hang;
 
     // FinishedWithSuccess, since it doesn't time out, it finishes process normally,
     // calls the callback handler and tries to stop the process forcefully what is no-op