Clang: Start ConnectionClient asynchronously

The connection client can block main thread at start up. This patch is
removing the wait functions and using signals instead.

Change-Id: I847c98b095752f6a875c0365bc27361bc5bdd051
Reviewed-by: Tim Jenssen <tim.jenssen@theqtcompany.com>
Reviewed-by: Nikolai Kosjar <nikolai.kosjar@qt.io>

src/libs/clangbackendipc/connectionclient.cpp

@@ -31,6 +31,7 @@
 #include "cmbunregistertranslationunitsforeditormessage.h"
 
 #include <QCoreApplication>
+#include <QMetaMethod>
 #include <QProcess>
 #include <QTemporaryDir>
 #include <QThread>
@@ -58,7 +59,6 @@ QString connectionName()
 
 ConnectionClient::ConnectionClient(ClangCodeModelClientInterface *client)
     : serverProxy_(client, &localSocket),
-      isAliveTimerResetted(false),
       stdErrPrefixer("clangbackend.stderr: "),
       stdOutPrefixer("clangbackend.stdout: ")
 {
@@ -66,15 +66,11 @@ ConnectionClient::ConnectionClient(ClangCodeModelClientInterface *client)
 
     const bool startAliveTimer = !qgetenv("QTC_CLANG_NO_ALIVE_TIMER").toInt();
 
-    if (startAliveTimer) {
+    if (startAliveTimer)
-        connect(&processAliveTimer, &QTimer::timeout,
+        connectAliveTimer();
-                this, &ConnectionClient::restartProcessIfTimerIsNotResettedAndSocketIsEmpty);
-    }
 
-    connect(&localSocket,
+    connectLocalSocketError();
-            static_cast<void (QLocalSocket::*)(QLocalSocket::LocalSocketError)>(&QLocalSocket::error),
+    connectLocalSocketConnected();
-            this,
-            &ConnectionClient::printLocalSocketError);
 }
 
 ConnectionClient::~ConnectionClient()
@@ -82,15 +78,9 @@ ConnectionClient::~ConnectionClient()
     finishProcess();
 }
 
-bool ConnectionClient::connectToServer()
+void ConnectionClient::startProcessAndConnectToServerAsynchronously()
 {
-    TIME_SCOPE_DURATION("ConnectionClient::connectToServer");
+    process_  = startProcess();
-
-    startProcess();
-    resetProcessAliveTimer();
-    const bool isConnected = connectToLocalSocket();
-
-    return isConnected;
 }
 
 bool ConnectionClient::disconnectFromServer()
@@ -145,28 +135,28 @@ QProcessEnvironment ConnectionClient::processEnvironment() const
     return processEnvironment;
 }
 
-void ConnectionClient::startProcess()
+std::unique_ptr<QProcess> ConnectionClient::startProcess()
 {
-    TIME_SCOPE_DURATION("ConnectionClient::startProcess");
+    processIsStarting = true;
 
-    if (!isProcessIsRunning()) {
+    auto process = std::unique_ptr<QProcess>(new QProcess);
-        connectProcessFinished();
+    connectProcessFinished(process.get());
-        connectStandardOutputAndError();
+    connectProcessStarted(process.get());
-        process()->setProcessEnvironment(processEnvironment());
+    connectStandardOutputAndError(process.get());
-        process()->start(processPath(), {connectionName()});
+    process->setProcessEnvironment(processEnvironment());
-        process()->waitForStarted();
+    process->start(processPath(), {connectionName()});
     resetProcessAliveTimer();
-    }
+
+    return process;
 }
 
-void ConnectionClient::restartProcess()
+void ConnectionClient::restartProcessAsynchronously()
 {
-    finishProcess();
+    if (!processIsStarting) {
-    startProcess();
+        finishProcess(std::move(process_));
 
-    connectToServer();
+        startProcessAndConnectToServerAsynchronously();
-
+    }
-    emit processRestarted();
 }
 
 void ConnectionClient::restartProcessIfTimerIsNotResettedAndSocketIsEmpty()
@@ -179,52 +169,48 @@ void ConnectionClient::restartProcessIfTimerIsNotResettedAndSocketIsEmpty()
     if (localSocket.bytesAvailable() > 0)
         return; // We come first, the incoming data was not yet processed.
 
-    restartProcess();
+    restartProcessAsynchronously();
 }
 
-bool ConnectionClient::connectToLocalSocket()
+void ConnectionClient::connectToLocalSocket()
 {
-    for (int counter = 0; counter < 1000; counter++) {
+    if (!isConnected()) {
         localSocket.connectToServer(connectionName());
-        bool isConnected = localSocket.waitForConnected(20);
+        QTimer::singleShot(20, this, &ConnectionClient::connectToLocalSocket);
-
+    }
-        if (isConnected)
-            return isConnected;
-        else
-            QThread::msleep(30);
 }
 
-    qDebug() << "Cannot connect:" <<localSocket.errorString();
+void ConnectionClient::endProcess(QProcess *process)
-
-    return false;
-}
-
-void ConnectionClient::endProcess()
 {
-    if (isProcessIsRunning()) {
+    if (isProcessIsRunning() && isConnected()) {
         sendEndMessage();
-        process()->waitForFinished();
+        process->waitForFinished();
     }
 }
 
-void ConnectionClient::terminateProcess()
+void ConnectionClient::terminateProcess(QProcess *process)
 {
 #ifndef Q_OS_WIN32
     if (isProcessIsRunning()) {
-        process()->terminate();
+        process->terminate();
-        process()->waitForFinished();
+        process->waitForFinished();
     }
 #endif
 }
 
-void ConnectionClient::killProcess()
+void ConnectionClient::killProcess(QProcess *process)
 {
     if (isProcessIsRunning()) {
-        process()->kill();
+        process->kill();
-        process()->waitForFinished();
+        process->waitForFinished();
     }
 }
 
+void ConnectionClient::resetProcessIsStarting()
+{
+    processIsStarting = false;
+}
+
 void ConnectionClient::printLocalSocketError(QLocalSocket::LocalSocketError socketError)
 {
     if (socketError != QLocalSocket::ServerNotFoundError)
@@ -241,28 +227,65 @@ void ConnectionClient::printStandardError()
     qDebug("%s", stdErrPrefixer.prefix(process_->readAllStandardError()).constData());
 }
 
+void ConnectionClient::connectLocalSocketConnected()
+{
+    connect(&localSocket,
+            &QLocalSocket::connected,
+            this,
+            &ConnectionClient::connectedToLocalSocket);
+
+    connect(&localSocket,
+            &QLocalSocket::connected,
+            this,
+            &ConnectionClient::resetProcessIsStarting);
+}
+
 void ConnectionClient::finishProcess()
+{
+    finishProcess(std::move(process_));
+}
+
+void ConnectionClient::finishProcess(std::unique_ptr<QProcess> &&process)
 {
     TIME_SCOPE_DURATION("ConnectionClient::finishProcess");
 
+    if (process) {
         processAliveTimer.stop();
 
-    disconnectProcessFinished();
+        disconnectProcessFinished(process.get());
-    endProcess();
+        endProcess(process.get());
         disconnectFromServer();
-    terminateProcess();
+        terminateProcess(process.get());
-    killProcess();
+        killProcess(process.get());
-
-    process_.reset();
 
         serverProxy_.resetCounter();
     }
+}
 
 bool ConnectionClient::waitForEcho()
 {
     return localSocket.waitForReadyRead();
 }
 
+bool ConnectionClient::waitForConnected()
+{
+    bool isConnected = false;
+
+    for (int counter = 0; counter < 100; counter++) {
+        isConnected = localSocket.waitForConnected(20);
+        if (isConnected)
+            return isConnected;
+        else {
+            QThread::msleep(30);
+            QCoreApplication::instance()->processEvents();
+        }
+    }
+
+    qWarning() << "Cannot connect:" << localSocket.errorString();
+
+    return isConnected;
+}
+
 ClangCodeModelServerProxy &ConnectionClient::serverProxy()
 {
     return serverProxy_;
@@ -278,37 +301,53 @@ bool ConnectionClient::isProcessIsRunning() const
     return process_ && process_->state() == QProcess::Running;
 }
 
-QProcess *ConnectionClient::process() const
+void ConnectionClient::connectProcessFinished(QProcess *process) const
 {
-    if (!process_)
+    connect(process,
-        process_.reset(new QProcess);
-
-    return process_.get();
-}
-
-void ConnectionClient::connectProcessFinished() const
-{
-    connect(process(),
             static_cast<void (QProcess::*)(int, QProcess::ExitStatus)>(&QProcess::finished),
             this,
-            &ConnectionClient::restartProcess);
+            &ConnectionClient::restartProcessAsynchronously);
 
 }
 
-void ConnectionClient::disconnectProcessFinished() const
+void ConnectionClient::connectProcessStarted(QProcess *process) const
 {
-    if (process_) {
+    connect(process,
-        disconnect(process_.get(),
+            &QProcess::started,
+            this,
+            &ConnectionClient::connectToLocalSocket);
+}
+
+void ConnectionClient::disconnectProcessFinished(QProcess *process) const
+{
+    if (process) {
+        disconnect(process,
                    static_cast<void (QProcess::*)(int, QProcess::ExitStatus)>(&QProcess::finished),
                    this,
-                   &ConnectionClient::restartProcess);
+                   &ConnectionClient::restartProcessAsynchronously);
     }
 }
 
-void ConnectionClient::connectStandardOutputAndError() const
+void ConnectionClient::connectStandardOutputAndError(QProcess *process) const
 {
-    connect(process(), &QProcess::readyReadStandardOutput, this, &ConnectionClient::printStandardOutput);
+    connect(process, &QProcess::readyReadStandardOutput, this, &ConnectionClient::printStandardOutput);
-    connect(process(), &QProcess::readyReadStandardError, this, &ConnectionClient::printStandardError);
+    connect(process, &QProcess::readyReadStandardError, this, &ConnectionClient::printStandardError);
+}
+
+void ConnectionClient::connectLocalSocketError() const
+{
+    connect(&localSocket,
+            static_cast<void (QLocalSocket::*)(QLocalSocket::LocalSocketError)>(&QLocalSocket::error),
+            this,
+            &ConnectionClient::printLocalSocketError);
+}
+
+void ConnectionClient::connectAliveTimer()
+{
+    connect(&processAliveTimer,
+            &QTimer::timeout,
+            this,
+            &ConnectionClient::restartProcessIfTimerIsNotResettedAndSocketIsEmpty);
 }
 
 const QString &ConnectionClient::processPath() const

src/libs/clangbackendipc/connectionclient.h

@@ -52,7 +52,7 @@ public:
     ConnectionClient(ClangCodeModelClientInterface *client);
     ~ConnectionClient();
 
-    bool connectToServer();
+    void startProcessAndConnectToServerAsynchronously();
     bool disconnectFromServer();
     bool isConnected() const;
 
@@ -64,34 +64,41 @@ public:
     const QString &processPath() const;
     void setProcessPath(const QString &processPath);
 
-    void startProcess();
+    void restartProcessAsynchronously();
-    void restartProcess();
     void restartProcessIfTimerIsNotResettedAndSocketIsEmpty();
     void finishProcess();
     bool isProcessIsRunning() const;
 
     bool waitForEcho();
+    bool waitForConnected();
 
     ClangCodeModelServerProxy &serverProxy();
 
     QProcess *processForTestOnly() const;
 
 signals:
-    void processRestarted();
+    void connectedToLocalSocket();
+    void processFinished();
 
 private:
-    bool connectToLocalSocket();
+    std::unique_ptr<QProcess> startProcess();
-    void endProcess();
+    void finishProcess(std::unique_ptr<QProcess> &&process);
-    void terminateProcess();
+    void connectToLocalSocket();
-    void killProcess();
+    void endProcess(QProcess *process);
+    void terminateProcess(QProcess *process);
+    void killProcess(QProcess *process);
+    void resetProcessIsStarting();
     void printLocalSocketError(QLocalSocket::LocalSocketError socketError);
     void printStandardOutput();
     void printStandardError();
 
-    QProcess *process() const;
+    void connectLocalSocketConnected();
-    void connectProcessFinished() const;
+    void connectProcessFinished(QProcess *process) const;
-    void disconnectProcessFinished() const;
+    void connectProcessStarted(QProcess *process) const;
-    void connectStandardOutputAndError() const;
+    void disconnectProcessFinished(QProcess *process) const;
+    void connectStandardOutputAndError(QProcess *process) const;
+    void connectLocalSocketError() const;
+    void connectAliveTimer();
 
     void ensureMessageIsWritten();
 
@@ -103,7 +110,8 @@ private:
     ClangCodeModelServerProxy serverProxy_;
     QTimer processAliveTimer;
     QString processPath_;
-    bool isAliveTimerResetted;
+    bool isAliveTimerResetted = false;
+    bool processIsStarting = false;
 
     LinePrefixer stdErrPrefixer;
     LinePrefixer stdOutPrefixer;

src/plugins/clangcodemodel/clangbackendipcintegration.cpp

@@ -331,12 +331,11 @@ void IpcCommunicator::initializeBackend()
     m_connection.setProcessAliveTimerInterval(30 * 1000);
     m_connection.setProcessPath(clangBackEndProcessPath);
 
-    connect(&m_connection, &ConnectionClient::processRestarted,
+    connect(&m_connection, &ConnectionClient::connectedToLocalSocket,
             this, &IpcCommunicator::onBackendRestarted);
 
     // TODO: Add a asynchron API to ConnectionClient, otherwise we might hang here
-    if (m_connection.connectToServer())
+    m_connection.startProcessAndConnectToServerAsynchronously();
-        initializeBackendWithCurrentData();
 }
 
 static QStringList projectPartOptions(const CppTools::ProjectPart::Ptr &projectPart)

tests/unit/unittest/clientserveroutsideprocess.cpp

@@ -60,6 +60,7 @@
 using namespace ClangBackEnd;
 
 using ::testing::Eq;
+using ::testing::SizeIs;
 
 class ClientServerOutsideProcess : public ::testing::Test
 {
@@ -77,30 +78,35 @@ protected:
 MockClangCodeModelClient ClientServerOutsideProcess::mockClangCodeModelClient;
 ClangBackEnd::ConnectionClient ClientServerOutsideProcess::client(&ClientServerOutsideProcess::mockClangCodeModelClient);
 
-TEST_F(ClientServerOutsideProcess, RestartProcess)
+TEST_F(ClientServerOutsideProcess, RestartProcessAsynchronously)
 {
-    client.restartProcess();
+    QSignalSpy clientSpy(&client, &ConnectionClient::connectedToLocalSocket);
 
+    client.restartProcessAsynchronously();
+
+    ASSERT_TRUE(clientSpy.wait(100000));
     ASSERT_TRUE(client.isProcessIsRunning());
     ASSERT_TRUE(client.isConnected());
 }
 
 TEST_F(ClientServerOutsideProcess, RestartProcessAfterAliveTimeout)
 {
-    QSignalSpy clientSpy(&client, SIGNAL(processRestarted()));
+    QSignalSpy clientSpy(&client, &ConnectionClient::connectedToLocalSocket);
 
     client.setProcessAliveTimerInterval(1);
 
     ASSERT_TRUE(clientSpy.wait(100000));
+    ASSERT_THAT(clientSpy, SizeIs(1));
 }
 
 TEST_F(ClientServerOutsideProcess, RestartProcessAfterTermination)
 {
-    QSignalSpy clientSpy(&client, SIGNAL(processRestarted()));
+    QSignalSpy clientSpy(&client, &ConnectionClient::connectedToLocalSocket);
 
     client.processForTestOnly()->kill();
 
     ASSERT_TRUE(clientSpy.wait(100000));
+    ASSERT_THAT(clientSpy, SizeIs(1));
 }
 
 TEST_F(ClientServerOutsideProcess, SendRegisterTranslationUnitForEditorMessage)
@@ -171,9 +177,13 @@ TEST_F(ClientServerOutsideProcess, SendUnregisterProjectPartsForEditorMessage)
 
 void ClientServerOutsideProcess::SetUpTestCase()
 {
+    QSignalSpy clientSpy(&client, &ConnectionClient::connectedToLocalSocket);
     client.setProcessPath(Utils::HostOsInfo::withExecutableSuffix(QStringLiteral(ECHOSERVER)));
 
-    ASSERT_TRUE(client.connectToServer());
+    client.startProcessAndConnectToServerAsynchronously();
+
+    ASSERT_TRUE(clientSpy.wait(100000));
+    ASSERT_THAT(clientSpy, SizeIs(1));
 }
 
 void ClientServerOutsideProcess::TearDownTestCase()
@@ -189,6 +199,9 @@ void ClientServerOutsideProcess::SetUp()
 
 void ClientServerOutsideProcess::TearDown()
 {
+    client.setProcessAliveTimerInterval(1000000);
+    client.waitForConnected();
+
     ASSERT_TRUE(client.isProcessIsRunning());
     ASSERT_TRUE(client.isConnected());
 }