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 <hjk@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

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<SignalType> oldSignals;
+        QList<LauncherHandle::SignalType> 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<SignalType> m_signals;
+    QList<LauncherHandle::SignalType> 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)
+    // e.g. if we are waiting for ReadyRead and we got Finished signal instead -> wake it, too.
-        m_processState = newState;
+    const bool shouldWake = m_waitingFor == wakeUpSignal
-    const bool shouldWake = m_waitingFor == wakeUpState;
+            || 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<ProcessStartedPacket>(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<ProcessFinishedPacket>(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(SignalType::ReadyRead);
-        m_callerHandle->appendSignal(CallerHandle::SignalType::Finished);
+        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
+    // It may happen, that after calling start() and before calling waitForStarted() we might have
-    // reached the Running or Finished state already. In this case we return true
+    // 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;
+    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()

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!
+    bool waitForStarted(int msecs)
-    { return waitForState(msecs, WaitingForState::Started, QProcess::Running); }
+    { 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 {
+    enum class SignalType {
-        Idle,
+        NoSignal,
         Started,
         ReadyRead,
         Finished
     };
     // called from other thread
-    bool waitForState(int msecs, WaitingForState newState, QProcess::ProcessState targetState);
+    bool waitForSignal(int msecs, SignalType newSignal);
-    bool doWaitForState(int msecs, WaitingForState newState, QProcess::ProcessState targetState);
+    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