Fix handling of unfinished lines by QtcProcess callbacks

Don't detect a call to QtcProcess::kill() from inside the
QtcProcess callback while awaiting inside QtcProcess::waitFor...().
That's not needed, since a call to kill() sends a stop message
to the process launcher, so we wait for confirmation
from process launcher instead. This may bring e.g. new
read data from the running process.

Fix a runBlockingStdOut() test so that when we write to the stdOut
from the running process we flush the unfinished line so that
it's not buffered inside the process.

Change-Id: I7944ac214d8cb9e10a71715a7ef8bfacab6df7c9
Reviewed-by: Alessandro Portale <alessandro.portale@qt.io>
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>

src/libs/utils/launchersocket.cpp

@@ -234,7 +234,7 @@ void CallerHandle::handleReadyRead(const ReadyReadSignal *launcherSignal)
     QTC_ASSERT(isCalledFromCallersThread(), return);
     if (m_setup->m_processChannelMode == QProcess::ForwardedOutputChannel
             || m_setup->m_processChannelMode == QProcess::ForwardedChannels) {
-        std::cout << launcherSignal->stdOut().constData();
+        std::cout << launcherSignal->stdOut().constData() << std::flush;
     } else {
         m_stdout += launcherSignal->stdOut();
         if (!m_stdout.isEmpty())
@@ -242,7 +242,7 @@ void CallerHandle::handleReadyRead(const ReadyReadSignal *launcherSignal)
     }
     if (m_setup->m_processChannelMode == QProcess::ForwardedErrorChannel
             || m_setup->m_processChannelMode == QProcess::ForwardedChannels) {
-        std::cerr << launcherSignal->stdErr().constData();
+        std::cerr << launcherSignal->stdErr().constData() << std::flush;
     } else {
         m_stderr += launcherSignal->stdErr();
         if (!m_stderr.isEmpty())
@@ -305,9 +305,6 @@ void CallerHandle::cancel()
         sendPacket(StopProcessPacket(m_token));
         break;
     }
-
-    if (m_launcherHandle)
-        m_launcherHandle->setCanceled();
 }
 
 QByteArray CallerHandle::readAllStandardOutput()
@@ -504,21 +501,16 @@ bool LauncherHandle::waitForSignal(int msecs, CallerHandle::SignalType newSignal
             break;
         if (!doWaitForSignal(deadline, newSignal))
             break;
-        m_awaitingShouldContinue = true; // TODO: make it recursive?
         const QList<CallerHandle::SignalType> flushedSignals = m_callerHandle->flushFor(newSignal);
-        const bool wasCanceled = !m_awaitingShouldContinue;
-        m_awaitingShouldContinue = false;
         const bool errorOccurred = flushedSignals.contains(CallerHandle::SignalType::Error);
         if (errorOccurred)
-            return false; // apparently QProcess behaves like this in case of error
+            return true; // apparently QProcess behaves like this in case of error
         const bool newSignalFlushed = flushedSignals.contains(newSignal);
         if (newSignalFlushed) // so we don't continue waiting
             return true;
-        if (wasCanceled)
-            return true; // or false? is false only in case of timeout?
         const bool finishedSignalFlushed = flushedSignals.contains(CallerHandle::SignalType::Finished);
         if (finishedSignalFlushed)
-            return false; // finish has appeared but we were waiting for other signal
+            return true; // finish has appeared but we were waiting for other signal
     }
     return false;
 }

src/libs/utils/launchersocket.h

@@ -184,8 +184,6 @@ public:
     bool waitForSignal(int msecs, CallerHandle::SignalType newSignal);
     CallerHandle *callerHandle() const { return m_callerHandle; }
     void setCallerHandle(CallerHandle *handle) { QMutexLocker locker(&m_mutex); m_callerHandle = handle; }
-    // Called from caller's thread exclusively.
-    void setCanceled() { m_awaitingShouldContinue = false; }
 
     // Called from launcher's thread exclusively.
     void handleSocketReady();
@@ -217,8 +215,6 @@ private:
     // Lives in caller's thread. Modified only in caller's thread. TODO: check usages - all should be with mutex
     CallerHandle *m_callerHandle = nullptr;
 
-    // Modified only in caller's thread.
-    bool m_awaitingShouldContinue = false;
     mutable QMutex m_mutex;
     QWaitCondition m_waitCondition;
     const quintptr m_token;

src/libs/utils/qtcprocess.cpp

@@ -1632,6 +1632,8 @@ void QtcProcess::runBlocking(EventLoopMode eventLoopMode)
             d->m_eventLoop = nullptr;
             d->m_stdOut.append(d->m_process->readAllStandardOutput());
             d->m_stdErr.append(d->m_process->readAllStandardError());
+            d->m_stdOut.handleRest();
+            d->m_stdErr.handleRest();
 
             timer.stop();
 #ifdef QT_GUI_LIB
@@ -1651,13 +1653,11 @@ void QtcProcess::runBlocking(EventLoopMode eventLoopMode)
                 kill();
                 waitForFinished(1000);
             }
+            d->m_stdOut.append(d->m_process->readAllStandardOutput());
+            d->m_stdErr.append(d->m_process->readAllStandardError());
+            d->m_stdOut.handleRest();
+            d->m_stdErr.handleRest();
         }
-
-        if (state() != QProcess::NotRunning)
-            return;
-
-        d->m_stdOut.append(d->m_process->readAllStandardOutput());
-        d->m_stdErr.append(d->m_process->readAllStandardError());
     }
 }
 

tests/auto/utils/qtcprocess/tst_qtcprocess.cpp

@@ -966,6 +966,8 @@ void tst_QtcProcess::RunBlockingStdOut::main()
     std::cout << runBlockingStdOutSubProcessMagicWord << "...Now wait for the question...";
     if (qEnvironmentVariable(envVar()) == "true")
         std::cout << std::endl;
+    else
+        std::cout << std::flush; // otherwise it won't reach the original process (will be buffered)
     QThread::msleep(5000);
     exit(0);
 }
@@ -974,22 +976,27 @@ void tst_QtcProcess::runBlockingStdOut_data()
 {
     QTest::addColumn<bool>("withEndl");
     QTest::addColumn<int>("timeOutS");
+    QTest::addColumn<ProcessResult>("expectedResult");
 
-    QTest::newRow("Terminated stdout delivered instantly")
+    // TerminatedAbnormally, since it didn't time out and callback stopped the process forcefully.
-            << true
+    QTest::newRow("Short timeout with end of line")
-            << 2;
+            << true << 2 << ProcessResult::TerminatedAbnormally;
-    QTest::newRow("Unterminated stdout lost: early timeout")
+
-            << false
+    // Hang, since it times out, calls the callback handler and stops the process forcefully.
-            << 2;
+    QTest::newRow("Short timeout without end of line")
-    QTest::newRow("Unterminated stdout lost: hanging")
+            << false << 2 << ProcessResult::Hang;
-            << false
+
-            << 20;
+    // 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
+    // at this point in time since the process is already finished.
+    QTest::newRow("Long timeout without end of line")
+            << false << 20 << ProcessResult::FinishedWithSuccess;}
 
 void tst_QtcProcess::runBlockingStdOut()
 {
     QFETCH(bool, withEndl);
     QFETCH(int, timeOutS);
+    QFETCH(ProcessResult, expectedResult);
 
     SubCreatorConfig subConfig(RunBlockingStdOut::envVar(), withEndl ? "true" : "false");
     TestProcess process;
@@ -1007,9 +1014,7 @@ void tst_QtcProcess::runBlockingStdOut()
 
     // See also QTCREATORBUG-25667 for why it is a bad idea to use QtcProcess::runBlocking
     // with interactive cli tools.
-    QEXPECT_FAIL("Unterminated stdout lost: early timeout", "", Continue);
+    QCOMPARE(process.result(), expectedResult);
-    QVERIFY2(process.result() != ProcessResult::Hang, "Process run did not time out.");
-    QEXPECT_FAIL("Unterminated stdout lost: early timeout", "", Continue);
     QVERIFY2(readLastLine, "Last line was read.");
 }