[clang-tools-extra] 39eebe6 - [clangd] Use a separate RunningTask flag instead of leaving a broken request on top of the queue

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2020-03-10T18:25:35+01:00
New Revision: 39eebe68b5990273a69ed527e827753e7d4dba75

URL: https://github.com/llvm/llvm-project/commit/39eebe68b5990273a69ed527e827753e7d4dba75
DIFF: https://github.com/llvm/llvm-project/commit/39eebe68b5990273a69ed527e827753e7d4dba75.diff

LOG: [clangd] Use a separate RunningTask flag instead of leaving a broken request on top of the queue

Summary:
This helps us prevent races when scheduler (or any other thread) tries
to read a request while it's still running.

Reviewers: sammccall

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

Tags: #clang

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

Added: 
    

Modified: 
    clang-tools-extra/clangd/TUScheduler.cpp
    clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 3ce0a5b10d95..7188d0be69ff 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -274,8 +274,9 @@ class ASTWorker {
   /// Becomes ready when the first preamble build finishes.
   Notification PreambleWasBuilt;
   /// Set to true to signal run() to finish processing.
-  bool Done;                    /* GUARDED_BY(Mutex) */
-  std::deque<Request> Requests; /* GUARDED_BY(Mutex) */
+  bool Done;                              /* GUARDED_BY(Mutex) */
+  std::deque<Request> Requests;           /* GUARDED_BY(Mutex) */
+  llvm::Optional<Request> CurrentRequest; /* GUARDED_BY(Mutex) */
   mutable std::condition_variable RequestsCV;
   /// Guards the callback that publishes results of AST-related computations
   /// (diagnostics, highlightings) and file statuses.
@@ -370,7 +371,8 @@ ASTWorker::~ASTWorker() {
 #ifndef NDEBUG
   std::lock_guard<std::mutex> Lock(Mutex);
   assert(Done && "handle was not destroyed");
-  assert(Requests.empty() && "unprocessed requests when destroying ASTWorker");
+  assert(Requests.empty() && !CurrentRequest &&
+         "unprocessed requests when destroying ASTWorker");
 #endif
 }
 
@@ -606,8 +608,10 @@ void ASTWorker::getCurrentPreamble(
   auto LastUpdate =
       std::find_if(Requests.rbegin(), Requests.rend(),
                    [](const Request &R) { return R.UpdateType.hasValue(); });
-  // If there were no writes in the queue, the preamble is ready now.
-  if (LastUpdate == Requests.rend()) {
+  // If there were no writes in the queue, and CurrentRequest is not a write,
+  // the preamble is ready now.
+  if (LastUpdate == Requests.rend() &&
+      (!CurrentRequest || CurrentRequest->UpdateType.hasValue())) {
     Lock.unlock();
     return Callback(getPossiblyStalePreamble());
   }
@@ -714,9 +718,9 @@ void ASTWorker::emitTUStatus(TUAction Action,
 
 void ASTWorker::run() {
   while (true) {
-    Request Req;
     {
       std::unique_lock<std::mutex> Lock(Mutex);
+      assert(!CurrentRequest && "A task is already running, multiple workers?");
       for (auto Wait = scheduleLocked(); !Wait.expired();
            Wait = scheduleLocked()) {
         if (Done) {
@@ -734,7 +738,7 @@ void ASTWorker::run() {
           Tracer.emplace("Debounce");
           SPAN_ATTACH(*Tracer, "next_request", Requests.front().Name);
           if (!(Wait == Deadline::infinity())) {
-            emitTUStatus({TUAction::Queued, Req.Name});
+            emitTUStatus({TUAction::Queued, Requests.front().Name});
             SPAN_ATTACH(*Tracer, "sleep_ms",
                         std::chrono::duration_cast<std::chrono::milliseconds>(
                             Wait.time() - steady_clock::now())
@@ -744,26 +748,28 @@ void ASTWorker::run() {
 
         wait(Lock, RequestsCV, Wait);
       }
-      Req = std::move(Requests.front());
-      // Leave it on the queue for now, so waiters don't see an empty queue.
+      CurrentRequest = std::move(Requests.front());
+      Requests.pop_front();
     } // unlock Mutex
 
+    // It is safe to perform reads to CurrentRequest without holding the lock as
+    // only writer is also this thread.
     {
       std::unique_lock<Semaphore> Lock(Barrier, std::try_to_lock);
       if (!Lock.owns_lock()) {
-        emitTUStatus({TUAction::Queued, Req.Name});
+        emitTUStatus({TUAction::Queued, CurrentRequest->Name});
         Lock.lock();
       }
-      WithContext Guard(std::move(Req.Ctx));
-      trace::Span Tracer(Req.Name);
-      emitTUStatus({TUAction::RunningAction, Req.Name});
-      Req.Action();
+      WithContext Guard(std::move(CurrentRequest->Ctx));
+      trace::Span Tracer(CurrentRequest->Name);
+      emitTUStatus({TUAction::RunningAction, CurrentRequest->Name});
+      CurrentRequest->Action();
     }
 
     bool IsEmpty = false;
     {
       std::lock_guard<std::mutex> Lock(Mutex);
-      Requests.pop_front();
+      CurrentRequest.reset();
       IsEmpty = Requests.empty();
     }
     if (IsEmpty)
@@ -843,7 +849,8 @@ bool ASTWorker::shouldSkipHeadLocked() const {
 
 bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
   std::unique_lock<std::mutex> Lock(Mutex);
-  return wait(Lock, RequestsCV, Timeout, [&] { return Requests.empty(); });
+  return wait(Lock, RequestsCV, Timeout,
+              [&] { return Requests.empty() && !CurrentRequest; });
 }
 
 // Render a TUAction to a user-facing string representation.

diff  --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
index c9569d2d96cc..3d6812496f47 100644
--- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -367,6 +367,30 @@ TEST_F(TUSchedulerTests, Cancellation) {
       << "All reads other than R2B were cancelled";
 }
 
+TEST_F(TUSchedulerTests, InvalidationNoCrash) {
+  auto Path = testPath("foo.cpp");
+  TUScheduler S(CDB, optsForTest(), captureDiags());
+
+  Notification StartedRunning;
+  Notification ScheduledChange;
+  // We expect invalidation logic to not crash by trying to invalidate a running
+  // request.
+  S.update(Path, getInputs(Path, ""), WantDiagnostics::Auto);
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+  S.runWithAST(
+      "invalidatable-but-running", Path,
+      [&](llvm::Expected<InputsAndAST> AST) {
+        StartedRunning.notify();
+        ScheduledChange.wait();
+        ASSERT_TRUE(bool(AST));
+      },
+      TUScheduler::InvalidateOnUpdate);
+  StartedRunning.wait();
+  S.update(Path, getInputs(Path, ""), WantDiagnostics::Auto);
+  ScheduledChange.notify();
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+}
+
 TEST_F(TUSchedulerTests, Invalidation) {
   auto Path = testPath("foo.cpp");
   TUScheduler S(CDB, optsForTest(), captureDiags());