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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:279
   std::deque<Request> Requests; /* GUARDED_BY(Mutex) */
+  bool RunningTask = false;     /* GUARDED_BY(Mutex) */
   mutable std::condition_variable RequestsCV;
----------------
I'd consider making this an `llvm::Optional<Request>` and using it as storage too. Even though it won't be accessed directly outside the main loop, I think it makes it easier to reason about the code: it emphasizes the parallel with `Requests` and minimizes the spooky-flags-at-a-distance effect.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:612
   // If there were no writes in the queue, the preamble is ready now.
   if (LastUpdate == Requests.rend()) {
     Lock.unlock();
----------------
what if the currently-running task is a write?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:721
     {
       std::unique_lock<std::mutex> Lock(Mutex);
       for (auto Wait = scheduleLocked(); !Wait.expired();
----------------
maybe assert there's no running task here?


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:376
   Notification Start;
-  updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector<Diag>) {
-    ++Builds;
-    Start.wait();
-  });
+  updateWithDiags(S, Path, "b", WantDiagnostics::Auto,
+                  [&](std::vector<Diag>) { ++Builds; });
----------------
does this deliberately match "b" below? if not, change back to something unique?
If so, probably needs a comment.


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:379
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+  S.runWithAST(
+      "invalidatable-but-running", Path,
----------------
This looks racy to me. What guarantees that the worker thread pulls this item off the queue and into the "running" slot before the second updateWithDiags?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75927