[clangd] Cancel certain operations if the file changes before we start.
authorSam McCall <sam.mccall@gmail.com>
Wed, 4 Mar 2020 13:04:17 +0000 (14:04 +0100)
committerSam McCall <sam.mccall@gmail.com>
Wed, 4 Mar 2020 23:10:07 +0000 (00:10 +0100)
Summary:
Otherwise they can force us to build lots of snapshots that we don't need.
Particularly, try to do this for operations that are frequently
generated by editors without explicit user interaction, and where
editing the file makes the result less useful. (Code action
enumeration is a good example).

https://github.com/clangd/clangd/issues/298

This doesn't return the "right" LSP error code (ContentModified) to the client,
we need to teach the cancellation API to distinguish between different causes.

Reviewers: kadircet

Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, jfb, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D75602

clang-tools-extra/clangd/Cancellation.cpp
clang-tools-extra/clangd/Cancellation.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/JSONTransport.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/CancellationTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

index 6b0e21d..ccd27a1 100644 (file)
@@ -13,20 +13,30 @@ namespace clang {
 namespace clangd {
 
 char CancelledError::ID = 0;
-static Key<std::shared_ptr<std::atomic<bool>>> FlagKey;
+
+// We don't want a cancelable scope to "shadow" an enclosing one.
+struct CancelState {
+  std::shared_ptr<std::atomic<bool>> Cancelled;
+  const CancelState *Parent;
+};
+static Key<CancelState> StateKey;
 
 std::pair<Context, Canceler> cancelableTask() {
-  auto Flag = std::make_shared<std::atomic<bool>>();
+  CancelState State;
+  State.Cancelled = std::make_shared<std::atomic<bool>>();
+  State.Parent = Context::current().get(StateKey);
   return {
-      Context::current().derive(FlagKey, Flag),
-      [Flag] { *Flag = true; },
+      Context::current().derive(StateKey, State),
+      [Flag(State.Cancelled)] { *Flag = true; },
   };
 }
 
 bool isCancelled(const Context &Ctx) {
-  if (auto *Flag = Ctx.get(FlagKey))
-    return **Flag;
-  return false; // Not in scope of a task.
+  for (const CancelState *State = Ctx.get(StateKey); State != nullptr;
+       State = State->Parent)
+    if (State->Cancelled->load())
+      return true;
+  return false;
 }
 
 } // namespace clangd
index 6872c44..bdb0bd4 100644 (file)
@@ -76,6 +76,7 @@ using Canceler = std::function<void()>;
 std::pair<Context, Canceler> cancelableTask();
 
 /// True if the current context is within a cancelable task which was cancelled.
+/// (If the context is within multiple nested tasks, true if any are cancelled).
 /// Always false if there is no active cancelable task.
 /// This isn't free (context lookup) - don't call it in a tight loop.
 bool isCancelled(const Context &Ctx = Context::current());
index 5dd0032..cd833af 100644 (file)
@@ -437,7 +437,8 @@ void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
     CB(std::move(Res));
   };
 
-  WorkScheduler.runWithAST("EnumerateTweaks", File, std::move(Action));
+  WorkScheduler.runWithAST("EnumerateTweaks", File, std::move(Action),
+                           TUScheduler::InvalidateOnUpdate);
 }
 
 void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
@@ -556,7 +557,8 @@ void ClangdServer::findDocumentHighlights(
         CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
       };
 
-  WorkScheduler.runWithAST("Highlights", File, std::move(Action));
+  WorkScheduler.runWithAST("Highlights", File, std::move(Action),
+                           TUScheduler::InvalidateOnUpdate);
 }
 
 void ClangdServer::findHover(PathRef File, Position Pos,
@@ -570,7 +572,8 @@ void ClangdServer::findHover(PathRef File, Position Pos,
     CB(clangd::getHover(InpAST->AST, Pos, std::move(Style), Index));
   };
 
-  WorkScheduler.runWithAST("Hover", File, std::move(Action));
+  WorkScheduler.runWithAST("Hover", File, std::move(Action),
+                           TUScheduler::InvalidateOnUpdate);
 }
 
 void ClangdServer::typeHierarchy(PathRef File, Position Pos, int Resolve,
@@ -618,7 +621,8 @@ void ClangdServer::documentSymbols(llvm::StringRef File,
           return CB(InpAST.takeError());
         CB(clangd::getDocumentSymbols(InpAST->AST));
       };
-  WorkScheduler.runWithAST("documentSymbols", File, std::move(Action));
+  WorkScheduler.runWithAST("documentSymbols", File, std::move(Action),
+                           TUScheduler::InvalidateOnUpdate);
 }
 
 void ClangdServer::findReferences(PathRef File, Position Pos, uint32_t Limit,
@@ -664,7 +668,8 @@ void ClangdServer::documentLinks(PathRef File,
           return CB(InpAST.takeError());
         CB(clangd::getDocumentLinks(InpAST->AST));
       };
-  WorkScheduler.runWithAST("DocumentLinks", File, std::move(Action));
+  WorkScheduler.runWithAST("DocumentLinks", File, std::move(Action),
+                           TUScheduler::InvalidateOnUpdate);
 }
 
 std::vector<std::pair<Path, std::size_t>>
index 43be9f7..d82dca1 100644 (file)
@@ -19,6 +19,8 @@ namespace {
 llvm::json::Object encodeError(llvm::Error E) {
   std::string Message;
   ErrorCode Code = ErrorCode::UnknownErrorCode;
+  // FIXME: encode cancellation errors using RequestCancelled or ContentModified
+  // as appropriate.
   if (llvm::Error Unhandled = llvm::handleErrors(
           std::move(E), [&](const LSPError &L) -> llvm::Error {
             Message = L.Message;
index 3f3162a..714bc72 100644 (file)
@@ -184,7 +184,8 @@ public:
   void update(ParseInputs Inputs, WantDiagnostics);
   void
   runWithAST(llvm::StringRef Name,
-             llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action);
+             llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
+             TUScheduler::ASTActionInvalidation);
   bool blockUntilIdle(Deadline Timeout) const;
 
   std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
@@ -214,7 +215,8 @@ private:
   void stop();
   /// Adds a new task to the end of the request queue.
   void startTask(llvm::StringRef Name, llvm::unique_function<void()> Task,
-                 llvm::Optional<WantDiagnostics> UpdateType);
+                 llvm::Optional<WantDiagnostics> UpdateType,
+                 TUScheduler::ASTActionInvalidation);
   /// Updates the TUStatus and emits it. Only called in the worker thread.
   void emitTUStatus(TUAction FAction,
                     const TUStatus::BuildDetails *Detail = nullptr);
@@ -237,6 +239,8 @@ private:
     steady_clock::time_point AddTime;
     Context Ctx;
     llvm::Optional<WantDiagnostics> UpdateType;
+    TUScheduler::ASTActionInvalidation InvalidationPolicy;
+    Canceler Invalidate;
   };
 
   /// Handles retention of ASTs.
@@ -542,12 +546,13 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
     // Stash the AST in the cache for further use.
     IdleASTs.put(this, std::move(*AST));
   };
-  startTask(TaskName, std::move(Task), WantDiags);
+  startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation);
 }
 
 void ASTWorker::runWithAST(
     llvm::StringRef Name,
-    llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action) {
+    llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
+    TUScheduler::ASTActionInvalidation Invalidation) {
   auto Task = [=, Action = std::move(Action)]() mutable {
     if (isCancelled())
       return Action(llvm::make_error<CancelledError>());
@@ -576,7 +581,7 @@ void ASTWorker::runWithAST(
           "invalid AST", llvm::errc::invalid_argument));
     Action(InputsAndAST{*CurrentInputs, **AST});
   };
-  startTask(Name, std::move(Task), /*UpdateType=*/None);
+  startTask(Name, std::move(Task), /*UpdateType=*/None, Invalidation);
 }
 
 std::shared_ptr<const PreambleData>
@@ -606,7 +611,9 @@ void ASTWorker::getCurrentPreamble(
                           },
                           "GetPreamble", steady_clock::now(),
                           Context::current().clone(),
-                          /*UpdateType=*/None});
+                          /*UpdateType=*/None,
+                          /*InvalidationPolicy=*/TUScheduler::NoInvalidation,
+                          /*Invalidate=*/nullptr});
   Lock.unlock();
   RequestsCV.notify_all();
 }
@@ -650,7 +657,8 @@ void ASTWorker::stop() {
 
 void ASTWorker::startTask(llvm::StringRef Name,
                           llvm::unique_function<void()> Task,
-                          llvm::Optional<WantDiagnostics> UpdateType) {
+                          llvm::Optional<WantDiagnostics> UpdateType,
+                          TUScheduler::ASTActionInvalidation Invalidation) {
   if (RunSync) {
     assert(!Done && "running a task after stop()");
     trace::Span Tracer(Name + ":" + llvm::sys::path::filename(FileName));
@@ -661,9 +669,26 @@ void ASTWorker::startTask(llvm::StringRef Name,
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     assert(!Done && "running a task after stop()");
-    Requests.push_back(
-        {std::move(Task), std::string(Name), steady_clock::now(),
-         Context::current().derive(kFileBeingProcessed, FileName), UpdateType});
+    // Cancel any requests invalidated by this request.
+    if (UpdateType) {
+      for (auto &R : llvm::reverse(Requests)) {
+        if (R.InvalidationPolicy == TUScheduler::InvalidateOnUpdate)
+          R.Invalidate();
+        if (R.UpdateType)
+          break; // Older requests were already invalidated by the older update.
+      }
+    }
+
+    // Allow this request to be cancelled if invalidated.
+    Context Ctx = Context::current().derive(kFileBeingProcessed, FileName);
+    Canceler Invalidate = nullptr;
+    if (Invalidation) {
+      WithContext WC(std::move(Ctx));
+      std::tie(Ctx, Invalidate) = cancelableTask();
+    }
+    Requests.push_back({std::move(Task), std::string(Name), steady_clock::now(),
+                        std::move(Ctx), UpdateType, Invalidation,
+                        std::move(Invalidate)});
   }
   RequestsCV.notify_all();
 }
@@ -942,7 +967,8 @@ void TUScheduler::run(llvm::StringRef Name,
 
 void TUScheduler::runWithAST(
     llvm::StringRef Name, PathRef File,
-    llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action) {
+    llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
+    TUScheduler::ASTActionInvalidation Invalidation) {
   auto It = Files.find(File);
   if (It == Files.end()) {
     Action(llvm::make_error<LSPError>(
@@ -950,7 +976,7 @@ void TUScheduler::runWithAST(
     return;
   }
 
-  It->second->Worker->runWithAST(Name, std::move(Action));
+  It->second->Worker->runWithAST(Name, std::move(Action), Invalidation);
 }
 
 void TUScheduler::runWithPreamble(llvm::StringRef Name, PathRef File,
@@ -1067,5 +1093,15 @@ DebouncePolicy DebouncePolicy::fixed(clock::duration T) {
   return P;
 }
 
+void TUScheduler::InvalidatedError::log(llvm::raw_ostream &OS) const {
+  switch (Policy) {
+  case InvalidateOnUpdate:
+    OS << "Task was cancelled due to a subsequent change to the file.";
+    break;
+  case NoInvalidation:
+    llvm_unreachable("Invalidated for no reason?");
+  }
+}
+
 } // namespace clangd
 } // namespace clang
index 948fde7..074404c 100644 (file)
@@ -219,6 +219,29 @@ public:
   /// Schedule an async task with no dependencies.
   void run(llvm::StringRef Name, llvm::unique_function<void()> Action);
 
+  /// Defines how a runWithAST action is implicitly cancelled by other actions.
+  enum ASTActionInvalidation {
+    /// The request will run unless explicitly cancelled.
+    NoInvalidation,
+    /// The request will be implicitly cancelled by a subsequent update().
+    /// (Only if the request was not yet cancelled).
+    /// Useful for requests that are generated by clients, without any explicit
+    /// user action. These can otherwise e.g. force every version to be built.
+    InvalidateOnUpdate,
+  };
+
+  /// Error to return when an ASTActionInvalidation policy fires.
+  class InvalidatedError : public llvm::ErrorInfo<InvalidatedError> {
+  public:
+    static char ID;
+    ASTActionInvalidation Policy;
+
+    void log(llvm::raw_ostream &OS) const override;
+    std::error_code convertToErrorCode() const override {
+      return std::make_error_code(std::errc::interrupted);
+    }
+  };
+
   /// Schedule an async read of the AST. \p Action will be called when AST is
   /// ready. The AST passed to \p Action refers to the version of \p File
   /// tracked at the time of the call, even if new updates are received before
@@ -226,9 +249,11 @@ public:
   /// If an error occurs during processing, it is forwarded to the \p Action
   /// callback.
   /// If the context is cancelled before the AST is ready, the callback will
-  /// receive a CancelledError.
+  /// receive a CancelledError. If the invalidation policy is triggered, the
+  /// callback will receive an InvalidatedError.
   void runWithAST(llvm::StringRef Name, PathRef File,
-                  Callback<InputsAndAST> Action);
+                  Callback<InputsAndAST> Action,
+                  ASTActionInvalidation = NoInvalidation);
 
   /// Controls whether preamble reads wait for the preamble to be up-to-date.
   enum PreambleConsistency {
index 611ce07..3fe7190 100644 (file)
@@ -44,6 +44,30 @@ TEST(CancellationTest, TaskContextDiesHandleLives) {
   Task.second();
 }
 
+struct NestedTasks {
+  std::pair<Context, Canceler> Outer, Inner;
+  NestedTasks() {
+    Outer = cancelableTask();
+    {
+      WithContext WithOuter(Outer.first.clone());
+      Inner = cancelableTask();
+    }
+  }
+};
+
+TEST(CancellationTest, Nested) {
+  // Cancelling inner task works but leaves outer task unaffected.
+  NestedTasks CancelInner;
+  CancelInner.Inner.second();
+  EXPECT_TRUE(isCancelled(CancelInner.Inner.first));
+  EXPECT_FALSE(isCancelled(CancelInner.Outer.first));
+  // Cancellation of outer task is inherited by inner task.
+  NestedTasks CancelOuter;
+  CancelOuter.Outer.second();
+  EXPECT_TRUE(isCancelled(CancelOuter.Inner.first));
+  EXPECT_TRUE(isCancelled(CancelOuter.Outer.first));
+}
+
 TEST(CancellationTest, AsynCancellationTest) {
   std::atomic<bool> HasCancelled(false);
   Notification Cancelled;
index 9e5952f..365e534 100644 (file)
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
+#include "Cancellation.h"
 #include "Context.h"
 #include "Diagnostics.h"
 #include "Matchers.h"
@@ -346,6 +347,63 @@ TEST_F(TUSchedulerTests, Cancellation) {
       << "All reads other than R2B were cancelled";
 }
 
+TEST_F(TUSchedulerTests, Invalidation) {
+  auto Path = testPath("foo.cpp");
+  TUScheduler S(CDB, optsForTest(), captureDiags());
+  std::atomic<int> Builds(0), Actions(0);
+
+  Notification Start;
+  updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector<Diag>) {
+    ++Builds;
+    Start.wait();
+  });
+  S.runWithAST(
+      "invalidatable", Path,
+      [&](llvm::Expected<InputsAndAST> AST) {
+        ++Actions;
+        EXPECT_FALSE(bool(AST));
+        llvm::Error E = AST.takeError();
+        EXPECT_TRUE(E.isA<CancelledError>());
+        consumeError(std::move(E));
+      },
+      TUScheduler::InvalidateOnUpdate);
+  S.runWithAST(
+      "not-invalidatable", Path,
+      [&](llvm::Expected<InputsAndAST> AST) {
+        ++Actions;
+        EXPECT_TRUE(bool(AST));
+      },
+      TUScheduler::NoInvalidation);
+  updateWithDiags(S, Path, "b", WantDiagnostics::Auto, [&](std::vector<Diag>) {
+    ++Builds;
+    ADD_FAILURE() << "Shouldn't build, all dependents invalidated";
+  });
+  S.runWithAST(
+      "invalidatable", Path,
+      [&](llvm::Expected<InputsAndAST> AST) {
+        ++Actions;
+        EXPECT_FALSE(bool(AST));
+        llvm::Error E = AST.takeError();
+        EXPECT_TRUE(E.isA<CancelledError>());
+        consumeError(std::move(E));
+      },
+      TUScheduler::InvalidateOnUpdate);
+  updateWithDiags(S, Path, "c", WantDiagnostics::Auto,
+                  [&](std::vector<Diag>) { ++Builds; });
+  S.runWithAST(
+      "invalidatable", Path,
+      [&](llvm::Expected<InputsAndAST> AST) {
+        ++Actions;
+        EXPECT_TRUE(bool(AST)) << "Shouldn't be invalidated, no update follows";
+      },
+      TUScheduler::InvalidateOnUpdate);
+  Start.notify();
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+
+  EXPECT_EQ(2, Builds.load()) << "Middle build should be skipped";
+  EXPECT_EQ(4, Actions.load()) << "All actions should run (some with error)";
+}
+
 TEST_F(TUSchedulerTests, ManyUpdates) {
   const int FilesCount = 3;
   const int UpdatesPerFile = 10;