ProcessLauncher: Pass start packet into the launcher socket

Instead of holding the start packet on caller side when
launcher socket isn't ready yet, pass it into the launcher
socket and buffer it there until the socket becomes ready.
This simplifies starting the process considerably.

Get rid of CallerHandle::canWaitFor(), as this is already
checked by QtcProcess itself.

Get rid of LauncherHandle::m_waitingFor field and
LauncherInterface::isReady() method, as both are not used anymore.

Change-Id: Ida6f0629170647249e562028c3ea5db1830b8a0d
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>

src/libs/utils/launcherinterface.cpp

@@ -227,12 +227,6 @@ bool LauncherInterface::isStarted()
     return s_started;
 }
 
-bool LauncherInterface::isReady()
-{
-    QMutexLocker locker(&s_instanceMutex);
-    return instance()->m_private->socket()->isReady();
-}
-
 void LauncherInterface::sendData(const QByteArray &data)
 {
     QMutexLocker locker(&s_instanceMutex);

src/libs/utils/launcherinterface.h

@@ -52,7 +52,6 @@ private:
     friend class Utils::Internal::ProcessLauncherImpl;
 
     static bool isStarted();
-    static bool isReady();
     static void sendData(const QByteArray &data);
     static Utils::Internal::CallerHandle *registerHandle(QObject *parent, quintptr token);
     static void unregisterHandle(quintptr token);

src/libs/utils/launchersocket.cpp

@@ -234,32 +234,18 @@ QProcess::ProcessState CallerHandle::state() const
     return m_processState;
 }
 
-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()) {
+    // TODO: In case m_processState == QProcess::Starting and the launcher socket isn't ready yet
+    // we might want to remove posted start packet and finish the process immediately.
+    // In addition, we may always try to check if correspodning start packet for the m_token
+    // is still awaiting and do the same (remove the packet from the stack and finish immediately).
     StopProcessPacket packet(m_token);
     packet.signalType = signalType;
     sendPacket(packet);
-        return;
-    }
-
-    m_processState.store(QProcess::NotRunning);
-    const QString errorString = QCoreApplication::translate("Utils::LauncherHandle",
-                                "Process was canceled before it was started.");
-    const ProcessResultData result = { 0, QProcess::NormalExit, QProcess::FailedToStart,
-                                       errorString };
-    emit done(result);
 }
 
 void CallerHandle::terminate()
@@ -308,43 +294,24 @@ void CallerHandle::start(const QString &program, const QStringList &arguments)
     m_command = program;
     m_arguments = arguments;
     m_processState = QProcess::Starting;
-    StartProcessPacket *p = new StartProcessPacket(m_token);
+    StartProcessPacket p(m_token);
-    p->command = m_command;
+    p.command = m_command;
-    p->arguments = m_arguments;
+    p.arguments = m_arguments;
-    p->env = m_setup->m_environment.toStringList();
+    p.env = m_setup->m_environment.toStringList();
-    p->workingDir = m_setup->m_workingDirectory.path();
+    p.workingDir = m_setup->m_workingDirectory.path();
-    p->processMode = m_setup->m_processMode;
+    p.processMode = m_setup->m_processMode;
-    p->writeData = m_setup->m_writeData;
+    p.writeData = m_setup->m_writeData;
-    p->processChannelMode = m_setup->m_processChannelMode;
+    p.processChannelMode = m_setup->m_processChannelMode;
-    p->standardInputFile = m_setup->m_standardInputFile;
+    p.standardInputFile = m_setup->m_standardInputFile;
-    p->belowNormalPriority = m_setup->m_belowNormalPriority;
+    p.belowNormalPriority = m_setup->m_belowNormalPriority;
-    p->nativeArguments = m_setup->m_nativeArguments;
+    p.nativeArguments = m_setup->m_nativeArguments;
-    p->lowPriority = m_setup->m_lowPriority;
+    p.lowPriority = m_setup->m_lowPriority;
-    p->unixTerminalDisabled = m_setup->m_unixTerminalDisabled;
+    p.unixTerminalDisabled = m_setup->m_unixTerminalDisabled;
-    p->useCtrlCStub = m_setup->m_useCtrlCStub;
+    p.useCtrlCStub = m_setup->m_useCtrlCStub;
-    m_startPacket.reset(p);
+    sendPacket(p);
-    if (LauncherInterface::isReady())
-        doStart();
 }
 
-// Called from caller's or launcher's thread.
+// Called from caller's thread exclusively.
-void CallerHandle::startIfNeeded()
-{
-    QMutexLocker locker(&m_mutex);
-    if (m_processState == QProcess::Starting)
-        doStart();
-}
-
-// Called from caller's or launcher's thread. Call me with mutex locked.
-void CallerHandle::doStart()
-{
-    if (!m_startPacket)
-        return;
-    sendPacket(*m_startPacket);
-    m_startPacket.reset();
-}
-
-// Called from caller's or launcher's thread.
 void CallerHandle::sendPacket(const Internal::LauncherPacket &packet)
 {
     LauncherInterface::sendData(packet.serialize());
@@ -384,28 +351,10 @@ void CallerHandle::setProcessSetupData(ProcessSetupData *setup)
 bool CallerHandle::waitForSignal(int msecs, SignalType newSignal)
 {
     QTC_ASSERT(isCalledFromCallersThread(), return false);
-    if (!canWaitFor(newSignal))
+    QTC_ASSERT(m_launcherHandle, return false);
-        return false;
-    if (!m_launcherHandle)
-        return false;
     return m_launcherHandle->waitForSignal(msecs, newSignal);
 }
 
-bool CallerHandle::canWaitFor(SignalType newSignal) const
-{
-    QTC_ASSERT(isCalledFromCallersThread(), return false);
-    switch (newSignal) {
-    case SignalType::Started:
-        return m_processState == QProcess::Starting;
-    case SignalType::ReadyRead:
-    case SignalType::Done:
-        return m_processState != QProcess::NotRunning;
-    default:
-        break;
-    }
-    return false;
-}
-
 // Called from caller's or launcher's thread.
 bool CallerHandle::isCalledFromCallersThread() const
 {
@@ -428,7 +377,7 @@ bool LauncherHandle::waitForSignal(int msecs, CallerHandle::SignalType newSignal
     while (true) {
         if (deadline.hasExpired())
             break;
-        if (!doWaitForSignal(deadline, newSignal))
+        if (!doWaitForSignal(deadline))
             break;
         // Matching (or Done) signal was flushed
         if (m_callerHandle->flushFor(newSignal))
@@ -439,11 +388,10 @@ bool LauncherHandle::waitForSignal(int msecs, CallerHandle::SignalType newSignal
 }
 
 // Called from caller's thread exclusively.
-bool LauncherHandle::doWaitForSignal(QDeadlineTimer deadline, CallerHandle::SignalType newSignal)
+bool LauncherHandle::doWaitForSignal(QDeadlineTimer deadline)
 {
     QMutexLocker locker(&m_mutex);
     QTC_ASSERT(isCalledFromCallersThread(), return false);
-    QTC_ASSERT(m_waitingFor == CallerHandle::SignalType::NoSignal, return false);
 
     // Flush, if we have any stored signals.
     // This must be called when holding laucher's mutex locked prior to the call to wait,
@@ -451,10 +399,7 @@ bool LauncherHandle::doWaitForSignal(QDeadlineTimer deadline, CallerHandle::Sign
     if (m_callerHandle->shouldFlush())
         return true;
 
-    m_waitingFor = newSignal;
+    return m_waitCondition.wait(&m_mutex, deadline);
-    const bool ret = m_waitCondition.wait(&m_mutex, deadline);
-    m_waitingFor = CallerHandle::SignalType::NoSignal;
-    return ret;
 }
 
 // Called from launcher's thread exclusively. Call me with mutex locked.
@@ -552,15 +497,6 @@ void LauncherHandle::handleDonePacket(const QByteArray &packetData)
     flushCaller();
 }
 
-void LauncherHandle::handleSocketReady()
-{
-    QTC_ASSERT(isCalledFromLaunchersThread(), return);
-    m_socketError = false;
-    QMutexLocker locker(&m_mutex);
-    if (m_callerHandle)
-        m_callerHandle->startIfNeeded();
-}
-
 void LauncherHandle::handleSocketError(const QString &message)
 {
     QTC_ASSERT(isCalledFromLaunchersThread(), return);
@@ -618,9 +554,6 @@ LauncherSocket::~LauncherSocket()
 
 void LauncherSocket::sendData(const QByteArray &data)
 {
-    if (!isReady())
-        return;
-
     auto storeRequest = [this](const QByteArray &data)
     {
         QMutexLocker locker(&m_mutex);
@@ -647,8 +580,6 @@ CallerHandle *LauncherSocket::registerHandle(QObject *parent, quintptr token)
     // Call it after moving LauncherHandle to the launcher's thread.
     // Since this method is invoked from caller's thread, CallerHandle will live in caller's thread.
     m_handles.insert(token, launcherHandle);
-    connect(this, &LauncherSocket::ready,
-            launcherHandle, &LauncherHandle::handleSocketReady);
     connect(this, &LauncherSocket::errorOccurred,
             launcherHandle, &LauncherHandle::handleSocketError);
 
@@ -685,14 +616,13 @@ void LauncherSocket::setSocket(QLocalSocket *socket)
     QTC_ASSERT(!m_socket, return);
     m_socket.store(socket);
     m_packetParser.setDevice(m_socket);
-    connect(m_socket,
+    connect(m_socket, &QLocalSocket::errorOccurred,
-            &QLocalSocket::errorOccurred,
             this, &LauncherSocket::handleSocketError);
     connect(m_socket, &QLocalSocket::readyRead,
             this, &LauncherSocket::handleSocketDataAvailable);
     connect(m_socket, &QLocalSocket::disconnected,
             this, &LauncherSocket::handleSocketDisconnected);
-    emit ready();
+    handleRequests();
 }
 
 void LauncherSocket::shutdown()
@@ -769,13 +699,20 @@ void LauncherSocket::handleRequests()
 {
     QTC_ASSERT(isCalledFromLaunchersThread(), return);
     const auto socket = m_socket.load();
-    QTC_ASSERT(socket, return);
+    if (!socket)
+        return;
+
+    std::vector<QByteArray> requests;
+    {
         QMutexLocker locker(&m_mutex);
-    for (const QByteArray &request : qAsConst(m_requests))
+        requests = m_requests;
-        socket->write(request);
         m_requests.clear();
     }
 
+    for (const QByteArray &request : qAsConst(requests))
+        socket->write(request);
+}
+
 bool LauncherSocket::isCalledFromLaunchersThread() const
 {
     return QThread::currentThread() == thread();

src/libs/utils/launchersocket.h

@@ -88,7 +88,6 @@ public:
 
     // Called from caller's or launcher's thread.
     QProcess::ProcessState state() const;
-    bool isStartPacketAwaitingAndClear();
     void sendStopPacket(StopProcessPacket::SignalType signalType);
     void terminate();
     void kill();
@@ -96,8 +95,6 @@ public:
     qint64 processId() const;
 
     void start(const QString &program, const QStringList &arguments);
-    // Called from caller's or launcher's thread.
-    void startIfNeeded();
 
     qint64 write(const QByteArray &data);
 
@@ -114,11 +111,8 @@ signals:
 
 private:
     bool waitForSignal(int msecs, SignalType newSignal);
-    bool canWaitFor(SignalType newSignal) const;
 
-    // Called from caller's or launcher's thread. Call me with mutex locked.
+    // Called from caller's thread exclusively.
-    void doStart();
-    // Called from caller's or launcher's thread.
     void sendPacket(const Internal::LauncherPacket &packet);
     // Called from caller's or launcher's thread.
     bool isCalledFromCallersThread() const;
@@ -147,7 +141,6 @@ private:
 
     // Modified from caller's thread, read from launcher's thread
     std::atomic<QProcess::ProcessState> m_processState = QProcess::NotRunning;
-    std::unique_ptr<StartProcessPacket> m_startPacket;
     int m_processId = 0;
 
     QString m_command;
@@ -171,7 +164,6 @@ public:
     void setCallerHandle(CallerHandle *handle) { QMutexLocker locker(&m_mutex); m_callerHandle = handle; }
 
     // Called from launcher's thread exclusively.
-    void handleSocketReady();
     void handleSocketError(const QString &message);
     void handlePacket(LauncherPacketType type, const QByteArray &payload);
 
@@ -180,7 +172,7 @@ public:
 
 private:
     // Called from caller's thread exclusively.
-    bool doWaitForSignal(QDeadlineTimer deadline, CallerHandle::SignalType newSignal);
+    bool doWaitForSignal(QDeadlineTimer deadline);
     // Called from launcher's thread exclusively. Call me with mutex locked.
     void flushCaller();
     // Called from launcher's thread exclusively.
@@ -200,8 +192,6 @@ private:
     QWaitCondition m_waitCondition;
     const quintptr m_token;
     std::atomic_bool m_socketError = false;
-    // Modified only in caller's thread.
-    CallerHandle::SignalType m_waitingFor = CallerHandle::SignalType::NoSignal;
 };
 
 class LauncherSocket : public QObject
@@ -209,16 +199,12 @@ class LauncherSocket : public QObject
     Q_OBJECT
     friend class LauncherInterfacePrivate;
 public:
-    // Called from caller's or launcher's thread.
-    bool isReady() const { return m_socket.load(); }
-    void sendData(const QByteArray &data);
-
     // Called from caller's thread exclusively.
+    void sendData(const QByteArray &data);
     CallerHandle *registerHandle(QObject *parent, quintptr token);
     void unregisterHandle(quintptr token);
 
 signals:
-    void ready();
     void errorOccurred(const QString &error);
 
 private: