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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:180
+    for (auto &Mod : *Modules)
+      Mod.blockUntilIdle(Deadline::infinity());
+  }
----------------
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?


================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:261
+  class AsyncCounter : public Module {
+    bool ShouldStop;
+    int State = 0;
----------------
` = false;`


================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:349
+  ASSERT_TRUE(One && Two && Three) << "sync should wait for pending requests";
+  EXPECT_EQ(3, Counter->getSync()) << "sync avoids flaky tests";
+  // Sanity check: reads and writes are sequenced on the worker thread.
----------------
not sure what this is testing in addition to final callback receiving 3 as a value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96755