From 00acccfd3d668e2936677c7ebb9914e363c700c8 Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Fri, 1 Apr 2022 15:14:05 +0200 Subject: [PATCH] QtcProcess: Fix terminate() for process launcher ProcessLauncherImpl::terminate() was behaving the same as ProcessLauncherImpl::kill() in case of process launcher implementation - in both cases the launcher immediately returned confirmation about receiving the close request and was putting the corresponding process into its reaper. However, the issue with this approach is that we can't receive anymore any potential ready read signal from the process being terminated after a call to terminate(). The fix is to diverge the behavior of terminate() and kill() of ProcessLauncherImpl. The behavior of kill() remains the same, while terminate() just instructs the process launcher to start termination without putting the process into the reaper, yet. Add a test that checks for any possible zombies. We run the RecursiveBlockingProcess recursively. The most nested process blocks. After a short wait we terminate the outermost process. With this test we are trying to see if terminating the middle process terminates also its children and doesn't leave zombies. Before we execute the test (and also by the end of this test) we call Singleton::deleteAll() in order to ensure that reaping of previously running processes has finished. We ensure the number of running processtestapps is zero. Later, we ensure that the leaf process was already started and the number of running processtestapps equals the depth of recursion. Fix apparent bug in getLocalProcessesUsingProc(). Change-Id: I7e2bc46ad5ca22f26620da86fbaf0fa00a7db3c3 Reviewed-by: hjk Reviewed-by: Reviewed-by: Qt CI Bot --- src/libs/utils/launcherpackets.cpp | 6 +- src/libs/utils/launcherpackets.h | 7 +++ src/libs/utils/launchersocket.cpp | 55 +++++++++++------ src/libs/utils/launchersocket.h | 5 +- src/libs/utils/processinfo.cpp | 2 +- src/libs/utils/qtcprocess.cpp | 15 ++--- .../processlauncher/launchersockethandler.cpp | 9 +++ .../processtestapp/processtestapp.cpp | 59 ++++++++++++++++++- .../processtestapp/processtestapp.h | 3 + .../auto/utils/qtcprocess/tst_qtcprocess.cpp | 46 ++++++++++++++- 10 files changed, 172 insertions(+), 35 deletions(-) diff --git a/src/libs/utils/launcherpackets.cpp b/src/libs/utils/launcherpackets.cpp index d9a5d194c1e..07a8b41d1b0 100644 --- a/src/libs/utils/launcherpackets.cpp +++ b/src/libs/utils/launcherpackets.cpp @@ -114,12 +114,14 @@ StopProcessPacket::StopProcessPacket(quintptr token) void StopProcessPacket::doSerialize(QDataStream &stream) const { - Q_UNUSED(stream); + stream << int(signalType); } void StopProcessPacket::doDeserialize(QDataStream &stream) { - Q_UNUSED(stream); + int sig; + stream >> sig; + signalType = SignalType(sig); } void WritePacket::doSerialize(QDataStream &stream) const diff --git a/src/libs/utils/launcherpackets.h b/src/libs/utils/launcherpackets.h index 1fed478d6d8..0989ff791c4 100644 --- a/src/libs/utils/launcherpackets.h +++ b/src/libs/utils/launcherpackets.h @@ -142,6 +142,13 @@ class StopProcessPacket : public LauncherPacket public: StopProcessPacket(quintptr token); + enum class SignalType { + Kill, + Terminate + }; + + SignalType signalType = SignalType::Kill; + private: void doSerialize(QDataStream &stream) const override; void doDeserialize(QDataStream &stream) override; diff --git a/src/libs/utils/launchersocket.cpp b/src/libs/utils/launchersocket.cpp index e6dd83d8bd4..1a9e82b6077 100644 --- a/src/libs/utils/launchersocket.cpp +++ b/src/libs/utils/launchersocket.cpp @@ -308,25 +308,44 @@ QProcess::ProcessState CallerHandle::state() const return m_processState; } -void CallerHandle::cancel() +bool CallerHandle::isStartPacketAwaitingAndClear() +{ + QMutexLocker locker(&m_mutex); + const bool startPacketExisted = m_startPacket.get(); + m_startPacket.reset(); + return startPacketExisted; +} + +void CallerHandle::sendStopPacket(StopProcessPacket::SignalType signalType) +{ + if (m_processState == QProcess::NotRunning) + return; + + if (m_processState == QProcess::Running || !isStartPacketAwaitingAndClear()) { + StopProcessPacket packet(m_token); + packet.signalType = signalType; + sendPacket(packet); + return; + } + + m_processState.store(QProcess::NotRunning); + m_result.m_errorString = QCoreApplication::translate("Utils::LauncherHandle", + "Process was canceled before it was started."); + m_result.m_error = QProcess::FailedToStart; + emit errorOccurred(m_result.m_error); +} + + +void CallerHandle::terminate() { QTC_ASSERT(isCalledFromCallersThread(), return); - switch (m_processState.exchange(QProcess::NotRunning)) { - case QProcess::NotRunning: - break; - case QProcess::Starting: - m_result.m_errorString = QCoreApplication::translate("Utils::LauncherHandle", - "Process was canceled before it was started."); - m_result.m_error = QProcess::FailedToStart; - if (LauncherInterface::isReady()) // TODO: race condition with m_processState??? - sendPacket(StopProcessPacket(m_token)); - else - emit errorOccurred(m_result.m_error); - break; - case QProcess::Running: - sendPacket(StopProcessPacket(m_token)); - break; - } + sendStopPacket(StopProcessPacket::SignalType::Terminate); +} + +void CallerHandle::kill() +{ + QTC_ASSERT(isCalledFromCallersThread(), return); + sendStopPacket(StopProcessPacket::SignalType::Kill); } QByteArray CallerHandle::readAllStandardOutput() @@ -416,7 +435,7 @@ void CallerHandle::doStart() if (!m_startPacket) return; sendPacket(*m_startPacket); - m_startPacket.reset(nullptr); + m_startPacket.reset(); } // Called from caller's or launcher's thread. diff --git a/src/libs/utils/launchersocket.h b/src/libs/utils/launchersocket.h index 4696c516fbc..f4993886c27 100644 --- a/src/libs/utils/launchersocket.h +++ b/src/libs/utils/launchersocket.h @@ -90,7 +90,10 @@ public: // Called from caller's or launcher's thread. QProcess::ProcessState state() const; - void cancel(); + bool isStartPacketAwaitingAndClear(); + void sendStopPacket(StopProcessPacket::SignalType signalType); + void terminate(); + void kill(); QByteArray readAllStandardOutput(); QByteArray readAllStandardError(); diff --git a/src/libs/utils/processinfo.cpp b/src/libs/utils/processinfo.cpp index 67c6177efd1..98776784dca 100644 --- a/src/libs/utils/processinfo.cpp +++ b/src/libs/utils/processinfo.cpp @@ -98,7 +98,7 @@ static QList getLocalProcessesUsingProc() if (proc.executable.isEmpty()) { QFile statFile(root + QLatin1String("/stat")); - if (!statFile.open(QIODevice::ReadOnly)) { + if (statFile.open(QIODevice::ReadOnly)) { const QStringList data = QString::fromLocal8Bit(statFile.readAll()).split(QLatin1Char(' ')); if (data.size() < 2) continue; diff --git a/src/libs/utils/qtcprocess.cpp b/src/libs/utils/qtcprocess.cpp index b7499e1ca2e..f85244b82a9 100644 --- a/src/libs/utils/qtcprocess.cpp +++ b/src/libs/utils/qtcprocess.cpp @@ -422,7 +422,7 @@ public: } ~ProcessLauncherImpl() final { - cancel(); + m_handle->kill(); LauncherInterface::unregisterHandle(token()); m_handle = nullptr; } @@ -435,9 +435,9 @@ public: if (m_setup->m_useCtrlCStub) // bypass launcher and interrupt directly ProcessHelper::interruptPid(processId()); } - void terminate() final { cancel(); } // TODO: what are differences among terminate, kill and close? - void kill() final { cancel(); } // TODO: see above - void close() final { cancel(); } // TODO: see above + void terminate() final { m_handle->terminate(); } + void kill() final { m_handle->kill(); } + void close() final { m_handle->kill(); } // TODO: is it more like terminate or kill? qint64 write(const QByteArray &data) final { return m_handle->write(data); } ProcessResultData resultData() const final { return m_handle->resultData(); }; @@ -456,8 +456,6 @@ private: m_handle->start(program, arguments); } - void cancel(); - quintptr token() const { return m_token; } const uint m_token = 0; @@ -465,11 +463,6 @@ private: CallerHandle *m_handle = nullptr; }; -void ProcessLauncherImpl::cancel() -{ - m_handle->cancel(); -} - static ProcessImpl defaultProcessImpl() { if (qEnvironmentVariableIsSet("QTC_USE_QPROCESS")) diff --git a/src/tools/processlauncher/launchersockethandler.cpp b/src/tools/processlauncher/launchersockethandler.cpp index ccf7fa09509..9b643148430 100644 --- a/src/tools/processlauncher/launchersockethandler.cpp +++ b/src/tools/processlauncher/launchersockethandler.cpp @@ -247,6 +247,15 @@ void LauncherSocketHandler::handleStopPacket() logDebug("Got stop request for unknown process"); return; } + const auto packet = LauncherPacket::extractPacket( + m_packetParser.token(), + m_packetParser.packetData()); + + if (packet.signalType == StopProcessPacket::SignalType::Terminate) { + process->terminate(); + return; + } + if (process->state() == QProcess::NotRunning) { // This shouldn't happen, since as soon as process finishes or error occurrs // the process is being removed. diff --git a/tests/auto/utils/qtcprocess/processtestapp/processtestapp.cpp b/tests/auto/utils/qtcprocess/processtestapp/processtestapp.cpp index cb2473326e0..39fe59ed209 100644 --- a/tests/auto/utils/qtcprocess/processtestapp/processtestapp.cpp +++ b/tests/auto/utils/qtcprocess/processtestapp/processtestapp.cpp @@ -38,6 +38,10 @@ #ifdef Q_OS_WIN #include #include +#else +#include +#include +#include #endif using namespace Utils; @@ -210,7 +214,7 @@ int ProcessTestApp::CrashAfterOneSecond::main() int ProcessTestApp::RecursiveCrashingProcess::main() { const int currentDepth = qEnvironmentVariableIntValue(envVar()); - if (currentDepth == 0) { + if (currentDepth == 1) { QThread::sleep(1); doCrash(); return 1; @@ -224,3 +228,56 @@ int ProcessTestApp::RecursiveCrashingProcess::main() return process.exitCode(); return s_crashCode; } + +#ifndef Q_OS_WIN +static std::atomic_bool s_terminate = false; + +void terminate(int signum) +{ + Q_UNUSED(signum) + s_terminate.store(true); +} +#endif + +int ProcessTestApp::RecursiveBlockingProcess::main() +{ +#ifndef Q_OS_WIN + struct sigaction action; + memset(&action, 0, sizeof(struct sigaction)); + action.sa_handler = terminate; + sigaction(SIGTERM, &action, NULL); +#endif + + const int currentDepth = qEnvironmentVariableIntValue(envVar()); + if (currentDepth == 1) { + std::cout << s_leafProcessStarted << std::flush; + while (true) { + QThread::sleep(1); +#ifndef Q_OS_WIN + if (s_terminate.load()) { + std::cout << s_leafProcessTerminated << std::flush; + return s_crashCode; + } +#endif + } + } + SubProcessConfig subConfig(envVar(), QString::number(currentDepth - 1)); + QtcProcess process; + subConfig.setupSubProcess(&process); + process.setProcessChannelMode(QProcess::ForwardedChannels); + process.start(); + while (true) { + if (process.waitForFinished(1000)) + return 0; +#ifndef Q_OS_WIN + if (s_terminate.load()) { + process.terminate(); + process.waitForFinished(-1); + break; + } +#endif + } + if (process.exitStatus() == QProcess::NormalExit) + return process.exitCode(); + return s_crashCode; +} diff --git a/tests/auto/utils/qtcprocess/processtestapp/processtestapp.h b/tests/auto/utils/qtcprocess/processtestapp/processtestapp.h index 1e338f7a3b2..0ad08d45164 100644 --- a/tests/auto/utils/qtcprocess/processtestapp/processtestapp.h +++ b/tests/auto/utils/qtcprocess/processtestapp/processtestapp.h @@ -71,6 +71,7 @@ public: SUB_PROCESS(EmitOneErrorOnCrash); SUB_PROCESS(CrashAfterOneSecond); SUB_PROCESS(RecursiveCrashingProcess); + SUB_PROCESS(RecursiveBlockingProcess); // In order to get a value associated with the certain subprocess use SubProcessClass::envVar(). // The classes above define different custom executables. Inside invokeSubProcess(), called @@ -119,5 +120,7 @@ enum class BlockType { }; static const int s_crashCode = 123; +static const char s_leafProcessStarted[] = "Leaf process started"; +static const char s_leafProcessTerminated[] = "Leaf process terminated"; Q_DECLARE_METATYPE(BlockType) diff --git a/tests/auto/utils/qtcprocess/tst_qtcprocess.cpp b/tests/auto/utils/qtcprocess/tst_qtcprocess.cpp index 1da9a30769a..cffa22fbd04 100644 --- a/tests/auto/utils/qtcprocess/tst_qtcprocess.cpp +++ b/tests/auto/utils/qtcprocess/tst_qtcprocess.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -162,6 +163,7 @@ private slots: void emitOneErrorOnCrash(); void crashAfterOneSecond(); void recursiveCrashingProcess(); + void recursiveBlockingProcess(); void cleanupTestCase(); @@ -1187,7 +1189,7 @@ void tst_QtcProcess::crashAfterOneSecond() void tst_QtcProcess::recursiveCrashingProcess() { - const int recursionDepth = 5; // must be at least 1 + const int recursionDepth = 5; // must be at least 2 SubProcessConfig subConfig(ProcessTestApp::RecursiveCrashingProcess::envVar(), QString::number(recursionDepth)); TestProcess process; @@ -1199,6 +1201,48 @@ void tst_QtcProcess::recursiveCrashingProcess() QCOMPARE(process.exitStatus(), QProcess::NormalExit); QCOMPARE(process.exitCode(), s_crashCode); } + +static int runningTestProcessCount() +{ + int testProcessCounter = 0; + const QList processInfoList = ProcessInfo::processInfoList(); + for (const ProcessInfo &processInfo : processInfoList) { + if (FilePath::fromString(processInfo.executable).baseName() == "processtestapp") + ++testProcessCounter; + } + return testProcessCounter; +} + +void tst_QtcProcess::recursiveBlockingProcess() +{ + if (HostOsInfo::isWindowsHost()) + QSKIP("Windows implementation of this test is lacking handling of WM_CLOSE message."); + + Singleton::deleteAll(); + QCOMPARE(runningTestProcessCount(), 0); + const int recursionDepth = 5; // must be at least 2 + SubProcessConfig subConfig(ProcessTestApp::RecursiveBlockingProcess::envVar(), + QString::number(recursionDepth)); + { + TestProcess process; + subConfig.setupSubProcess(&process); + process.start(); + QVERIFY(process.waitForStarted(1000)); + QVERIFY(process.waitForReadyRead(1000)); + QCOMPARE(process.readAllStandardOutput(), s_leafProcessStarted); + QCOMPARE(runningTestProcessCount(), recursionDepth); + QVERIFY(!process.waitForFinished(1000)); + process.terminate(); + QVERIFY(process.waitForReadyRead()); + QCOMPARE(process.readAllStandardOutput(), s_leafProcessTerminated); + QVERIFY(process.waitForFinished()); + QCOMPARE(process.exitStatus(), QProcess::NormalExit); + QCOMPARE(process.exitCode(), s_crashCode); + } + Singleton::deleteAll(); + QCOMPARE(runningTestProcessCount(), 0); +} + QTEST_MAIN(tst_QtcProcess) #include "tst_qtcprocess.moc"