Clang: Take over jobs if document's project changes

On registerProjectPartsForEditor() we recreated the affected
DocumentProcessors, but did not take care of the jobs that were in the
queue, which effectively could led to lost jobs. Catch up on this.

Task-number: QTCREATORBUG-18856
Change-Id: I4184e5dc6480667953f2d2081ccf39a28c092186
Reviewed-by: Ivan Donchevskii <ivan.donchevskii@qt.io>

src/tools/clangbackend/source/clangcodemodelserver.cpp

@@ -28,7 +28,6 @@
 #include "clangdocuments.h"
 #include "clangdocumentsuspenderresumer.h"
 #include "clangfilesystemwatcher.h"
-#include "clangtranslationunits.h"
 #include "codecompleter.h"
 #include "diagnosticset.h"
 #include "tokeninfos.h"
@@ -158,12 +157,17 @@ void ClangCodeModelServer::registerProjectPartsForEditor(const RegisterProjectPa
         std::vector<Document> affectedDocuments = documents.setDocumentsDirtyIfProjectPartChanged();
 
         for (Document &document : affectedDocuments) {
-            document.setResponsivenessIncreaseNeeded(document.isResponsivenessIncreased());
+            documents.remove({document.fileContainer()});
 
-            documentProcessors().remove(document);
+            Document newDocument = *documents.create({document.fileContainer()}).begin();
-            document.translationUnits().removeAll();
+            newDocument.setDirtyIfDependencyIsMet(document.filePath());
-            document.translationUnits().createAndAppend();
+            newDocument.setIsUsedByCurrentEditor(document.isUsedByCurrentEditor());
-            documentProcessors().create(document);
+            newDocument.setIsVisibleInEditor(document.isVisibleInEditor(), document.visibleTimePoint());
+            newDocument.setResponsivenessIncreaseNeeded(document.isResponsivenessIncreased());
+
+            documentProcessors().reset(document, newDocument);
+
+            QTC_CHECK(document.useCount() == 1);
         }
 
         processJobsForDirtyAndVisibleDocuments();

src/tools/clangbackend/source/clangdocument.cpp

@@ -155,6 +155,11 @@ bool Document::isParsed() const
     return d->translationUnits.areAllTranslationUnitsParsed();
 }
 
+long Document::useCount() const
+{
+    return d.use_count();
+}
+
 Utf8String Document::filePath() const
 {
     checkIfNull();
@@ -175,6 +180,7 @@ FileContainer Document::fileContainer() const
 
     return FileContainer(d->filePath,
                          d->projectPart.id(),
+                         d->fileArguments,
                          Utf8String(),
                          false,
                          d->documentRevision);

src/tools/clangbackend/source/clangdocument.h

@@ -78,6 +78,7 @@ public:
     bool isNull() const;
     bool isIntact() const;
     bool isParsed() const;
+    long useCount() const;
 
     Utf8String filePath() const;
     Utf8StringVector fileArguments() const;

src/tools/clangbackend/source/clangdocumentprocessor.cpp

@@ -102,6 +102,12 @@ JobRequests DocumentProcessor::process()
     return d->jobs.process();
 }
 
+JobRequests DocumentProcessor::stop()
+{
+    d->supportiveTranslationUnitInitializer.abort();
+    return d->jobs.stop();
+}
+
 Document DocumentProcessor::document() const
 {
     return d->document;

src/tools/clangbackend/source/clangdocumentprocessor.h

@@ -59,6 +59,7 @@ public:
                         = PreferredTranslationUnit::RecentlyParsed);
 
     JobRequests process();
+    JobRequests stop();
 
     Document document() const;
 

src/tools/clangbackend/source/clangdocumentprocessors.cpp

@@ -28,6 +28,8 @@
 #include "clangexceptions.h"
 #include "projectpart.h"
 
+#include <utils/algorithm.h>
+
 namespace ClangBackEnd {
 
 DocumentProcessors::DocumentProcessors(Documents &documents,
@@ -84,6 +86,25 @@ void DocumentProcessors::remove(const Document &document)
         throw DocumentProcessorDoesNotExist(document.filePath(), document.projectPart().id());
 }
 
+void DocumentProcessors::reset(const Document &oldDocument, const Document &newDocument)
+{
+    // Wait until the currently running jobs finish and remember the not yet
+    // processed job requests for the new processor...
+    JobRequests jobsToTakeOver = processor(oldDocument).stop();
+    // ...but do not take over irrelevant ones.
+    jobsToTakeOver = Utils::filtered(jobsToTakeOver, [](const JobRequest &job){
+        return job.isTakeOverable();
+    });
+
+    // Remove current processor
+    remove(oldDocument);
+
+    // Create new processor and take over not yet processed jobs.
+    DocumentProcessor newProcessor = create(newDocument);
+    for (const JobRequest &job : jobsToTakeOver)
+        newProcessor.addJob(job);
+}
+
 JobRequests DocumentProcessors::process()
 {
     JobRequests jobsStarted;

src/tools/clangbackend/source/clangdocumentprocessors.h

@@ -54,6 +54,7 @@ public:
     DocumentProcessor create(const Document &document);
     DocumentProcessor processor(const Document &document);
     void remove(const Document &document);
+    void reset(const Document &oldDocument, const Document &newDocument);
 
     JobRequests process();
 

src/tools/clangbackend/source/clangjobrequest.cpp

@@ -168,6 +168,42 @@ static JobRequest::RunConditions conditionsForType(JobRequest::Type type)
     return conditions;
 }
 
+bool JobRequest::isTakeOverable() const
+{
+    // When new project information comes in and there are unprocessed jobs
+    // in the queue, we need to decide what to do with them.
+
+    switch (type) {
+    // Never discard these as the client side might wait for a response.
+    case Type::CompleteCode:
+    case Type::RequestReferences:
+    case Type::FollowSymbol:
+        return true;
+
+    // Discard this one as UpdateDocumentAnnotations will have the same effect.
+    case Type::RequestDocumentAnnotations:
+
+    // Discard Suspend because the document will be cleared anyway.
+    // Discard Resume because a (re)parse will happen on demand.
+    case Type::SuspendDocument:
+    case Type::ResumeDocument:
+
+    // Discard these as they are initial jobs that will be recreated on demand
+    // anyway.
+    case Type::UpdateDocumentAnnotations:
+    case Type::CreateInitialDocumentPreamble:
+
+    // Discard these as they only make sense in a row. Avoid splitting them up.
+    case Type::ParseSupportiveTranslationUnit:
+    case Type::ReparseSupportiveTranslationUnit:
+
+    case Type::Invalid:
+        return false;
+    }
+
+    return false;
+}
+
 JobRequest::JobRequest(Type type)
 {
     static quint64 idCounter = 0;

src/tools/clangbackend/source/clangjobrequest.h

@@ -96,6 +96,7 @@ public:
 
     IAsyncJob *createJob() const;
     void cancelJob(ClangCodeModelClientInterface &client) const;
+    bool isTakeOverable() const;
 
     bool operator==(const JobRequest &other) const;
 

src/tools/clangbackend/source/clangjobs.cpp

@@ -113,6 +113,20 @@ JobRequests Jobs::process()
     return jobsStarted;
 }
 
+JobRequests Jobs::stop()
+{
+    // Take the queued jobs to prevent processing them.
+    const JobRequests queuedJobs = queue();
+    queue().clear();
+
+    // Wait until currently running jobs finish.
+    QFutureSynchronizer<void> waitForFinishedJobs;
+    foreach (const RunningJob &runningJob, m_running.values())
+        waitForFinishedJobs.addFuture(runningJob.future);
+
+    return queuedJobs;
+}
+
 JobRequests Jobs::runJobs(const JobRequests &jobsRequests)
 {
     JobRequests jobsStarted;
@@ -166,6 +180,11 @@ void Jobs::onJobFinished(IAsyncJob *asyncJob)
     process();
 }
 
+Jobs::JobFinishedCallback Jobs::jobFinishedCallback() const
+{
+    return m_jobFinishedCallback;
+}
+
 void Jobs::setJobFinishedCallback(const JobFinishedCallback &jobFinishedCallback)
 {
     m_jobFinishedCallback = jobFinishedCallback;

src/tools/clangbackend/source/clangjobs.h

@@ -71,6 +71,7 @@ public:
                  = PreferredTranslationUnit::RecentlyParsed);
 
     JobRequests process();
+    JobRequests stop();
 
     void setJobFinishedCallback(const JobFinishedCallback &jobFinishedCallback);
 
@@ -80,6 +81,7 @@ public /*for tests*/:
     const JobRequests &queue() const;
     bool isJobRunningForTranslationUnit(const Utf8String &translationUnitId) const;
     bool isJobRunningForJobRequest(const JobRequest &jobRequest) const;
+    JobFinishedCallback jobFinishedCallback() const;
 
 private:
     JobRequests runJobs(const JobRequests &jobRequest);

src/tools/clangbackend/source/clangsupportivetranslationunitinitializer.cpp

@@ -56,8 +56,7 @@ SupportiveTranslationUnitInitializer::State SupportiveTranslationUnitInitializer
 
 void SupportiveTranslationUnitInitializer::startInitializing()
 {
-    QTC_CHECK(m_state == State::NotInitialized);
+    if (!checkStateAndDocument(State::NotInitialized))
-    if (abortIfDocumentIsClosed())
         return;
 
     m_document.translationUnits().createAndAppend();
@@ -71,10 +70,15 @@ void SupportiveTranslationUnitInitializer::startInitializing()
     m_state = State::WaitingForParseJob;
 }
 
+void SupportiveTranslationUnitInitializer::abort()
+{
+    m_jobs.setJobFinishedCallback(Jobs::JobFinishedCallback());
+    m_state = State::Aborted;
+}
+
 void SupportiveTranslationUnitInitializer::checkIfParseJobFinished(const Jobs::RunningJob &job)
 {
-    QTC_CHECK(m_state == State::WaitingForParseJob);
+    if (!checkStateAndDocument(State::WaitingForParseJob))
-    if (abortIfDocumentIsClosed())
         return;
 
     if (job.jobRequest.type == JobRequest::Type::ParseSupportiveTranslationUnit) {
@@ -90,8 +94,7 @@ void SupportiveTranslationUnitInitializer::checkIfParseJobFinished(const Jobs::R
 
 void SupportiveTranslationUnitInitializer::checkIfReparseJobFinished(const Jobs::RunningJob &job)
 {
-    QTC_CHECK(m_state == State::WaitingForReparseJob);
+    if (!checkStateAndDocument(State::WaitingForReparseJob))
-    if (abortIfDocumentIsClosed())
         return;
 
     if (job.jobRequest.type == JobRequest::Type::ReparseSupportiveTranslationUnit) {
@@ -106,16 +109,20 @@ void SupportiveTranslationUnitInitializer::checkIfReparseJobFinished(const Jobs:
     }
 }
 
-bool SupportiveTranslationUnitInitializer::abortIfDocumentIsClosed()
+bool SupportiveTranslationUnitInitializer::checkStateAndDocument(State currentExpectedState)
 {
-    QTC_CHECK(m_isDocumentClosedChecker);
+    if (m_state != currentExpectedState) {
-
-    if (m_isDocumentClosedChecker(m_document.filePath(), m_document.projectPart().id())) {
         m_state = State::Aborted;
-        return true;
+        return false;
     }
 
-    return false;
+    QTC_CHECK(m_isDocumentClosedChecker);
+    if (m_isDocumentClosedChecker(m_document.filePath(), m_document.projectPart().id())) {
+        m_state = State::Aborted;
+        return false;
+    }
+
+    return true;
 }
 
 void SupportiveTranslationUnitInitializer::addJob(JobRequest::Type jobRequestType)

src/tools/clangbackend/source/clangsupportivetranslationunitinitializer.h

@@ -52,6 +52,7 @@ public:
 
     State state() const;
     void startInitializing();
+    void abort();
 
 public: // for tests
     void setState(const State &state);
@@ -59,7 +60,7 @@ public: // for tests
     void checkIfReparseJobFinished(const Jobs::RunningJob &job);
 
 private:
-    bool abortIfDocumentIsClosed();
+    bool checkStateAndDocument(State currentExpectedState);
     void addJob(JobRequest::Type jobRequestType);
 
 private:

tests/unit/unittest/clangcodemodelserver-test.cpp

@@ -406,6 +406,18 @@ TEST_F(ClangCodeModelServerSlowTest, TranslationUnitAfterUpdateNeedsReparse)
     ASSERT_THAT(clangServer, HasDirtyDocument(filePathA, projectPartId, 1U, true, true));
 }
 
+TEST_F(ClangCodeModelServerSlowTest, TakeOverJobsOnProjectPartChange)
+{
+    registerProjectAndFileAndWaitForFinished(filePathC, 2);
+    updateVisibilty(filePathB, filePathB); // Disable processing jobs
+    requestReferences();
+
+    expectReferences();
+
+    changeProjectPartArguments(); // Here we do not want to loose the RequestReferences job
+    updateVisibilty(filePathC, filePathC); // Enable processing jobs
+}
+
 void ClangCodeModelServer::SetUp()
 {
     clangServer.setClient(&mockClangCodeModelClient);

tests/unit/unittest/clangdocumentprocessors-test.cpp

@@ -126,6 +126,19 @@ TEST_F(DocumentProcessors, Remove)
     ASSERT_TRUE(documentProcessors.processors().empty());
 }
 
+TEST_F(DocumentProcessors, ResetTakesOverJobsInQueue)
+{
+    documentProcessors.create(document);
+    documentProcessors.processor(document).addJob(JobRequest::Type::RequestReferences);
+    documents.remove({document.fileContainer()});
+    const auto newDocument = *documents.create({document.fileContainer()}).begin();
+
+    documentProcessors.reset(document, newDocument);
+
+    ASSERT_THAT(documentProcessors.processor(document).queue().first().type,
+                JobRequest::Type::RequestReferences);
+}
+
 TEST_F(DocumentProcessors, RemoveThrowsForNotExisting)
 {
     ASSERT_THROW(documentProcessors.remove(document),

tests/unit/unittest/clangsupportivetranslationunitinitializer-test.cpp

@@ -111,6 +111,18 @@ TEST_F(SupportiveTranslationUnitInitializerSlowTest, StartInitializingStartsJob)
     ASSERT_THAT(runningJob.jobRequest.type, JobRequest::Type::ParseSupportiveTranslationUnit);
 }
 
+TEST_F(SupportiveTranslationUnitInitializerSlowTest, Abort)
+{
+    initializer.startInitializing();
+    assertSingleJobRunningAndEmptyQueue();
+
+    initializer.abort();
+
+    ASSERT_THAT(initializer.state(),
+                Eq(ClangBackEnd::SupportiveTranslationUnitInitializer::State::Aborted));
+    ASSERT_FALSE(jobs.jobFinishedCallback());
+}
+
 TEST_F(SupportiveTranslationUnitInitializer, CheckIfParseJobFinishedAbortsIfDocumentIsClosed)
 {
     documents.remove({FileContainer(filePath, projectPartId)});