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); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
