[PATCH] D96856: [clangd] Narrow and document a loophole in blockUntilIdle
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 22 14:11:31 PST 2021
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2d9cfcfef029: [clangd] Narrow and document a loophole in blockUntilIdle (authored by sammccall).
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.325565.patch
Type: text/x-patch
Size: 2472 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210222/24a7074b/attachment.bin>
More information about the cfe-commits
mailing list