From a737ddd931330fc4b1c3718ca238692b68db3c5f Mon Sep 17 00:00:00 2001
From: Jarek Kobus <jaroslaw.kobus@qt.io>
Date: Wed, 6 Dec 2023 17:44:50 +0100
Subject: [PATCH] Process: Use GeneralProcessBlockingImpl for QProcess
 implementation

This should fix the Process::waitForReadyRead() when the data
appeared on stdErr channel and not on stdOut channel.

Amends ec722b9132657f8884c5a813592af4d0871ad03f

Change-Id: Ib9f361f4d1602487bdbbb735e55d0ad24fb5e4c3
Reviewed-by: hjk <hjk@qt.io>
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
---
 src/libs/utils/deviceshell.cpp           |  2 -
 src/libs/utils/process.cpp               | 54 ++++++++++++++----------
 tests/auto/utils/process/tst_process.cpp |  2 -
 3 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/src/libs/utils/deviceshell.cpp b/src/libs/utils/deviceshell.cpp
index 77b197f80ba..8cc100f9aa8 100644
--- a/src/libs/utils/deviceshell.cpp
+++ b/src/libs/utils/deviceshell.cpp
@@ -163,8 +163,6 @@ CommandLine DeviceShell::createFallbackCommand(const CommandLine &cmd)
 expected_str<void> DeviceShell::start()
 {
     m_shellProcess = std::make_unique<Process>();
-    // FIXME: This shouldn't be needed, it's a temporary workaround.
-    m_shellProcess->setProcessImpl(ProcessImpl::ProcessLauncher);
     connect(m_shellProcess.get(), &Process::done, m_shellProcess.get(),
             [this] { emit done(m_shellProcess->resultData()); });
     connect(&m_thread, &QThread::finished, m_shellProcess.get(), [this] { closeShellProcess(); }, Qt::DirectConnection);
diff --git a/src/libs/utils/process.cpp b/src/libs/utils/process.cpp
index 29f4a1f852b..d1a8f812189 100644
--- a/src/libs/utils/process.cpp
+++ b/src/libs/utils/process.cpp
@@ -292,27 +292,37 @@ bool DefaultImpl::ensureProgramExists(const QString &program)
     return false;
 }
 
-class QProcessBlockingImpl : public ProcessBlockingInterface
-{
-public:
-    QProcessBlockingImpl(QProcess *process) : m_process(process) {}
+// TODO: Remove QProcessBlockingImpl later, after Creator 13.0 is released at least.
 
-private:
-    bool waitForSignal(ProcessSignalType signalType, int msecs) final
-    {
-        switch (signalType) {
-        case ProcessSignalType::Started:
-            return m_process->waitForStarted(msecs);
-        case ProcessSignalType::ReadyRead:
-            return m_process->waitForReadyRead(msecs);
-        case ProcessSignalType::Done:
-            return m_process->waitForFinished(msecs);
-        }
-        return false;
-    }
+// Rationale: QProcess::waitForReadyRead() waits only for one channel, either stdOut or stdErr.
+// Since we can't predict where the data will come first,
+// setting the QProcess::setReadChannel() in advance is a mis-design of the QProcess API.
+// This issue does not affect GeneralProcessBlockingImpl, but it might be not as optimal
+// as QProcessBlockingImpl. However, since we are blocking the caller thread anyway,
+// the small overhead in speed doesn't play the most significant role, thus the proper
+// behavior of Process::waitForReadyRead(), which listens to both channels, wins.
 
-    QProcess *m_process = nullptr;
-};
+// class QProcessBlockingImpl : public ProcessBlockingInterface
+// {
+// public:
+//     QProcessBlockingImpl(QProcess *process) : m_process(process) {}
+
+// private:
+//     bool waitForSignal(ProcessSignalType signalType, int msecs) final
+//     {
+//         switch (signalType) {
+//         case ProcessSignalType::Started:
+//             return m_process->waitForStarted(msecs);
+//         case ProcessSignalType::ReadyRead:
+//             return m_process->waitForReadyRead(msecs);
+//         case ProcessSignalType::Done:
+//             return m_process->waitForFinished(msecs);
+//         }
+//         return false;
+//     }
+
+//     QProcess *m_process = nullptr;
+// };
 
 class PtyProcessImpl final : public DefaultImpl
 {
@@ -445,7 +455,7 @@ class QProcessImpl final : public DefaultImpl
 public:
     QProcessImpl()
         : m_process(new ProcessHelper(this))
-        , m_blockingImpl(new QProcessBlockingImpl(m_process))
+        // , m_blockingImpl(new QProcessBlockingImpl(m_process))
     {
         connect(m_process, &QProcess::started, this, &QProcessImpl::handleStarted);
         connect(m_process, &QProcess::finished, this, &QProcessImpl::handleFinished);
@@ -481,7 +491,7 @@ private:
         }
     }
 
-    ProcessBlockingInterface *processBlockingInterface() const override { return m_blockingImpl; }
+    // ProcessBlockingInterface *processBlockingInterface() const override { return m_blockingImpl; }
 
     void doDefaultStart(const QString &program, const QStringList &arguments) final
     {
@@ -533,7 +543,7 @@ private:
     }
 
     ProcessHelper *m_process = nullptr;
-    QProcessBlockingImpl *m_blockingImpl = nullptr;
+    // QProcessBlockingImpl *m_blockingImpl = nullptr;
 };
 
 static uint uniqueToken()
diff --git a/tests/auto/utils/process/tst_process.cpp b/tests/auto/utils/process/tst_process.cpp
index 7aa27e7b241..6ba3989b2d9 100644
--- a/tests/auto/utils/process/tst_process.cpp
+++ b/tests/auto/utils/process/tst_process.cpp
@@ -272,8 +272,6 @@ void tst_Process::multiRead()
     Process process;
     subConfig.setupSubProcess(&process);
 
-    // TODO: Fix ProcessImpl::QProcess
-    process.setProcessImpl(ProcessImpl::ProcessLauncher);
     process.setProcessMode(ProcessMode::Writer);
     process.start();