diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -31,6 +31,7 @@ #include "clang/Tooling/Core/Replacement.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/Optional.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Errc.h" @@ -60,11 +61,17 @@ FIndex->updatePreamble(Path, Ctx, std::move(PP), CanonIncludes); } - void onMainAST(PathRef Path, ParsedAST &AST) override { + void onMainAST(PathRef Path, ParsedAST &AST, + llvm::function_ref Publish) override { if (FIndex) FIndex->updateMain(Path, AST); - if (SemanticHighlighting) - DiagConsumer.onHighlightingsReady(Path, getSemanticHighlightings(AST)); + + if (!SemanticHighlighting) + return; + auto Highlightings = getSemanticHighlightings(AST); + Publish([&]() { + DiagConsumer.onHighlightingsReady(Path, std::move(Highlightings)); + }); } void onDiagnostics(PathRef File, std::vector Diags) override { diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -15,6 +15,7 @@ #include "Threading.h" #include "index/CanonicalIncludes.h" #include "llvm/ADT/Optional.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include @@ -88,6 +89,7 @@ BuildDetails Details; }; + class ParsingCallbacks { public: virtual ~ParsingCallbacks() = default; @@ -98,6 +100,10 @@ virtual void onPreambleAST(PathRef Path, ASTContext &Ctx, std::shared_ptr PP, const CanonicalIncludes &) {} + /// A function that is run under the critical section guarding against races + /// when closing the files. + using PublishResults = llvm::function_ref; + /// Called on the AST built for the file itself. Note that preamble AST nodes /// are not deserialized and should be processed in the onPreambleAST call /// instead. @@ -108,7 +114,14 @@ /// etc. Clients are expected to process only the AST nodes from the main file /// in this callback (obtained via ParsedAST::getLocalTopLevelDecls) to obtain /// optimal performance. - virtual void onMainAST(PathRef Path, ParsedAST &AST) {} + /// + /// This callback is run on a worker thread and if we need to send results of + /// processing AST to the users, we might run into a race condition with file + /// removal. To avoid the race condition, send the results by running \p + /// Publish. That would ensure publishing the results is either sequenced + /// before removing the file or does not happen at all. + virtual void onMainAST(PathRef Path, ParsedAST &AST, + llvm::function_ref Publish) {} /// Called whenever the diagnostics for \p File are produced. virtual void onDiagnostics(PathRef File, std::vector Diags) {} diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -486,13 +486,22 @@ // spam us with updates. // Note *AST can still be null if buildAST fails. if (*AST) { - { + // Results from the worker thread should only be published if the file was + // not removed. This callback ensures the passed operation is not racing + // with file removal. + auto Publish = [&](ParsingCallbacks::PublishResults Action) { std::lock_guard Lock(DiagsMu); if (ReportDiagnostics) - Callbacks.onDiagnostics(FileName, (*AST)->getDiagnostics()); - } + Action(); + }; + + // Publish diagnostics. + Publish([&]() { + Callbacks.onDiagnostics(FileName, (*AST)->getDiagnostics()); + }); + trace::Span Span("Running main AST callback"); - Callbacks.onMainAST(FileName, **AST); + Callbacks.onMainAST(FileName, **AST, Publish); DiagsWereReported = true; } // Stash the AST in the cache for further use.