[clang-tools-extra] r347468 - [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 22 07:39:54 PST 2018
Author: ibiryukov
Date: Thu Nov 22 07:39:54 2018
New Revision: 347468
URL: http://llvm.org/viewvc/llvm-project?rev=347468&view=rev
Log:
[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
Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=347468&r1=347467&r2=347468&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Thu Nov 22 07:39:54 2018
@@ -133,19 +133,17 @@ ClangdServer::ClangdServer(const GlobalC
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<Diag> Diags) {
- consumeDiagnostics(FileStr, Version, std::move(Diags));
+ [this, FileStr](std::vector<Diag> 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 Fil
WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB)));
}
-void ClangdServer::consumeDiagnostics(PathRef File, DocVersion Version,
- std::vector<Diag> Diags) {
- // We need to serialize access to resulting diagnostics to avoid calling
- // `onDiagnosticsReady` in the wrong order.
- std::lock_guard<std::mutex> 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<tooling::CompileCommand> C = CDB.getCompileCommand(File);
Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=347468&r1=347467&r2=347468&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.h Thu Nov 22 07:39:54 2018
@@ -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<tooling::Range> Ranges);
- typedef uint64_t DocVersion;
-
- void consumeDiagnostics(PathRef File, DocVersion Version,
- std::vector<Diag> 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<DocVersion> 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<std::string> WorkspaceRoot;
std::shared_ptr<PCHContainerOperations> 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<DocVersion> 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.
Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=347468&r1=347467&r2=347468&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Thu Nov 22 07:39:54 2018
@@ -252,6 +252,13 @@ private:
bool Done; /* GUARDED_BY(Mutex) */
std::deque<Request> 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 Input
if (WantDiags == WantDiagnostics::No)
return;
+ {
+ std::lock_guard<std::mutex> 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<std::unique_ptr<ParsedAST>> AST = IdleASTs.take(this);
if (!AST) {
@@ -418,7 +433,11 @@ void ASTWorker::update(ParseInputs Input
// spam us with updates.
// Note *AST can still be null if buildAST fails.
if (*AST) {
- OnUpdated((*AST)->getDiagnostics());
+ {
+ std::lock_guard<std::mutex> Lock(DiagsMu);
+ if (ReportDiagnostics)
+ OnUpdated((*AST)->getDiagnostics());
+ }
trace::Span Span("Running main AST callback");
Callbacks.onMainAST(FileName, **AST);
DiagsWereReported = true;
@@ -513,6 +532,10 @@ bool ASTWorker::isASTCached() const { re
void ASTWorker::stop() {
{
+ std::lock_guard<std::mutex> Lock(DiagsMu);
+ ReportDiagnostics = false;
+ }
+ {
std::lock_guard<std::mutex> Lock(Mutex);
assert(!Done && "stop() called twice");
Done = true;
Modified: clang-tools-extra/trunk/clangd/TUScheduler.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.h?rev=347468&r1=347467&r2=347468&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.h (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.h Thu Nov 22 07:39:54 2018
@@ -108,7 +108,8 @@ public:
llvm::unique_function<void(std::vector<Diag>)> 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.
Modified: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp?rev=347468&r1=347467&r2=347468&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp Thu Nov 22 07:39:54 2018
@@ -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<FileStat> Stats = DiagConsumer.takeFileStats();
Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=347468&r1=347467&r2=347468&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Thu Nov 22 07:39:54 2018
@@ -124,6 +124,8 @@ TEST_F(TUSchedulerTests, WantDiagnostics
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
[&](std::vector<Diag> 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<Diag> 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<std::mutex> Lock(Mut);
More information about the cfe-commits
mailing list