[PATCH] D90595: [clangd] Fix race in background index rebuild, where index could stay stale.

[Sam McCall via Phabricator via cfe-commits]

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

The BackgroundQueue worker that finishes the last task is responsible for
calling OnIdle(). However calling OnIdle itself may be the "last task"
necessitating another call to OnIdle!

Consider:

  Main      +T1 +T2            +T3
  Worker1      [---T1---]       [---T3---]
  Worker2           [---T2---][---Idle1---]

Idle1 starts running before T3 has started, let alone finished.
So it doesn't satisfy the goal that OnIdle runs after other work.

On the other hand, Worker1 won't run OnIdle after T3, as Idle1 is still running.
Therefore Worker2 needs to run OnIdle again (Idle2) once Idle1 completes.
This patch does that.

An example test flake due to this oversight:
http://lab.llvm.org:8011/#/builders/100/builds/721/steps/7/logs/FAIL__Clangd_Unit_Tests__BackgroundIndexTest_Confi


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90595

Files:
  clang-tools-extra/clangd/index/Background.h
  clang-tools-extra/clangd/index/BackgroundQueue.cpp
  clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp


Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
@@ -7,6 +7,7 @@
 #include "TestTU.h"
 #include "index/Background.h"
 #include "index/BackgroundRebuild.h"
+#include "support/Threading.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -826,6 +827,41 @@
   }
 }
 
+TEST(BackgroundQueueTest, Idle) {
+  std::atomic<unsigned> TaskCount(0), IdleCount(0);
+  BackgroundQueue::Task Task([&] { ++TaskCount; });
+
+  // Set up a queue initially with 10 tasks.
+  AsyncTaskRunner ThreadPool;
+  BackgroundQueue Q;
+  for (unsigned I = 0; I < 10; ++I)
+    Q.push(Task);
+  Notification Start;
+  for (unsigned I = 0; I < 5; ++I)
+    ThreadPool.runAsync("worker", [&] {
+      Start.wait();
+      Q.work(/*OnIdle=*/[&] {
+        // Should be nothing else running now, so this assertion isn't racy.
+        EXPECT_EQ(10u + 2u * IdleCount, TaskCount);
+
+        // The first two times the queue goes idle, add a couple more tasks.
+        // This should caute OnIdle to run again.
+        if (++IdleCount <= 2) {
+          Q.push(Task);
+          Q.push(Task);
+        }
+      });
+    });
+
+  Start.notify();
+  ASSERT_TRUE(Q.blockUntilIdleForTest(/*TimeoutSeconds=*/5));
+  Q.stop();
+  ASSERT_TRUE(ThreadPool.wait(timeoutSeconds(5)));
+
+  EXPECT_EQ(3u, IdleCount)
+      << "Queue exhausts original items, then the extra two (twice)";
+}
+
 TEST(BackgroundQueueTest, Progress) {
   using testing::AnyOf;
   BackgroundQueue::Stats S;
Index: clang-tools-extra/clangd/index/BackgroundQueue.cpp
===================================================================
--- clang-tools-extra/clangd/index/BackgroundQueue.cpp
+++ clang-tools-extra/clangd/index/BackgroundQueue.cpp
@@ -46,8 +46,11 @@
     {
       std::unique_lock<std::mutex> Lock(Mu);
       ++Stat.Completed;
-      if (Stat.Active == 1 && Queue.empty()) {
-        // We just finished the last item, the queue is going idle.
+      // If we just finished the last item, the queue is going idle.
+      // Loop in case more items get scheduled and completed during OnIdle().
+      // (We treat this as going idle a second time).
+      while (Stat.Active == 1 && Queue.empty() &&
+             Stat.LastIdle != Stat.Completed) {
         assert(ShouldStop || Stat.Completed == Stat.Enqueued);
         Stat.LastIdle = Stat.Completed;
         if (OnIdle) {
Index: clang-tools-extra/clangd/index/Background.h
===================================================================
--- clang-tools-extra/clangd/index/Background.h
+++ clang-tools-extra/clangd/index/Background.h
@@ -100,7 +100,7 @@
   void boost(llvm::StringRef Tag, unsigned NewPriority);
 
   // Process items on the queue until the queue is stopped.
-  // If the queue becomes empty, OnIdle will be called (on one worker).
+  // Each time the queue becomes empty, OnIdle will be called (on one worker).
   void work(std::function<void()> OnIdle = nullptr);
 
   // Stop processing new tasks, allowing all work() calls to return soon.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D90595.302244.patch
Type: text/x-patch
Size: 3264 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201102/2f22ecf4/attachment.bin>