[PATCH] D96755: [clangd] Shutdown sequence for modules, and doc threading requirements

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:180
+    for (auto &Mod : *Modules)
+      Mod.blockUntilIdle(Deadline::infinity());
+  }
----------------
kadircet wrote:
> why is our contract saying that just calling `stop` is not enough?
> i think clangdserver should just signal shutdown to modules, and our contract should say that server facilities will be undefined from this point forward.
> that way modules accessing the facilities, could block stop until they are done, and never make use of it afterwards? it'll make modules a little more complicated, at the very least they would need some stricter control whenever they are accessing facilities, but I think it is worth for keeping clangdserver shutdown cleaner. wdyt?
We need Module::blockUntilIdle anyway for tests and stuff. And in practice requesting shutdown of background work doesn't naturally block until that work is complete IME.

So we could ask every module to call blockUntilIdle() in stop(), or make stop() non-virtual and have it wrap requestStop()+blockUntilIdle().

Neither really seems simpler/cleaner to me overall, and it means modules shut down in serial instead of parallel.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96755