From 2d7f1d6c8cf5c9be01d12bd71dbf05da6ce7c3d6 Mon Sep 17 00:00:00 2001 From: Nikolai Kosjar Date: Fri, 5 Jan 2018 09:35:00 +0100 Subject: [PATCH] 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 --- .../source/clangcodemodelserver.cpp | 16 +++++---- .../clangbackend/source/clangdocument.cpp | 6 ++++ src/tools/clangbackend/source/clangdocument.h | 1 + .../source/clangdocumentprocessor.cpp | 6 ++++ .../source/clangdocumentprocessor.h | 1 + .../source/clangdocumentprocessors.cpp | 21 +++++++++++ .../source/clangdocumentprocessors.h | 1 + .../clangbackend/source/clangjobrequest.cpp | 36 +++++++++++++++++++ .../clangbackend/source/clangjobrequest.h | 1 + src/tools/clangbackend/source/clangjobs.cpp | 19 ++++++++++ src/tools/clangbackend/source/clangjobs.h | 2 ++ ...ngsupportivetranslationunitinitializer.cpp | 31 +++++++++------- ...langsupportivetranslationunitinitializer.h | 3 +- .../unittest/clangcodemodelserver-test.cpp | 12 +++++++ .../unittest/clangdocumentprocessors-test.cpp | 13 +++++++ ...portivetranslationunitinitializer-test.cpp | 12 +++++++ 16 files changed, 162 insertions(+), 19 deletions(-) diff --git a/src/tools/clangbackend/source/clangcodemodelserver.cpp b/src/tools/clangbackend/source/clangcodemodelserver.cpp index d3e873d7323..7e004e17d03 100644 --- a/src/tools/clangbackend/source/clangcodemodelserver.cpp +++ b/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 affectedDocuments = documents.setDocumentsDirtyIfProjectPartChanged(); for (Document &document : affectedDocuments) { - document.setResponsivenessIncreaseNeeded(document.isResponsivenessIncreased()); + documents.remove({document.fileContainer()}); - documentProcessors().remove(document); - document.translationUnits().removeAll(); - document.translationUnits().createAndAppend(); - documentProcessors().create(document); + Document newDocument = *documents.create({document.fileContainer()}).begin(); + newDocument.setDirtyIfDependencyIsMet(document.filePath()); + newDocument.setIsUsedByCurrentEditor(document.isUsedByCurrentEditor()); + newDocument.setIsVisibleInEditor(document.isVisibleInEditor(), document.visibleTimePoint()); + newDocument.setResponsivenessIncreaseNeeded(document.isResponsivenessIncreased()); + + documentProcessors().reset(document, newDocument); + + QTC_CHECK(document.useCount() == 1); } processJobsForDirtyAndVisibleDocuments(); diff --git a/src/tools/clangbackend/source/clangdocument.cpp b/src/tools/clangbackend/source/clangdocument.cpp index cbbaf51b332..bef88e9cb28 100644 --- a/src/tools/clangbackend/source/clangdocument.cpp +++ b/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); diff --git a/src/tools/clangbackend/source/clangdocument.h b/src/tools/clangbackend/source/clangdocument.h index 415deefbe9d..92a852c820a 100644 --- a/src/tools/clangbackend/source/clangdocument.h +++ b/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; diff --git a/src/tools/clangbackend/source/clangdocumentprocessor.cpp b/src/tools/clangbackend/source/clangdocumentprocessor.cpp index 8196e804d0f..3c51db5b499 100644 --- a/src/tools/clangbackend/source/clangdocumentprocessor.cpp +++ b/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; diff --git a/src/tools/clangbackend/source/clangdocumentprocessor.h b/src/tools/clangbackend/source/clangdocumentprocessor.h index 9c3fb147f94..8f4e8051586 100644 --- a/src/tools/clangbackend/source/clangdocumentprocessor.h +++ b/src/tools/clangbackend/source/clangdocumentprocessor.h @@ -59,6 +59,7 @@ public: = PreferredTranslationUnit::RecentlyParsed); JobRequests process(); + JobRequests stop(); Document document() const; diff --git a/src/tools/clangbackend/source/clangdocumentprocessors.cpp b/src/tools/clangbackend/source/clangdocumentprocessors.cpp index 611611a41f5..ef17dddee0e 100644 --- a/src/tools/clangbackend/source/clangdocumentprocessors.cpp +++ b/src/tools/clangbackend/source/clangdocumentprocessors.cpp @@ -28,6 +28,8 @@ #include "clangexceptions.h" #include "projectpart.h" +#include + 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; diff --git a/src/tools/clangbackend/source/clangdocumentprocessors.h b/src/tools/clangbackend/source/clangdocumentprocessors.h index 83e37f52766..4060fd4b47a 100644 --- a/src/tools/clangbackend/source/clangdocumentprocessors.h +++ b/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(); diff --git a/src/tools/clangbackend/source/clangjobrequest.cpp b/src/tools/clangbackend/source/clangjobrequest.cpp index e2c6f8f6d49..2715ecde1c7 100644 --- a/src/tools/clangbackend/source/clangjobrequest.cpp +++ b/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; diff --git a/src/tools/clangbackend/source/clangjobrequest.h b/src/tools/clangbackend/source/clangjobrequest.h index 81c78ba9dbc..04474967537 100644 --- a/src/tools/clangbackend/source/clangjobrequest.h +++ b/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; diff --git a/src/tools/clangbackend/source/clangjobs.cpp b/src/tools/clangbackend/source/clangjobs.cpp index 872aecd936b..61f99b39852 100644 --- a/src/tools/clangbackend/source/clangjobs.cpp +++ b/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 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; diff --git a/src/tools/clangbackend/source/clangjobs.h b/src/tools/clangbackend/source/clangjobs.h index c2984db4b44..dd6b5d50810 100644 --- a/src/tools/clangbackend/source/clangjobs.h +++ b/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); diff --git a/src/tools/clangbackend/source/clangsupportivetranslationunitinitializer.cpp b/src/tools/clangbackend/source/clangsupportivetranslationunitinitializer.cpp index 9c162eb8cfa..b84fe431553 100644 --- a/src/tools/clangbackend/source/clangsupportivetranslationunitinitializer.cpp +++ b/src/tools/clangbackend/source/clangsupportivetranslationunitinitializer.cpp @@ -56,8 +56,7 @@ SupportiveTranslationUnitInitializer::State SupportiveTranslationUnitInitializer void SupportiveTranslationUnitInitializer::startInitializing() { - QTC_CHECK(m_state == State::NotInitialized); - if (abortIfDocumentIsClosed()) + if (!checkStateAndDocument(State::NotInitialized)) 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 (abortIfDocumentIsClosed()) + if (!checkStateAndDocument(State::WaitingForParseJob)) 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 (abortIfDocumentIsClosed()) + if (!checkStateAndDocument(State::WaitingForReparseJob)) 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_isDocumentClosedChecker(m_document.filePath(), m_document.projectPart().id())) { + if (m_state != currentExpectedState) { 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) diff --git a/src/tools/clangbackend/source/clangsupportivetranslationunitinitializer.h b/src/tools/clangbackend/source/clangsupportivetranslationunitinitializer.h index 18a01864ddc..785e145c60f 100644 --- a/src/tools/clangbackend/source/clangsupportivetranslationunitinitializer.h +++ b/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: diff --git a/tests/unit/unittest/clangcodemodelserver-test.cpp b/tests/unit/unittest/clangcodemodelserver-test.cpp index 36efb1597a8..edcbcff7ade 100644 --- a/tests/unit/unittest/clangcodemodelserver-test.cpp +++ b/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); diff --git a/tests/unit/unittest/clangdocumentprocessors-test.cpp b/tests/unit/unittest/clangdocumentprocessors-test.cpp index 87990bca114..0e16ab1f4e0 100644 --- a/tests/unit/unittest/clangdocumentprocessors-test.cpp +++ b/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), diff --git a/tests/unit/unittest/clangsupportivetranslationunitinitializer-test.cpp b/tests/unit/unittest/clangsupportivetranslationunitinitializer-test.cpp index 540fcf7fdc7..65062a9d165 100644 --- a/tests/unit/unittest/clangsupportivetranslationunitinitializer-test.cpp +++ b/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)});