From 48c1f56ebee8e4386b9a7633f9cc553ed5b3c210 Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Tue, 3 Aug 2021 13:29:28 +0200 Subject: [PATCH] Refactor waiting for logic Instead of waiting for certain state we wait for certain signal to be placed into the buffer. No need for m_finished flag anymore, since we are only interesting if the buffer contains the finished signal. This will enable to implement waiting for ready read properly, since no state is changed in this case. Change-Id: I4209da385b2e37de6f1897357e35c0ed0c9e4096 Reviewed-by: hjk Reviewed-by: Qt CI Bot --- src/libs/utils/launchersocket.cpp | 108 ++++++++++++++++++++---------- src/libs/utils/launchersocket.h | 25 ++++--- 2 files changed, 87 insertions(+), 46 deletions(-) diff --git a/src/libs/utils/launchersocket.cpp b/src/libs/utils/launchersocket.cpp index c5b997f3068..20b1ca0148c 100644 --- a/src/libs/utils/launchersocket.cpp +++ b/src/libs/utils/launchersocket.cpp @@ -40,43 +40,58 @@ class CallerHandle : public QObject public: CallerHandle() : QObject() {} - enum class SignalType { - Started, - ReadyRead, - Finished - }; - // always called in caller's thread void flush() { - QList oldSignals; + QList oldSignals; { QMutexLocker locker(&m_mutex); oldSignals = m_signals; m_signals = {}; } - for (SignalType signalType : qAsConst(oldSignals)) { + for (LauncherHandle::SignalType signalType : qAsConst(oldSignals)) { switch (signalType) { - case SignalType::Started: + case LauncherHandle::SignalType::NoSignal: + break; + case LauncherHandle::SignalType::Started: emit started(); break; - case SignalType::ReadyRead: + case LauncherHandle::SignalType::ReadyRead: emit readyRead(); break; - case SignalType::Finished: + case LauncherHandle::SignalType::Finished: emit finished(); break; } } } - void appendSignal(SignalType signalType) { QMutexLocker locker(&m_mutex); m_signals.append(signalType); } + void appendSignal(LauncherHandle::SignalType signalType) + { + if (signalType == LauncherHandle::SignalType::NoSignal) + return; + + QMutexLocker locker(&m_mutex); + if (m_signals.contains(signalType)) + return; + + m_signals.append(signalType); + } + bool shouldFlushFor(LauncherHandle::SignalType signalType) + { + QMutexLocker locker(&m_mutex); + if (m_signals.contains(signalType)) + return true; + if (m_signals.contains(LauncherHandle::SignalType::Finished)) + return true; + return false; + } signals: void started(); void readyRead(); void finished(); private: QMutex m_mutex; - QList m_signals; + QList m_signals; }; void LauncherHandle::handlePacket(LauncherPacketType type, const QByteArray &payload) @@ -101,9 +116,9 @@ void LauncherHandle::handleErrorPacket(const QByteArray &packetData) QMutexLocker locker(&m_mutex); if (!m_canceled) m_processState = QProcess::NotRunning; - if (m_waitingFor != WaitingForState::Idle) { + if (m_waitingFor != SignalType::NoSignal) { m_waitCondition.wakeOne(); - m_waitingFor = WaitingForState::Idle; + m_waitingFor = SignalType::NoSignal; } m_failed = true; @@ -115,14 +130,14 @@ void LauncherHandle::handleErrorPacket(const QByteArray &packetData) } // call me with mutex locked -void LauncherHandle::stateReached(WaitingForState wakeUpState, QProcess::ProcessState newState) +void LauncherHandle::wakeUpIfWaitingFor(SignalType wakeUpSignal) { - if (!m_canceled) - m_processState = newState; - const bool shouldWake = m_waitingFor == wakeUpState; + // e.g. if we are waiting for ReadyRead and we got Finished signal instead -> wake it, too. + const bool shouldWake = m_waitingFor == wakeUpSignal + || m_waitingFor != SignalType::NoSignal && wakeUpSignal == SignalType::Finished; if (shouldWake) { m_waitCondition.wakeOne(); - m_waitingFor = WaitingForState::Idle; + m_waitingFor = SignalType::NoSignal; } } @@ -144,13 +159,14 @@ void LauncherHandle::flushCaller() void LauncherHandle::handleStartedPacket(const QByteArray &packetData) { QMutexLocker locker(&m_mutex); - stateReached(WaitingForState::Started, QProcess::Running); + wakeUpIfWaitingFor(SignalType::Started); if (m_canceled) return; + m_processState = QProcess::Running; const auto packet = LauncherPacket::extractPacket(m_token, packetData); m_processId = packet.processId; if (m_callerHandle) { - m_callerHandle->appendSignal(CallerHandle::SignalType::Started); + m_callerHandle->appendSignal(SignalType::Started); flushCaller(); } } @@ -158,10 +174,10 @@ void LauncherHandle::handleStartedPacket(const QByteArray &packetData) void LauncherHandle::handleFinishedPacket(const QByteArray &packetData) { QMutexLocker locker(&m_mutex); - stateReached(WaitingForState::Finished, QProcess::NotRunning); + wakeUpIfWaitingFor(SignalType::Finished); if (m_canceled) return; - m_finished = true; + m_processState = QProcess::NotRunning; const auto packet = LauncherPacket::extractPacket(m_token, packetData); m_exitCode = packet.exitCode; m_stdout = packet.stdOut; @@ -170,8 +186,8 @@ void LauncherHandle::handleFinishedPacket(const QByteArray &packetData) m_exitStatus = packet.exitStatus; if (m_callerHandle) { if (!m_stdout.isEmpty() || !m_stderr.isEmpty()) - m_callerHandle->appendSignal(CallerHandle::SignalType::ReadyRead); - m_callerHandle->appendSignal(CallerHandle::SignalType::Finished); + m_callerHandle->appendSignal(SignalType::ReadyRead); + m_callerHandle->appendSignal(SignalType::Finished); flushCaller(); } } @@ -196,29 +212,51 @@ void LauncherHandle::handleSocketError(const QString &message) } } -bool LauncherHandle::waitForState(int msecs, WaitingForState newState, QProcess::ProcessState targetState) +bool LauncherHandle::waitForSignal(int msecs, SignalType newSignal) { - const bool ok = doWaitForState(msecs, newState, targetState); + const bool ok = doWaitForSignal(msecs, newSignal); if (ok) m_callerHandle->flush(); return ok; } -bool LauncherHandle::doWaitForState(int msecs, WaitingForState newState, QProcess::ProcessState targetState) +bool LauncherHandle::doWaitForSignal(int msecs, SignalType newSignal) { QMutexLocker locker(&m_mutex); - // TODO: ASSERT if we are in Idle state + QTC_ASSERT(m_waitingFor == SignalType::NoSignal, return false); if (m_canceled) // we don't want to wait if we have canceled it before (ASSERT it?) return false; - // It may happen, than after calling start() and before calling waitForStarted() we might have - // reached the Running or Finished state already. In this case we return true + // It may happen, that after calling start() and before calling waitForStarted() we might have + // reached the Running (or even Finished) state already. In this case we should have + // collected Started (or even Finished) signal to be flushed - so we return true // and we are going to flush pending signals synchronously. - if (m_processState == targetState || m_finished) + // It could also happen, that some new readyRead data has appeared, so before we wait for + // more we flush it, too. + if (m_callerHandle->shouldFlushFor(newSignal)) return true; - m_waitingFor = newState; - return m_waitCondition.wait(&m_mutex, msecs) && !m_failed; + if (canWaitFor(newSignal)) { // e.g. can't wait for started if we are in not running state. + m_waitingFor = newSignal; + return m_waitCondition.wait(&m_mutex, msecs) && !m_failed; + } + + return false; +} + +// call me with mutex locked +bool LauncherHandle::canWaitFor(SignalType newSignal) const +{ + switch (newSignal) { + case SignalType::Started: + return m_processState == QProcess::Starting; + case SignalType::ReadyRead: + case SignalType::Finished: + return m_processState != QProcess::NotRunning; + default: + break; + } + return false; } void LauncherHandle::cancel() diff --git a/src/libs/utils/launchersocket.h b/src/libs/utils/launchersocket.h index a8499bd5466..f6a0b9d5418 100644 --- a/src/libs/utils/launchersocket.h +++ b/src/libs/utils/launchersocket.h @@ -62,10 +62,12 @@ class LauncherHandle : public QObject Q_OBJECT public: // called from main thread - bool waitForStarted(int msecs) // TODO: we might already be in finished state when calling this method - fix it! - { return waitForState(msecs, WaitingForState::Started, QProcess::Running); } + bool waitForStarted(int msecs) + { return waitForSignal(msecs, SignalType::Started); } + bool waitForReadyRead(int msces) + { return waitForSignal(msces, SignalType::ReadyRead); } bool waitForFinished(int msecs) - { return waitForState(msecs, WaitingForState::Finished, QProcess::NotRunning); } + { return waitForSignal(msecs, SignalType::Finished); } QProcess::ProcessState state() const { QMutexLocker locker(&m_mutex); return m_processState; } @@ -83,7 +85,7 @@ public: // Called from other thread. Create a temp object receiver which lives in caller's thread. // Add started and finished signals to it and post a flush to it. - // When we are in waitForState() which is called from the same thread, + // When we are in waitForSignal() which is called from the same thread, // we may flush the signal queue and emit these signals immediately. // Who should remove this object? deleteLater()? void start(const QString &program, const QStringList &arguments, QIODevice::OpenMode mode); @@ -110,15 +112,16 @@ signals: void readyReadStandardOutput(); void readyReadStandardError(); private: - enum class WaitingForState { - Idle, + enum class SignalType { + NoSignal, Started, ReadyRead, Finished }; // called from other thread - bool waitForState(int msecs, WaitingForState newState, QProcess::ProcessState targetState); - bool doWaitForState(int msecs, WaitingForState newState, QProcess::ProcessState targetState); + bool waitForSignal(int msecs, SignalType newSignal); + bool doWaitForSignal(int msecs, SignalType newSignal); + bool canWaitFor(SignalType newSignal) const; void doStart(); @@ -141,7 +144,7 @@ private: void handleSocketReady(); void handleSocketError(const QString &message); - void stateReached(WaitingForState wakeUpState, QProcess::ProcessState newState); + void wakeUpIfWaitingFor(SignalType wakeUpSignal); QByteArray readAndClear(QByteArray &data) { @@ -155,12 +158,11 @@ private: mutable QMutex m_mutex; QWaitCondition m_waitCondition; const quintptr m_token; - WaitingForState m_waitingFor = WaitingForState::Idle; + SignalType m_waitingFor = SignalType::NoSignal; QProcess::ProcessState m_processState = QProcess::NotRunning; std::atomic_bool m_canceled = false; std::atomic_bool m_failed = false; - std::atomic_bool m_finished = false; int m_processId = 0; int m_exitCode = 0; QProcess::ExitStatus m_exitStatus = QProcess::ExitStatus::NormalExit; @@ -181,6 +183,7 @@ private: CallerHandle *m_callerHandle = nullptr; friend class LauncherSocket; + friend class CallerHandle; }; class LauncherSocket : public QObject