From c76394bc4b74b3104654ade5c0e87afcba13d6cd Mon Sep 17 00:00:00 2001 From: Ilya Biryukov Date: Thu, 22 Nov 2018 15:39:54 +0000 Subject: [PATCH] [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy Summary: Previously, removeDoc followed by an addDoc to TUScheduler resulted in racy diagnostic responses, i.e. the old dianostics could be delivered to the client after the new ones by TUScheduler. To workaround this, we tracked a version number in ClangdServer and discarded stale diagnostics. After this commit, the TUScheduler will stop delivering diagnostics for removed files and the workaround in ClangdServer is not required anymore. Reviewers: sammccall Reviewed By: sammccall Subscribers: javed.absar, ioeric, MaskRay, jkorous, arphaman, jfb, kadircet, cfe-commits Differential Revision: https://reviews.llvm.org/D54829 llvm-svn: 347468 --- clang-tools-extra/clangd/ClangdServer.cpp | 26 +++---------------- clang-tools-extra/clangd/ClangdServer.h | 17 ++---------- clang-tools-extra/clangd/TUScheduler.cpp | 25 +++++++++++++++++- clang-tools-extra/clangd/TUScheduler.h | 3 ++- .../unittests/clangd/ClangdTests.cpp | 3 ++- .../unittests/clangd/TUSchedulerTests.cpp | 7 +++++ 6 files changed, 40 insertions(+), 41 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 8d925d7cdcad..03ebcc60a279 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -133,19 +133,17 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB, void ClangdServer::addDocument(PathRef File, StringRef Contents, WantDiagnostics WantDiags) { - DocVersion Version = ++InternalVersion[File]; ParseInputs Inputs = {getCompileCommand(File), FSProvider.getFileSystem(), Contents.str()}; - Path FileStr = File.str(); WorkScheduler.update(File, std::move(Inputs), WantDiags, - [this, FileStr, Version](std::vector Diags) { - consumeDiagnostics(FileStr, Version, std::move(Diags)); + [this, FileStr](std::vector Diags) { + DiagConsumer.onDiagnosticsReady(FileStr, + std::move(Diags)); }); } void ClangdServer::removeDocument(PathRef File) { - ++InternalVersion[File]; WorkScheduler.remove(File); } @@ -444,24 +442,6 @@ void ClangdServer::findHover(PathRef File, Position Pos, WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB))); } -void ClangdServer::consumeDiagnostics(PathRef File, DocVersion Version, - std::vector Diags) { - // We need to serialize access to resulting diagnostics to avoid calling - // `onDiagnosticsReady` in the wrong order. - std::lock_guard DiagsLock(DiagnosticsMutex); - DocVersion &LastReportedDiagsVersion = ReportedDiagnosticVersions[File]; - - // FIXME(ibiryukov): get rid of '<' comparison here. In the current - // implementation diagnostics will not be reported after version counters' - // overflow. This should not happen in practice, since DocVersion is a - // 64-bit unsigned integer. - if (Version < LastReportedDiagsVersion) - return; - LastReportedDiagsVersion = Version; - - DiagConsumer.onDiagnosticsReady(File, std::move(Diags)); -} - tooling::CompileCommand ClangdServer::getCompileCommand(PathRef File) { trace::Span Span("GetCompileCommand"); Optional C = CDB.getCompileCommand(File); diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index 92d2b3691ba2..5c829ae83132 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -125,7 +125,8 @@ public: WantDiagnostics WD = WantDiagnostics::Auto); /// Remove \p File from list of tracked files, schedule a request to free - /// resources associated with it. + /// resources associated with it. Pending diagnostics for closed files may not + /// be delivered, even if requested with WantDiags::Auto or WantDiags::Yes. void removeDocument(PathRef File); /// Run code completion for \p File at \p Pos. @@ -222,20 +223,12 @@ private: formatCode(llvm::StringRef Code, PathRef File, ArrayRef Ranges); - typedef uint64_t DocVersion; - - void consumeDiagnostics(PathRef File, DocVersion Version, - std::vector Diags); - tooling::CompileCommand getCompileCommand(PathRef File); const GlobalCompilationDatabase &CDB; DiagnosticsConsumer &DiagConsumer; const FileSystemProvider &FSProvider; - /// Used to synchronize diagnostic responses for added and removed files. - llvm::StringMap InternalVersion; - Path ResourceDir; // The index used to look up symbols. This could be: // - null (all index functionality is optional) @@ -255,12 +248,6 @@ private: llvm::Optional WorkspaceRoot; std::shared_ptr PCHs; - /// Used to serialize diagnostic callbacks. - /// FIXME(ibiryukov): get rid of an extra map and put all version counters - /// into CppFile. - std::mutex DiagnosticsMutex; - /// Maps from a filename to the latest version of reported diagnostics. - llvm::StringMap ReportedDiagnosticVersions; // WorkScheduler has to be the last member, because its destructor has to be // called before all other members to stop the worker thread that references // ClangdServer. diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp index 79299ca7fe3c..8bd3b8b41dab 100644 --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -252,6 +252,13 @@ private: bool Done; /* GUARDED_BY(Mutex) */ std::deque Requests; /* GUARDED_BY(Mutex) */ mutable std::condition_variable RequestsCV; + /// Guards a critical section for running the diagnostics callbacks. + std::mutex DiagsMu; + // Used to prevent remove document + leading to out-of-order diagnostics: + // The lifetime of the old/new ASTWorkers will overlap, but their handles + // don't. When the old handle is destroyed, the old worker will stop reporting + // diagnostics. + bool ReportDiagnostics = true; /* GUARDED_BY(DiagMu) */ }; /// A smart-pointer-like class that points to an active ASTWorker. @@ -406,6 +413,14 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags, if (WantDiags == WantDiagnostics::No) return; + { + std::lock_guard Lock(DiagsMu); + // No need to rebuild the AST if we won't send the diagnotics. However, + // note that we don't prevent preamble rebuilds. + if (!ReportDiagnostics) + return; + } + // Get the AST for diagnostics. Optional> AST = IdleASTs.take(this); if (!AST) { @@ -418,7 +433,11 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags, // spam us with updates. // Note *AST can still be null if buildAST fails. if (*AST) { - OnUpdated((*AST)->getDiagnostics()); + { + std::lock_guard Lock(DiagsMu); + if (ReportDiagnostics) + OnUpdated((*AST)->getDiagnostics()); + } trace::Span Span("Running main AST callback"); Callbacks.onMainAST(FileName, **AST); DiagsWereReported = true; @@ -512,6 +531,10 @@ std::size_t ASTWorker::getUsedBytes() const { bool ASTWorker::isASTCached() const { return IdleASTs.getUsedBytes(this) != 0; } void ASTWorker::stop() { + { + std::lock_guard Lock(DiagsMu); + ReportDiagnostics = false; + } { std::lock_guard Lock(Mutex); assert(!Done && "stop() called twice"); diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h index d78ae8630776..9dd4f3732a81 100644 --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -108,7 +108,8 @@ public: llvm::unique_function)> OnUpdated); /// Remove \p File from the list of tracked files and schedule removal of its - /// resources. + /// resources. Pending diagnostics for closed files may not be delivered, even + /// if requested with WantDiags::Auto or WantDiags::Yes. void remove(PathRef File); /// Schedule an async task with no dependencies. diff --git a/clang-tools-extra/unittests/clangd/ClangdTests.cpp b/clang-tools-extra/unittests/clangd/ClangdTests.cpp index d3a09ab891ea..023ca489227a 100644 --- a/clang-tools-extra/unittests/clangd/ClangdTests.cpp +++ b/clang-tools-extra/unittests/clangd/ClangdTests.cpp @@ -748,7 +748,8 @@ int d; BlockingRequests[RequestIndex](); } } - } // Wait for ClangdServer to shutdown before proceeding. + ASSERT_TRUE(Server.blockUntilIdleForTest()); + } // Check some invariants about the state of the program. std::vector Stats = DiagConsumer.takeFileStats(); diff --git a/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp b/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp index e21788ee4440..8f5b4c6f4033 100644 --- a/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp +++ b/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp @@ -124,6 +124,8 @@ TEST_F(TUSchedulerTests, WantDiagnostics) { S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto, [&](std::vector Diags) { ++CallbackCount; }); Ready.notify(); + + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } EXPECT_EQ(2, CallbackCount); } @@ -147,6 +149,8 @@ TEST_F(TUSchedulerTests, Debounce) { std::this_thread::sleep_for(std::chrono::seconds(2)); S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto, [&](std::vector Diags) { ++CallbackCount; }); + + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } EXPECT_EQ(2, CallbackCount); } @@ -267,6 +271,8 @@ TEST_F(TUSchedulerTests, Cancellation) { Update("U3"); Read("R3")(); Proceed.notify(); + + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } EXPECT_THAT(DiagsSeen, ElementsAre("U2", "U3")) << "U1 and all dependent reads were cancelled. " @@ -373,6 +379,7 @@ TEST_F(TUSchedulerTests, ManyUpdates) { } } } + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } // TUScheduler destructor waits for all operations to finish. std::lock_guard Lock(Mut);