Fix parallel installations

We lost that feature in a refactoring a few years ago, but nobody noticed
until now. We can now have multiple app installations in the wait-for-
acknowledge state, while downloading and unpacking of other installations
can still happen in parallel.

Change-Id: I6d33aa7df796e91a0bb595dd79c49f6b9d6084d3
Fixes: AUTOSUITE-630
Reviewed-by: Dominik Holland <dominik.holland@pelagicore.com>

5 files changed, 81 insertions, 26 deletions

diff --git a/src/installer-lib/applicationinstaller.cpp b/src/installer-lib/applicationinstaller.cpp
index 54d0d7a9..cc605be9 100644
--- a/src/installer-lib/applicationinstaller.cpp
+++ b/src/installer-lib/applicationinstaller.cpp
@@ -761,10 +761,9 @@ void ApplicationInstaller::acknowledgePackageInstallation(const QString &taskId)
 {
     AM_TRACE(LogInstaller, taskId)
 
-    auto allTasks = d->taskQueue;
-    allTasks.append(d->activeTask);
+    const auto allTasks = d->allTasks();
 
-    for (AsynchronousTask *task : qAsConst(allTasks)) {
+    for (AsynchronousTask *task : allTasks) {
         if (qobject_cast<InstallationTask *>(task) && (task->id() == taskId)) {
             static_cast<InstallationTask *>(task)->acknowledge();
             break;
@@ -815,10 +814,9 @@ QString ApplicationInstaller::removePackage(const QString &id, bool keepDocument
 */
 AsynchronousTask::TaskState ApplicationInstaller::taskState(const QString &taskId) const
 {
-    auto allTasks = d->taskQueue;
-    allTasks.append(d->activeTask);
+    const auto allTasks = d->allTasks();
 
-    for (const AsynchronousTask *task : qAsConst(allTasks)) {
+    for (const AsynchronousTask *task : allTasks) {
         if (task && (task->id() == taskId))
             return task->state();
     }
@@ -837,10 +835,9 @@ AsynchronousTask::TaskState ApplicationInstaller::taskState(const QString &taskI
 */
 QString ApplicationInstaller::taskApplicationId(const QString &taskId) const
 {
-    auto allTasks = d->taskQueue;
-    allTasks.append(d->activeTask);
+    const auto allTasks = d->allTasks();
 
-    for (const AsynchronousTask *task : qAsConst(allTasks)) {
+    for (const AsynchronousTask *task : allTasks) {
         if (task && (task->id() == taskId))
             return task->applicationId();
     }
@@ -850,15 +847,15 @@ QString ApplicationInstaller::taskApplicationId(const QString &taskId) const
 /*!
     \qmlmethod list<string> ApplicationInstaller::activeTaskIds()
 
-    Retuns a list of all currently active installation task ids.
+    Retuns a list of all currently active (as in not yet finished or failed) installation task ids.
 */
 QStringList ApplicationInstaller::activeTaskIds() const
 {
+    const auto allTasks = d->allTasks();
+
     QStringList result;
-    for (const AsynchronousTask *task : qAsConst(d->taskQueue))
+    for (const AsynchronousTask *task : allTasks)
         result << task->id();
-    if (d->activeTask)
-        result << d->activeTask->id();
     return result;
 }
 
@@ -873,21 +870,29 @@ bool ApplicationInstaller::cancelTask(const QString &taskId)
 {
     AM_TRACE(LogInstaller, taskId)
 
-    if (d->activeTask && d->activeTask->id() == taskId)
-        return d->activeTask->cancel();
-
-    for (AsynchronousTask *task : qAsConst(d->taskQueue)) {
+    // incoming tasks can be forcefully cancelled right away
+    for (AsynchronousTask *task : qAsConst(d->incomingTaskList)) {
         if (task->id() == taskId) {
             task->forceCancel();
             task->deleteLater();
 
             handleFailure(task);
 
-            d->taskQueue.removeOne(task);
+            d->incomingTaskList.removeOne(task);
             triggerExecuteNextTask();
             return true;
         }
     }
+
+    // the active task and async tasks might be in a state where cancellation is not possible,
+    // so we have to ask them nicely
+    if (d->activeTask && d->activeTask->id() == taskId)
+        return d->activeTask->cancel();
+
+    for (AsynchronousTask *task : qAsConst(d->installationTaskList)) {
+        if (task->id() == taskId)
+            return task->cancel();
+    }
     return false;
 }
 
@@ -1174,7 +1179,7 @@ bool ApplicationInstaller::deactivatePackage(const QString &id)
 
 QString ApplicationInstaller::enqueueTask(AsynchronousTask *task)
 {
-    d->taskQueue.enqueue(task);
+    d->incomingTaskList.append(task);
     triggerExecuteNextTask();
     return task->id();
 }
@@ -1187,10 +1192,10 @@ void ApplicationInstaller::triggerExecuteNextTask()
 
 void ApplicationInstaller::executeNextTask()
 {
-    if (d->activeTask || d->taskQueue.isEmpty())
+    if (d->activeTask || d->incomingTaskList.isEmpty())
         return;
 
-    AsynchronousTask *task = d->taskQueue.dequeue();
+    AsynchronousTask *task = d->incomingTaskList.takeFirst();
 
     if (task->hasFailed()) {
         task->setState(AsynchronousTask::Failed);
@@ -1231,8 +1236,8 @@ void ApplicationInstaller::executeNextTask()
 
         if (d->activeTask == task)
             d->activeTask = nullptr;
+        d->installationTaskList.removeOne(task);
 
-        //task->deleteLater();
         delete task;
         triggerExecuteNextTask();
     });
@@ -1241,6 +1246,14 @@ void ApplicationInstaller::executeNextTask()
         connect(static_cast<InstallationTask *>(task), &InstallationTask::finishedPackageExtraction, this, [this, task]() {
             qCDebug(LogInstaller) << "emit blockingUntilInstallationAcknowledge" << task->id();
             emit taskBlockingUntilInstallationAcknowledge(task->id());
+
+            // we can now start the next download in parallel - the InstallationTask will take care
+            // of serializing the final installation steps on its own as soon as it gets the
+            // required acknowledge (or cancel).
+            if (d->activeTask == task)
+                d->activeTask = nullptr;
+            d->installationTaskList.append(task);
+            triggerExecuteNextTask();
         });
     }
 
diff --git a/src/installer-lib/applicationinstaller_p.h b/src/installer-lib/applicationinstaller_p.h
index 4e0bf357..0d96b56a 100644
--- a/src/installer-lib/applicationinstaller_p.h
+++ b/src/installer-lib/applicationinstaller_p.h
@@ -42,7 +42,7 @@
 #pragma once
 
 #include <QMutex>
-#include <QQueue>
+#include <QList>
 #include <QSet>
 #include <QScopedPointer>
 #include <QThread>
@@ -75,8 +75,19 @@ public:
     QString hardwareId;
     QList<QByteArray> chainOfTrust;
 
-    QQueue<AsynchronousTask *> taskQueue;
-    AsynchronousTask *activeTask = nullptr;
+    QList<AsynchronousTask *> incomingTaskList;     // incoming queue
+    QList<AsynchronousTask *> installationTaskList; // installation jobs in state >= AwaitingAcknowledge
+    AsynchronousTask *activeTask = nullptr;         // currently active
+
+    QList<AsynchronousTask *> allTasks() const
+    {
+        QList<AsynchronousTask *> all = incomingTaskList;
+        if (!installationTaskList.isEmpty())
+            all += installationTaskList;
+        if (activeTask)
+            all += activeTask;
+        return all;
+    }
 
     QMutex activationLock;
     QMap<QString, QString> activatedPackages; // id -> installationPath
diff --git a/src/installer-lib/installationtask.cpp b/src/installer-lib/installationtask.cpp
index 758d5a40..e96f06a7 100644
--- a/src/installer-lib/installationtask.cpp
+++ b/src/installer-lib/installationtask.cpp
@@ -148,6 +148,9 @@ private:
     Q_DISABLE_COPY(TemporaryDir)
 };
 
+
+QMutex InstallationTask::s_serializeFinishInstallation { };
+
 InstallationTask::InstallationTask(const InstallationLocation &installationLocation, const QUrl &sourceUrl, QObject *parent)
     : AsynchronousTask(parent)
     , m_ai(ApplicationInstaller::instance())
@@ -260,7 +263,9 @@ void InstallationTask::execute()
 
         setState(Installing);
 
-        // if we would allow parallel installations - this would be the place to serialize them
+        // However many downloads are allowed to happen in parallel: we need to serialize those
+        // tasks here for the finishInstallation() step
+        QMutexLocker finishLocker(&s_serializeFinishInstallation);
 
         finishInstallation();
 
diff --git a/src/installer-lib/installationtask.h b/src/installer-lib/installationtask.h
index 10b77ff8..abba24b4 100644
--- a/src/installer-lib/installationtask.h
+++ b/src/installer-lib/installationtask.h
@@ -98,6 +98,8 @@ private:
     bool m_installationAcknowledged = false;
     QWaitCondition m_installationAcknowledgeWaitCondition;
 
+    static QMutex s_serializeFinishInstallation;
+
     QDir m_manifestDir;
     QDir m_applicationDir;
     QDir m_extractionDir;
diff --git a/tests/applicationinstaller/tst_applicationinstaller.cpp b/tests/applicationinstaller/tst_applicationinstaller.cpp
index 879e9d85..62fefa8b 100644
--- a/tests/applicationinstaller/tst_applicationinstaller.cpp
+++ b/tests/applicationinstaller/tst_applicationinstaller.cpp
@@ -129,6 +129,8 @@ private slots:
     void cancelPackageInstallation_data();
     void cancelPackageInstallation();
 
+    void parallelPackageInstallation();
+
     void validateDnsName_data();
     void validateDnsName();
 
@@ -1036,6 +1038,28 @@ void tst_ApplicationInstaller::cancelPackageInstallation()
         QVERIFY(m_finishedSpy->wait(spyTimeout));
         QCOMPARE(m_finishedSpy->first()[0].toString(), taskId);
     }
+    clearSignalSpies();
+}
+
+void tst_ApplicationInstaller::parallelPackageInstallation()
+{
+    QString task1Id = m_ai->startPackageInstallation("internal-0", QUrl::fromLocalFile(AM_TESTDATA_DIR "packages/test-dev-signed.appkg"));
+    QVERIFY(!task1Id.isEmpty());
+    QVERIFY(m_blockingUntilInstallationAcknowledgeSpy->wait(spyTimeout));
+    QCOMPARE(m_blockingUntilInstallationAcknowledgeSpy->first()[0].toString(), task1Id);
+
+    QString task2Id = m_ai->startPackageInstallation("internal-0", QUrl::fromLocalFile(AM_TESTDATA_DIR "packages/bigtest-dev-signed.appkg"));
+    QVERIFY(!task2Id.isEmpty());
+    m_ai->acknowledgePackageInstallation(task2Id);
+    QVERIFY(m_finishedSpy->wait(spyTimeout));
+    QCOMPARE(m_finishedSpy->first()[0].toString(), task2Id);
+
+    clearSignalSpies();
+    m_ai->acknowledgePackageInstallation(task1Id);
+    QVERIFY(m_finishedSpy->wait(spyTimeout));
+    QCOMPARE(m_finishedSpy->first()[0].toString(), task1Id);
+
+    clearSignalSpies();
 }