[PATCH] D96856: [clangd] Narrow and document a loophole in blockUntilIdle

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 324609.
sammccall added a comment.

Revert unintended changes, add FIXME


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96856

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


Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -335,6 +335,8 @@
 
   // 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);
 
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -859,10 +859,34 @@
 
 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 {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D96856.324609.patch
Type: text/x-patch
Size: 2472 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210218/6173ec8c/attachment.bin>