[PATCH] D75602: [clangd] Cancel certain operations if the file changes before we start.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D75602#1906587 , @thakis wrote:

> This broke building on my bots:
>
> http://45.33.8.238/linux/11862/step_4.txt
>
> `ld.lld: error: undefined symbol: 
> clang::clangd::TUScheduler::InvalidatedError::ID`
>
> Looks like that is indeed declared in this change, but it doesn't have a 
> definition.


Sorry about that, fixing. (I didn't see this on other linkers, maybe because 
the class itself wasn't used in this patch)




Comment at: clang-tools-extra/clangd/Cancellation.cpp:39
+  return true;
   return false; // Not in scope of a task.
 }

kadircet wrote:
> comment seems to be out-of-date.
> 
> `Either not cancelled or not in a cancellable task` ?
Yeah, just removed the comment, this is now pretty obvious.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75602/new/

https://reviews.llvm.org/D75602



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75602: [clangd] Cancel certain operations if the file changes before we start.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

This broke building on my bots:

http://45.33.8.238/linux/11862/step_4.txt

`ld.lld: error: undefined symbol: 
clang::clangd::TUScheduler::InvalidatedError::ID`

Looks like that is indeed declared in this change, but it doesn't have a 
definition.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75602/new/

https://reviews.llvm.org/D75602



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75602: [clangd] Cancel certain operations if the file changes before we start.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc627b120eb8b: [clangd] Cancel certain operations if the file 
changes before we start. (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75602/new/

https://reviews.llvm.org/D75602

Files:
  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: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "Annotations.h"
+#include "Cancellation.h"
 #include "Context.h"
 #include "Diagnostics.h"
 #include "Matchers.h"
@@ -346,6 +347,63 @@
   << "All reads other than R2B were cancelled";
 }
 
+TEST_F(TUSchedulerTests, Invalidation) {
+  auto Path = testPath("foo.cpp");
+  TUScheduler S(CDB, optsForTest(), captureDiags());
+  std::atomic Builds(0), Actions(0);
+
+  Notification Start;
+  updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector) {
+++Builds;
+Start.wait();
+  });
+  S.runWithAST(
+  "invalidatable", Path,
+  [&](llvm::Expected AST) {
+++Actions;
+EXPECT_FALSE(bool(AST));
+llvm::Error E = AST.takeError();
+EXPECT_TRUE(E.isA());
+consumeError(std::move(E));
+  },
+  TUScheduler::InvalidateOnUpdate);
+  S.runWithAST(
+  "not-invalidatable", Path,
+  [&](llvm::Expected AST) {
+++Actions;
+EXPECT_TRUE(bool(AST));
+  },
+  TUScheduler::NoInvalidation);
+  updateWithDiags(S, Path, "b", WantDiagnostics::Auto, [&](std::vector) {
+++Builds;
+ADD_FAILURE() << "Shouldn't build, all dependents invalidated";
+  });
+  S.runWithAST(
+  "invalidatable", Path,
+  [&](llvm::Expected AST) {
+++Actions;
+EXPECT_FALSE(bool(AST));
+llvm::Error E = AST.takeError();
+EXPECT_TRUE(E.isA());
+consumeError(std::move(E));
+  },
+  TUScheduler::InvalidateOnUpdate);
+  updateWithDiags(S, Path, "c", WantDiagnostics::Auto,
+  [&](std::vector) { ++Builds; });
+  S.runWithAST(
+  "invalidatable", Path,
+  [&](llvm::Expected 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;
Index: clang-tools-extra/clangd/unittests/CancellationTests.cpp
===
--- clang-tools-extra/clangd/unittests/CancellationTests.cpp
+++ clang-tools-extra/clangd/unittests/CancellationTests.cpp
@@ -44,6 +44,30 @@
   Task.second();
 }
 
+struct NestedTasks {
+  std::pair 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 HasCancelled(false);
   Notification Cancelled;
Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -219,6 +219,29 @@
   /// Schedule an async task with no dependencies.
   void run(llvm::StringRef Name, llvm::unique_function 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).
+/// Usefu

[PATCH] D75602: [clangd] Cancel certain operations if the file changes before we start.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 248336.
sammccall marked 7 inline comments as done.
sammccall added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75602/new/

https://reviews.llvm.org/D75602

Files:
  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: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "Annotations.h"
+#include "Cancellation.h"
 #include "Context.h"
 #include "Diagnostics.h"
 #include "Matchers.h"
@@ -346,6 +347,63 @@
   << "All reads other than R2B were cancelled";
 }
 
+TEST_F(TUSchedulerTests, Invalidation) {
+  auto Path = testPath("foo.cpp");
+  TUScheduler S(CDB, optsForTest(), captureDiags());
+  std::atomic Builds(0), Actions(0);
+
+  Notification Start;
+  updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector) {
+++Builds;
+Start.wait();
+  });
+  S.runWithAST(
+  "invalidatable", Path,
+  [&](llvm::Expected AST) {
+++Actions;
+EXPECT_FALSE(bool(AST));
+llvm::Error E = AST.takeError();
+EXPECT_TRUE(E.isA());
+consumeError(std::move(E));
+  },
+  TUScheduler::InvalidateOnUpdate);
+  S.runWithAST(
+  "not-invalidatable", Path,
+  [&](llvm::Expected AST) {
+++Actions;
+EXPECT_TRUE(bool(AST));
+  },
+  TUScheduler::NoInvalidation);
+  updateWithDiags(S, Path, "b", WantDiagnostics::Auto, [&](std::vector) {
+++Builds;
+ADD_FAILURE() << "Shouldn't build, all dependents invalidated";
+  });
+  S.runWithAST(
+  "invalidatable", Path,
+  [&](llvm::Expected AST) {
+++Actions;
+EXPECT_FALSE(bool(AST));
+llvm::Error E = AST.takeError();
+EXPECT_TRUE(E.isA());
+consumeError(std::move(E));
+  },
+  TUScheduler::InvalidateOnUpdate);
+  updateWithDiags(S, Path, "c", WantDiagnostics::Auto,
+  [&](std::vector) { ++Builds; });
+  S.runWithAST(
+  "invalidatable", Path,
+  [&](llvm::Expected 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;
Index: clang-tools-extra/clangd/unittests/CancellationTests.cpp
===
--- clang-tools-extra/clangd/unittests/CancellationTests.cpp
+++ clang-tools-extra/clangd/unittests/CancellationTests.cpp
@@ -44,6 +44,30 @@
   Task.second();
 }
 
+struct NestedTasks {
+  std::pair 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 HasCancelled(false);
   Notification Cancelled;
Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -219,6 +219,29 @@
   /// Schedule an async task with no dependencies.
   void run(llvm::StringRef Name, llvm::unique_function 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

[PATCH] D75602: [clangd] Cancel certain operations if the file changes before we start.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:58
+/// without any explicit user action.
+static TUScheduler::ASTActionInvalidation LikelyImplicit =
+TUScheduler::InvalidateOnUpdate;

nit: i would actually keep `InvalidateOnUpdate` on the callsites, but up to you.

also constexpr instead of static ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75602/new/

https://reviews.llvm.org/D75602



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75602: [clangd] Cancel certain operations if the file changes before we start.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, LGTM




Comment at: clang-tools-extra/clangd/Cancellation.cpp:39
+  return true;
   return false; // Not in scope of a task.
 }

comment seems to be out-of-date.

`Either not cancelled or not in a cancellable task` ?



Comment at: clang-tools-extra/clangd/JSONTransport.cpp:23
+  // FIXME: encode cancellation errors using RequestCancelled or 
ContentModified
+  // as approprate.
   if (llvm::Error Unhandled = llvm::handleErrors(

s/approprate/appropriate/



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:673
+// Cancel any requests invalidated by this request.
+if (UpdateType)
+  for (auto &R : llvm::reverse(Requests)) {

braces



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:682
+// Allow this request to be cancelled if invalidated.
+Context Ctx = Context::current().derive(kFileBeingProcessed, FileName);
+Canceler Invalidate = nullptr;

this one is already being put into the context before pushing into requests.



Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:364
+++Actions;
+EXPECT_FALSE(!!AST);
+llvm::Error E = AST.takeError();

nit: `bool(AST)` ? same in other places.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75602/new/

https://reviews.llvm.org/D75602



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75602: [clangd] Cancel certain operations if the file changes before we start.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, jfb, arphaman, jkorous, 
MaskRay, javed.absar, ilya-biryukov.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75602

Files:
  clang-tools-extra/clangd/Cancellation.cpp
  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: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "Annotations.h"
+#include "Cancellation.h"
 #include "Context.h"
 #include "Diagnostics.h"
 #include "Matchers.h"
@@ -346,6 +347,63 @@
   << "All reads other than R2B were cancelled";
 }
 
+TEST_F(TUSchedulerTests, Invalidation) {
+  auto Path = testPath("foo.cpp");
+  TUScheduler S(CDB, optsForTest(), captureDiags());
+  std::atomic Builds(0), Actions(0);
+
+  Notification Start;
+  updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector) {
+++Builds;
+Start.wait();
+  });
+  S.runWithAST(
+  "invalidatable", Path,
+  [&](llvm::Expected AST) {
+++Actions;
+EXPECT_FALSE(!!AST);
+llvm::Error E = AST.takeError();
+EXPECT_TRUE(E.isA());
+consumeError(std::move(E));
+  },
+  TUScheduler::InvalidateOnUpdate);
+  S.runWithAST(
+  "not-invalidatable", Path,
+  [&](llvm::Expected AST) {
+++Actions;
+EXPECT_TRUE(!!AST);
+  },
+  TUScheduler::NoInvalidation);
+  updateWithDiags(S, Path, "b", WantDiagnostics::Auto, [&](std::vector) {
+++Builds;
+ADD_FAILURE() << "Shouldn't build, all dependents invalidated";
+  });
+  S.runWithAST(
+  "invalidatable", Path,
+  [&](llvm::Expected AST) {
+++Actions;
+EXPECT_FALSE(!!AST);
+llvm::Error E = AST.takeError();
+EXPECT_TRUE(E.isA());
+consumeError(std::move(E));
+  },
+  TUScheduler::InvalidateOnUpdate);
+  updateWithDiags(S, Path, "c", WantDiagnostics::Auto,
+  [&](std::vector) { ++Builds; });
+  S.runWithAST(
+  "invalidatable", Path,
+  [&](llvm::Expected AST) {
+++Actions;
+EXPECT_TRUE(!!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;
Index: clang-tools-extra/clangd/unittests/CancellationTests.cpp
===
--- clang-tools-extra/clangd/unittests/CancellationTests.cpp
+++ clang-tools-extra/clangd/unittests/CancellationTests.cpp
@@ -44,6 +44,30 @@
   Task.second();
 }
 
+struct NestedTasks {
+  std::pair 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 HasCancelled(false);
   Notification Cancelled;
Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -219,6 +219,27 @@
   /// Schedule an async task with no dependencies.
   void run(llv