[clang-tools-extra] 2d9cfcf - [clangd] Narrow and document a loophole in blockUntilIdle

[Sam McCall via cfe-commits]

Author: Sam McCall
Date: 2021-02-22T23:08:52+01:00
New Revision: 2d9cfcfef029952511462ee45c49c1bf223b9495

URL: https://github.com/llvm/llvm-project/commit/2d9cfcfef029952511462ee45c49c1bf223b9495
DIFF: https://github.com/llvm/llvm-project/commit/2d9cfcfef029952511462ee45c49c1bf223b9495.diff

LOG: [clangd] Narrow and document a loophole in blockUntilIdle

blockUntilIdle of a parent can't always be correctly implemented as
  return ChildA.blockUntilIdle() && ChildB.blockUntilIdle()
The problem is that B can schedule work on A while we're waiting on it.

I believe this is theoretically possible today between CDB and background index.
Modules open more possibilities and it's hard to reason about all of them.

I don't have a perfect fix, and the abstraction is too good to lose. this patch:
 - calls out why we block on workscheduler first, and asserts correctness
 - documents the issue
 - reduces the practical possibility of spuriously returning true significantly

This function is ultimately only for testing, so we're driving down flake rate.

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

Added: 
    

Modified: 
    clang-tools-extra/clangd/ClangdServer.cpp
    clang-tools-extra/clangd/ClangdServer.h

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 67aba1c3d92e..5ec0d816c95f 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -859,10 +859,34 @@ llvm::StringMap<TUScheduler::FileStats> ClangdServer::fileStats() const {
 
 LLVM_NODISCARD bool
 ClangdServer::blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds) {
-  return WorkScheduler.blockUntilIdle(timeoutSeconds(TimeoutSeconds)) &&
-         CDB.blockUntilIdle(timeoutSeconds(TimeoutSeconds)) &&
-         (!BackgroundIdx ||
-          BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
+  // Order is important here: we don't want to block on A and then B,
+  // if B might schedule work on A.
+
+  // Nothing else can schedule work on TUScheduler, because it's not threadsafe
+  // and we're blocking the main thread.
+  if (!WorkScheduler.blockUntilIdle(timeoutSeconds(TimeoutSeconds)))
+    return false;
+
+  // Unfortunately we don't have strict topological order between the rest of
+  // the components. E.g. CDB broadcast triggers backrgound indexing.
+  // This queries the CDB which may discover new work if disk has changed.
+  //
+  // So try each one a few times in a loop.
+  // If there are no tricky interactions then all after the first are no-ops.
+  // Then on the last iteration, verify they're idle without waiting.
+  //
+  // There's a small chance they're juggling work and we didn't catch them :-(
+  for (llvm::Optional<double> Timeout :
+       {TimeoutSeconds, TimeoutSeconds, llvm::Optional<double>(0)}) {
+    if (!CDB.blockUntilIdle(timeoutSeconds(Timeout)))
+      return false;
+    if (BackgroundIdx && !BackgroundIdx->blockUntilIdleForTest(Timeout))
+      return false;
+  }
+
+  assert(WorkScheduler.blockUntilIdle(Deadline::zero()) &&
+         "Something scheduled work while we're blocking the main thread!");
+  return true;
 }
 
 void ClangdServer::profile(MemoryTree &MT) const {

diff  --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index 9581a558ea31..1c09c66e6406 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -335,6 +335,8 @@ class ClangdServer {
 
   // Blocks the main thread until the server is idle. Only for use in tests.
   // Returns false if the timeout expires.
+  // FIXME: various subcomponents each get the full timeout, so it's more of
+  // an order of magnitude than a hard deadline.
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds = 10);