This revision was automatically updated to reflect the committed changes.
Closed by commit rG39eebe68b599: [clangd] Use a separate RunningTask flag 
instead of leaving a broken request on… (authored by kadircet).
Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75927

Files:
  clang-tools-extra/clangd/TUScheduler.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
@@ -367,6 +367,30 @@
       << "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());
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -274,8 +274,9 @@
   /// 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 @@
 #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 @@
   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::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 @@
           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 @@
 
         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::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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to