From f2e5e220090a88d6d65bc1d417faae99a497a1db Mon Sep 17 00:00:00 2001 From: Nikolai Kosjar Date: Thu, 7 Sep 2017 16:28:44 +0200 Subject: Clang: Avoid waiting plugin side if (re)parse fails If a parse or reparse operation fails, the corresponding translation unit is "not intact" anymore and further job requests for that translation unit will be silently discarded without notifying the plugin. This is especially problematic when the plugin expects/waits (for) a certain message. A parse/reparse can fail due to e.g. an invalid commandline or an internal crash. In such a case, ensure to send some dummy results. Task-number: QTCREATORBUG-18864 Change-Id: Ida9b8066fd55e64027a9b0d8bd5cf365b16a7356 Reviewed-by: Ivan Donchevskii Reviewed-by: Tim Jenssen --- src/tools/clangbackend/ipcsource/clangjobqueue.cpp | 29 ++++++++++++- src/tools/clangbackend/ipcsource/clangjobqueue.h | 7 +++- src/tools/clangbackend/ipcsource/clangjobs.cpp | 31 ++++++++++++++ src/tools/clangbackend/ipcsource/clangjobs.h | 1 + tests/unit/unittest/clangjobqueue-test.cpp | 49 ++++++++++++++++++++++ 5 files changed, 114 insertions(+), 3 deletions(-) diff --git a/src/tools/clangbackend/ipcsource/clangjobqueue.cpp b/src/tools/clangbackend/ipcsource/clangjobqueue.cpp index 8c9ace68e6..383c2ecaa4 100644 --- a/src/tools/clangbackend/ipcsource/clangjobqueue.cpp +++ b/src/tools/clangbackend/ipcsource/clangjobqueue.cpp @@ -53,6 +53,19 @@ bool JobQueue::add(const JobRequest &job) return false; } + if (!m_documents.hasDocument(job.filePath, job.projectPartId)) { + qCDebug(jobsLog) << "Not adding / cancelling due to already closed document:" << job; + cancelJobRequest(job); + return false; + } + + const Document document = m_documents.document(job.filePath, job.projectPartId); + if (!document.isIntact()) { + qCDebug(jobsLog) << "Not adding / cancelling due not intact document:" << job; + cancelJobRequest(job); + return false; + } + qCDebug(jobsLog) << "Adding" << job; m_queue.append(job); @@ -90,7 +103,7 @@ void JobQueue::removeExpiredRequests() m_queue = cleanedRequests; } -bool JobQueue::isJobRequestExpired(const JobRequest &jobRequest) const +bool JobQueue::isJobRequestExpired(const JobRequest &jobRequest) { const JobRequest::ExpirationReasons expirationReasons = jobRequest.expirationReasons; const UnsavedFiles unsavedFiles = m_documents.unsavedFiles(); @@ -120,7 +133,8 @@ bool JobQueue::isJobRequestExpired(const JobRequest &jobRequest) const const Document document = m_documents.document(jobRequest.filePath, jobRequest.projectPartId); if (!document.isIntact()) { - qCDebug(jobsLog) << "Removing due to not intact translation unit:" << jobRequest; + qCDebug(jobsLog) << "Removing/Cancelling due to not intact document:" << jobRequest; + cancelJobRequest(jobRequest); return true; } @@ -174,6 +188,12 @@ void JobQueue::prioritizeRequests() std::stable_sort(m_queue.begin(), m_queue.end(), lessThan); } +void JobQueue::cancelJobRequest(const JobRequest &jobRequest) +{ + if (m_cancelJobRequest) + m_cancelJobRequest(jobRequest); +} + static bool passesPreconditions(const JobRequest &request, const Document &document) { using Condition = JobRequest::Condition; @@ -280,6 +300,11 @@ void JobQueue::setIsJobRunningForJobRequestHandler( m_isJobRunningForJobRequestHandler = isJobRunningHandler; } +void JobQueue::setCancelJobRequest(const JobQueue::CancelJobRequest &cancelJobRequest) +{ + m_cancelJobRequest = cancelJobRequest; +} + JobRequests &JobQueue::queue() { return m_queue; diff --git a/src/tools/clangbackend/ipcsource/clangjobqueue.h b/src/tools/clangbackend/ipcsource/clangjobqueue.h index 736ff3f2a3..0d9c49ef04 100644 --- a/src/tools/clangbackend/ipcsource/clangjobqueue.h +++ b/src/tools/clangbackend/ipcsource/clangjobqueue.h @@ -51,17 +51,21 @@ public: void setIsJobRunningForJobRequestHandler( const IsJobRunningForJobRequestHandler &isJobRunningHandler); + using CancelJobRequest = std::function; + void setCancelJobRequest(const CancelJobRequest &cancelJobRequest); + public: // for tests JobRequests &queue(); int size() const; void prioritizeRequests(); private: + void cancelJobRequest(const JobRequest &jobRequest); bool isJobRunningForTranslationUnit(const Utf8String &translationUnitId); bool isJobRunningForJobRequest(const JobRequest &jobRequest); JobRequests takeJobRequestsToRunNow(); void removeExpiredRequests(); - bool isJobRequestExpired(const JobRequest &jobRequest) const; + bool isJobRequestExpired(const JobRequest &jobRequest); private: Documents &m_documents; @@ -69,6 +73,7 @@ private: IsJobRunningForTranslationUnitHandler m_isJobRunningForTranslationUnitHandler; IsJobRunningForJobRequestHandler m_isJobRunningForJobRequestHandler; + CancelJobRequest m_cancelJobRequest; JobRequests m_queue; }; diff --git a/src/tools/clangbackend/ipcsource/clangjobs.cpp b/src/tools/clangbackend/ipcsource/clangjobs.cpp index dfe619546c..f168387505 100644 --- a/src/tools/clangbackend/ipcsource/clangjobs.cpp +++ b/src/tools/clangbackend/ipcsource/clangjobs.cpp @@ -29,6 +29,9 @@ #include "clangiasyncjob.h" #include "projects.h" +#include +#include + #include #include #include @@ -54,6 +57,9 @@ Jobs::Jobs(Documents &documents, m_queue.setIsJobRunningForJobRequestHandler([this](const JobRequest &jobRequest) { return isJobRunningForJobRequest(jobRequest); }); + m_queue.setCancelJobRequest([this](const JobRequest &jobRequest) { + return cancelJobRequest(jobRequest); + }); } Jobs::~Jobs() @@ -197,4 +203,29 @@ bool Jobs::isJobRunningForJobRequest(const JobRequest &jobRequest) const return Utils::anyOf(m_running.values(), hasJobRequest); } +void Jobs::cancelJobRequest(const JobRequest &jobRequest) +{ + // TODO: Consider to refactor this. Jobs should not know anything about + // concrete messages. On the other hand, having this here avoids + // duplication in multiple job classes. + + // If a job request with a ticket number is cancelled, the plugin side + // must get back some results in order to clean up the state there. + switch (jobRequest.type) { + case JobRequest::Type::RequestReferences: + m_client.references(ReferencesMessage(FileContainer(), + QVector(), + false, + jobRequest.ticketNumber)); + break; + case JobRequest::Type::CompleteCode: + m_client.codeCompleted(CodeCompletedMessage(CodeCompletions(), + CompletionCorrection::NoCorrection, + jobRequest.ticketNumber)); + break; + default: + break; + } +} + } // namespace ClangBackEnd diff --git a/src/tools/clangbackend/ipcsource/clangjobs.h b/src/tools/clangbackend/ipcsource/clangjobs.h index c4167110f5..711f15e67a 100644 --- a/src/tools/clangbackend/ipcsource/clangjobs.h +++ b/src/tools/clangbackend/ipcsource/clangjobs.h @@ -80,6 +80,7 @@ public /*for tests*/: bool isJobRunningForJobRequest(const JobRequest &jobRequest) const; private: + void cancelJobRequest(const JobRequest &jobRequest); JobRequests runJobs(const JobRequests &jobRequest); bool runJob(const JobRequest &jobRequest); void onJobFinished(IAsyncJob *asyncJob); diff --git a/tests/unit/unittest/clangjobqueue-test.cpp b/tests/unit/unittest/clangjobqueue-test.cpp index ea786e87f1..c3236a4d6d 100644 --- a/tests/unit/unittest/clangjobqueue-test.cpp +++ b/tests/unit/unittest/clangjobqueue-test.cpp @@ -118,6 +118,41 @@ TEST_F(JobQueue, DoNotAddDuplicateForWhichAJobIsAlreadyRunning) ASSERT_FALSE(added); } +TEST_F(JobQueue, DoNotAddForNotExistingDocument) +{ + jobQueue.setCancelJobRequest([](const JobRequest &) { + return true; + }); + + const bool added = jobQueue.add(createJobRequest(Utf8StringLiteral("notExistingDocument.cpp"), + JobRequest::Type::UpdateDocumentAnnotations)); + + ASSERT_FALSE(added); +} + +TEST_F(JobQueue, DoNotAddForNotIntactDocument) +{ + document.setHasParseOrReparseFailed(true); + const bool added = jobQueue.add(createJobRequest(filePath1, + JobRequest::Type::UpdateDocumentAnnotations)); + + ASSERT_FALSE(added); +} + +TEST_F(JobQueue, CancelDuringAddForNotIntactDocument) +{ + document.setHasParseOrReparseFailed(true); + bool canceled = false; + jobQueue.setCancelJobRequest([&canceled](const JobRequest &) { + canceled = true; + }); + + + jobQueue.add(createJobRequest(filePath1, JobRequest::Type::UpdateDocumentAnnotations)); + + ASSERT_TRUE(canceled); +} + TEST_F(JobQueue, ProcessEmpty) { jobQueue.processQueue(); @@ -218,6 +253,20 @@ TEST_F(JobQueue, RemoveRequestsForNotIntactDocuments) ASSERT_THAT(jobsToRun.size(), Eq(0)); } +TEST_F(JobQueue, CancelRequestsForNotIntactDocuments) +{ + jobQueue.add(createJobRequest(filePath1, JobRequest::Type::UpdateDocumentAnnotations)); + document.setHasParseOrReparseFailed(true); + bool canceled = false; + jobQueue.setCancelJobRequest([&canceled](const JobRequest &) { + canceled = true; + }); + + jobQueue.processQueue(); + + ASSERT_TRUE(canceled); +} + TEST_F(JobQueue, PrioritizeCurrentDocumentOverNotCurrent) { resetVisibilityAndCurrentEditor(); -- cgit v1.2.1