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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!



================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:180
+    for (auto &Mod : *Modules)
+      Mod.blockUntilIdle(Deadline::infinity());
+  }
----------------
sammccall wrote:
> 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.
> 
> We need Module::blockUntilIdle anyway for tests and stuff

i was trying to ensure it is *only* used for tests, but i suppose you are right, in practice making `stop` block until in-flight tasks finishes vs stop+blockuntilidle is likely to have same effect, with the latter having the benefit of triggering shut down in parallel. so SGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96755