[clangd] Narrow and document a loophole in blockUntilIdle

[sammccall]

Summary

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.

[kadircet]

LG for the blockUntilIdle update, but feels like two patches wind up together :(

[kadircet]

clang-tools-extra/clangd/ClangdServer.cpp

881

this is extending the Deadline in theory, e.g. if user requested idleness in 10 seconds, this can now wait for up to 20 seconds. but this was possible in the previous case too, e.g. CDB could block for 10 seconds, and then bgindex would block for another 10 seconds, and mentioned this is only for tests, so should be fine (but might be worth mentioning in the comments.)

[sammccall]

Revert unintended changes, add FIXME

[sammccall]

Doh, sorry about the garbled patch

clang-tools-extra/clangd/ClangdServer.cpp

881

Right - the idea with Deadline is it's something absolute you can pass around, and here we are creating a new one for each call.

This is wrong, but it's consistent with the code before this patch and I'd rather not fix both at once. Added a FIXME to the header.

(To fix this we'd want to change the signature of BackgroundIndex to take a Deadline too)

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/ClangdServer.h

public:
   /// here, as this metric does not account (at least) for:
   ///   - memory occupied by static and dynamic index,
   ///   - memory required for in-flight requests,
   /// FIXME: those metrics might be useful too, we should add them.
   llvm::StringMap<TUScheduler::FileStats> fileStats() const;
 
   // 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);
 
   /// Builds a nested representation of memory used by components.
   void profile(MemoryTree &MT) const;
 
 private:
   void formatCode(PathRef File, llvm::StringRef Code,

clang-tools-extra/clangd/ClangdServer.cpp

 }
 
 llvm::StringMap<TUScheduler::FileStats> ClangdServer::fileStats() const {
   return WorkScheduler.fileStats();
 }
 
 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 {
   if (DynamicIdx)
     DynamicIdx->profile(MT.child("dynamic_index"));
   if (BackgroundIdx)
     BackgroundIdx->profile(MT.child("background_index"));
   WorkScheduler.profile(MT.child("tuscheduler"));
 }
 } // namespace clangd
 } // namespace clang