From f6be85b1d2762951dffa77d5b7f03199eb795f35 Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Mon, 22 Jan 2024 12:49:58 +0100 Subject: [PATCH] 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 --- src/libs/utils/process.cpp | 43 ++++++++++++------- src/libs/utils/process.h | 1 + src/libs/utils/processenums.h | 7 +-- src/libs/utils/processinterface.h | 1 - .../cmakeprojectmanager/cmakeprocess.cpp | 37 +++++----------- .../cmakeprojectmanager/cmakeprocess.h | 2 - src/plugins/git/gerrit/gerritmodel.cpp | 18 ++------ src/plugins/git/gerrit/gerritplugin.cpp | 2 +- tests/auto/utils/process/tst_process.cpp | 10 ++--- 9 files changed, 51 insertions(+), 70 deletions(-) diff --git a/src/libs/utils/process.cpp b/src/libs/utils/process.cpp index 493493605a0..c6236ec5df2 100644 --- a/src/libs/utils/process.cpp +++ b/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 m_startTimestamp = {}; + time_point 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) - m_resultData.m_canceledByUser = true; + if (controlSignal == ControlSignal::Terminate || controlSignal == ControlSignal::Kill) { + 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: - return Tr::tr("The command \"%1\" did not respond within the timeout limit (%2 s).") + case ProcessResult::Canceled: + 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(processDuration()).count()); +} + +milliseconds Process::processDuration() const +{ + if (d->m_startTimestamp == time_point()) + return {}; + const auto end = (d->m_doneTimestamp == time_point()) + ? system_clock::now() : d->m_doneTimestamp; + return duration_cast(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? - if (m_result != ProcessResult::Hang) - m_result = ProcessResult::TerminatedAbnormally; + m_result = ProcessResult::TerminatedAbnormally; break; } } @@ -2093,7 +2107,6 @@ void ProcessPrivate::setupDebugLog() return; auto now = [] { - using namespace std::chrono; return duration_cast(system_clock::now().time_since_epoch()).count(); }; diff --git a/src/libs/utils/process.h b/src/libs/utils/process.h index 3c1f5011978..d219cee10e3 100644 --- a/src/libs/utils/process.h +++ b/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; diff --git a/src/libs/utils/processenums.h b/src/libs/utils/processenums.h index a57817123f8..903bc8df9bf 100644 --- a/src/libs/utils/processenums.h +++ b/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 - Hang + // Canceled due to a call to terminate() or kill(), + // This includes a call to stop() or timeout has triggered for runBlocking(). + Canceled }; using TextChannelCallback = std::function; diff --git a/src/libs/utils/processinterface.h b/src/libs/utils/processinterface.h index 796972d3093..37d47db5de2 100644 --- a/src/libs/utils/processinterface.h +++ b/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 { diff --git a/src/plugins/cmakeprojectmanager/cmakeprocess.cpp b/src/plugins/cmakeprojectmanager/cmakeprocess.cpp index 1762684a744..0542e5b71c4 100644 --- a/src/plugins/cmakeprojectmanager/cmakeprocess.cpp +++ b/src/plugins/cmakeprojectmanager/cmakeprocess.cpp @@ -131,7 +131,16 @@ void CMakeProcess::run(const BuildDirParameters ¶meters, 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) { diff --git a/src/plugins/cmakeprojectmanager/cmakeprocess.h b/src/plugins/cmakeprojectmanager/cmakeprocess.h index c46a4ba84c9..293170cc336 100644 --- a/src/plugins/cmakeprojectmanager/cmakeprocess.h +++ b/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 m_process; Utils::OutputFormatter m_parser; QElapsedTimer m_elapsed; diff --git a/src/plugins/git/gerrit/gerritmodel.cpp b/src/plugins/git/gerrit/gerritmodel.cpp index a4f31f3393d..231933c9cb4 100644 --- a/src/plugins/git/gerrit/gerritmodel.cpp +++ b/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) - 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 + if (m_process.result() == ProcessResult::FinishedWithSuccess) emit resultRetrieved(m_output); + else if (m_process.result() != ProcessResult::Canceled) + VcsOutputWindow::appendError(m_process.exitMessage()); emit finished(); } diff --git a/src/plugins/git/gerrit/gerritplugin.cpp b/src/plugins/git/gerrit/gerritplugin.cpp index 9355d77c9c5..69025b11e91 100644 --- a/src/plugins/git/gerrit/gerritplugin.cpp +++ b/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; } diff --git a/tests/auto/utils/process/tst_process.cpp b/tests/auto/utils/process/tst_process.cpp index 78df7eb4d51..624a15ad351 100644 --- a/tests/auto/utils/process/tst_process.cpp +++ b/tests/auto/utils/process/tst_process.cpp @@ -946,13 +946,11 @@ void tst_Process::runBlockingStdOut_data() QTest::addColumn("timeOutS"); QTest::addColumn("expectedResult"); - // TerminatedAbnormally, since it didn't time out and callback stopped the process forcefully. - QTest::newRow("Short timeout with end of line") - << true << 2 << ProcessResult::TerminatedAbnormally; + // Canceled, since the process is killed (canceled) from the callback. + QTest::newRow("Short timeout with end of line") << true << 2 << ProcessResult::Canceled; - // Hang, since it times out, calls the callback handler and stops the process forcefully. - QTest::newRow("Short timeout without end of line") - << false << 2 << ProcessResult::Hang; + // Canceled, since it times out. + QTest::newRow("Short timeout without end of line") << false << 2 << ProcessResult::Canceled; // 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