Lock the build graph while jobs are running.

All jobs except setting up the project are mutually exclusive, and it is
also forbidden to change the project internally while they are going on.
Currently, ignoring this requirement leads to undefined behavior. Since
we can detect such a condition and react in a defined way, we should do
it.
Note: This is about the API and the in-memory build graph, not about
competing accesses to the stored build graph from different processes.
That's a different (and more difficult) problem.

Change-Id: I2d8a715334b2b73b4f4d32781d0b4f83b1612d40
Reviewed-by: Joerg Bornemann <joerg.bornemann@digia.com>

8 files changed, 61 insertions, 4 deletions

diff --git a/src/lib/api/internaljobs.cpp b/src/lib/api/internaljobs.cpp
index af4901692..4fb1a323c 100644
--- a/src/lib/api/internaljobs.cpp
+++ b/src/lib/api/internaljobs.cpp
@@ -53,6 +53,12 @@
 namespace qbs {
 namespace Internal {
 
+static void unlockBuildGraph(const TopLevelProjectPtr &project)
+{
+    QBS_ASSERT(project->locked, return);
+    project->locked = false;
+}
+
 class JobObserver : public ProgressObserver
 {
 public:
@@ -358,6 +364,7 @@ void InternalBuildJob::handleFinished()
 
 void InternalBuildJob::emitFinished()
 {
+    unlockBuildGraph(project());
     emit finished(this);
 }
 
@@ -383,6 +390,7 @@ void InternalCleanJob::start()
         setError(error);
     }
     storeBuildGraph();
+    unlockBuildGraph(project());
     emit finished(this);
 }
 
@@ -412,6 +420,7 @@ void InternalInstallJob::start()
     } catch (const ErrorInfo &error) {
         setError(error);
     }
+    unlockBuildGraph(m_project);
     emit finished(this);
 }
 
diff --git a/src/lib/api/internaljobs.h b/src/lib/api/internaljobs.h
index 08a451a40..eaeda238b 100644
--- a/src/lib/api/internaljobs.h
+++ b/src/lib/api/internaljobs.h
@@ -61,6 +61,7 @@ public:
 
     void cancel();
     ErrorInfo error() const { return m_error; }
+    void setError(const ErrorInfo &error) { m_error = error; }
 
     Logger logger() const { return m_logger; }
     bool timed() const { return m_timed; }
@@ -70,7 +71,6 @@ protected:
     explicit InternalJob(const Logger &logger, QObject *parent = 0);
 
     JobObserver *observer() const { return m_observer; }
-    void setError(const ErrorInfo &error) { m_error = error; }
     void setTimed(bool timed) { m_timed = timed; }
     void storeBuildGraph(const TopLevelProjectConstPtr &project);
 
diff --git a/src/lib/api/jobs.cpp b/src/lib/api/jobs.cpp
index 068a991e8..e2c17f5a4 100644
--- a/src/lib/api/jobs.cpp
+++ b/src/lib/api/jobs.cpp
@@ -33,6 +33,8 @@
 #include <language/language.h>
 #include <tools/qbsassert.h>
 
+#include <QMetaObject>
+
 namespace qbs {
 using namespace Internal;
 
@@ -113,6 +115,20 @@ AbstractJob::AbstractJob(InternalJob *internalJob, QObject *parent)
     m_state = StateRunning;
 }
 
+bool AbstractJob::lockBuildGraph(const TopLevelProjectPtr &project)
+{
+    // The API is not thread-safe, so we don't need a mutex here, as the API requests come in
+    // synchronously.
+    if (project->locked) {
+        internalJob()->setError(tr("Cannot start a job while another one is in process."));
+        QMetaObject::invokeMethod(this, "finished", Qt::QueuedConnection, Q_ARG(bool, false),
+                                  Q_ARG(qbs::AbstractJob *, this));
+        return false;
+    }
+    project->locked = true;
+    return true;
+}
+
 /*!
  * \brief Destroys the object, canceling the operation if necessary.
  */
@@ -254,6 +270,8 @@ BuildJob::BuildJob(const Logger &logger, QObject *parent)
 void BuildJob::build(const TopLevelProjectPtr &project, const QList<ResolvedProductPtr> &products,
                      const BuildOptions &options)
 {
+    if (!lockBuildGraph(project))
+        return;
     qobject_cast<InternalBuildJob *>(internalJob())->build(project, products, options);
 }
 
@@ -271,6 +289,8 @@ CleanJob::CleanJob(const Logger &logger, QObject *parent)
 void CleanJob::clean(const TopLevelProjectPtr &project, const QList<ResolvedProductPtr> &products,
                      const qbs::CleanOptions &options)
 {
+    if (!lockBuildGraph(project))
+        return;
     InternalJobThreadWrapper * wrapper = qobject_cast<InternalJobThreadWrapper *>(internalJob());
     qobject_cast<InternalCleanJob *>(wrapper->synchronousJob())->init(project, products, options);
     wrapper->start();
@@ -289,6 +309,8 @@ InstallJob::InstallJob(const Logger &logger, QObject *parent)
 void InstallJob::install(const TopLevelProjectPtr &project,
                          const QList<ResolvedProductPtr> &products, const InstallOptions &options)
 {
+    if (!lockBuildGraph(project))
+        return;
     InternalJobThreadWrapper *wrapper = qobject_cast<InternalJobThreadWrapper *>(internalJob());
     InternalInstallJob *installJob = qobject_cast<InternalInstallJob *>(wrapper->synchronousJob());
     installJob->init(project, products, options);
diff --git a/src/lib/api/jobs.h b/src/lib/api/jobs.h
index f1e85c46b..e5d5d6687 100644
--- a/src/lib/api/jobs.h
+++ b/src/lib/api/jobs.h
@@ -68,6 +68,8 @@ protected:
     AbstractJob(Internal::InternalJob *internalJob, QObject *parent);
     Internal::InternalJob *internalJob() const { return m_internalJob; }
 
+    bool lockBuildGraph(const Internal::TopLevelProjectPtr &project);
+
 signals:
     void taskStarted(const QString &description, int maximumProgressValue, qbs::AbstractJob *job);
     void totalEffortChanged(int totalEffort, qbs::AbstractJob *job);
diff --git a/src/lib/api/project.cpp b/src/lib/api/project.cpp
index 250c46eb4..66346bde5 100644
--- a/src/lib/api/project.cpp
+++ b/src/lib/api/project.cpp
@@ -147,6 +147,7 @@ public:
                                      const CodeLocation &changeLocation, int lineOffset);
     void updateExternalCodeLocations(const ProjectData &project,
                                      const CodeLocation &changeLocation, int lineOffset);
+    void prepareChangeToProject();
 
     const TopLevelProjectPtr internalProject;
     Logger logger;
@@ -587,6 +588,14 @@ void ProjectPrivate::updateExternalCodeLocations(const ProjectData &project,
     }
 }
 
+void ProjectPrivate::prepareChangeToProject()
+{
+    if (internalProject->locked)
+        throw ErrorInfo(Tr::tr("A job is currently in process."));
+    if (!m_projectDataRetrieved)
+        retrieveProjectData(m_projectData, internalProject);
+}
+
 void ProjectPrivate::retrieveProjectData(ProjectData &projectData,
                                          const ResolvedProjectConstPtr &internalProject)
 {
@@ -940,6 +949,7 @@ QSet<QString> Project::buildSystemFiles() const
 ErrorInfo Project::addGroup(const ProductData &product, const QString &groupName)
 {
     try {
+        d->prepareChangeToProject();
         d->addGroup(product, groupName);
         return ErrorInfo();
     } catch (ErrorInfo errorInfo) {
@@ -963,6 +973,7 @@ ErrorInfo Project::addFiles(const ProductData &product, const GroupData &group,
                             const QStringList &filePaths)
 {
     try {
+        d->prepareChangeToProject();
         d->addFiles(product, group, filePaths);
         return ErrorInfo();
     } catch (ErrorInfo errorInfo) {
@@ -985,6 +996,7 @@ ErrorInfo Project::removeFiles(const ProductData &product, const GroupData &grou
                                const QStringList &filePaths)
 {
     try {
+        d->prepareChangeToProject();
         d->removeFiles(product, group, filePaths);
         return ErrorInfo();
     } catch (ErrorInfo errorInfo) {
@@ -1002,6 +1014,7 @@ ErrorInfo Project::removeFiles(const ProductData &product, const GroupData &grou
 ErrorInfo Project::removeGroup(const ProductData &product, const GroupData &group)
 {
     try {
+        d->prepareChangeToProject();
         d->removeGroup(product, group);
         return ErrorInfo();
     } catch (ErrorInfo errorInfo) {
diff --git a/src/lib/language/language.cpp b/src/lib/language/language.cpp
index 39f6b0272..0530a78a6 100644
--- a/src/lib/language/language.cpp
+++ b/src/lib/language/language.cpp
@@ -789,7 +789,7 @@ void ResolvedProject::store(PersistentPool &pool) const
 }
 
 
-TopLevelProject::TopLevelProject()
+TopLevelProject::TopLevelProject() : locked(false)
 {
 }
 
diff --git a/src/lib/language/language.h b/src/lib/language/language.h
index 5bbeae7d5..183a79e44 100644
--- a/src/lib/language/language.h
+++ b/src/lib/language/language.h
@@ -428,6 +428,7 @@ public:
     QHash<QString, QString> usedEnvironment; // Environment variables requested by the project while resolving.
     QHash<QString, bool> fileExistsResults; // Results of calls to "File.exists()".
     QScopedPointer<ProjectBuildData> buildData;
+    bool locked;
 
     QSet<QString> buildSystemFiles;
 
diff --git a/tests/auto/api/tst_api.cpp b/tests/auto/api/tst_api.cpp
index a148f5052..61b1ebc83 100644
--- a/tests/auto/api/tst_api.cpp
+++ b/tests/auto/api/tst_api.cpp
@@ -295,7 +295,8 @@ void TestApi::changeContent()
     job.reset(qbs::Project::setupProject(setupParams, m_logSink, 0));
     waitForFinished(job.data());
     QVERIFY2(!job->error().hasError(), qPrintable(job->error().toString()));
-    const qbs::ProjectData newProjectData = job->project().projectData();
+    project = job->project();
+    const qbs::ProjectData newProjectData = project.projectData();
     const bool projectDataMatches = newProjectData == projectData;
     if (!projectDataMatches) {
         qDebug("This is the assumed project:");
@@ -306,13 +307,22 @@ void TestApi::changeContent()
     QVERIFY(projectDataMatches); // Will fail if e.g. code locations don't match.
 
     // Now try building again and check if the newly resolved product behaves the same way.
-    buildJob.reset(job->project().buildAllProducts(buildOptions, this));
+    buildJob.reset(project.buildAllProducts(buildOptions, this));
     connect(buildJob.data(), SIGNAL(reportCommandDescription(QString, QString)), &rcvr,
             SLOT(handleDescription(QString,QString)));
     waitForFinished(buildJob.data());
     QVERIFY2(!buildJob->error().hasError(), qPrintable(buildJob->error().toString()));
     QVERIFY(rcvr.descriptions.contains("compiling file.cpp"));
     QVERIFY(!rcvr.descriptions.contains("compiling main.cpp"));
+
+    // Error handling: Try to change the project during a build.
+    buildJob.reset(project.buildAllProducts(buildOptions, this));
+    errorInfo = project.addGroup(newProjectData.products().first(), "blubb");
+    QVERIFY(errorInfo.hasError());
+    QVERIFY2(errorInfo.toString().contains("in process"), qPrintable(errorInfo.toString()));
+    waitForFinished(buildJob.data());
+    errorInfo = project.addGroup(newProjectData.products().first(), "blubb");
+    QVERIFY2(!errorInfo.hasError(), qPrintable(errorInfo.toString()));
 }
 
 void TestApi::disabledInstallGroup()