[PATCH] D129100: [clangd] Support external throttler for preamble builds

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1499
+
+    S.remove(Filenames.back());
+    // Now shut down the TU Scheduler.
----------------
kadircet wrote:
> sequencing is hard here but it'd be nice to ensure release is actually seen.
So this turned out to be the cause of the test flakiness: the mechanism we discussed offline (context destruction) doesn't work.

The problem is that the context lives in the request, and we haven't moved the request from NextReq into CurrentReq yet (we're still throttling).
NextReq is destroyed when stop() is called. So S.remove(A) actually triggers `AfterFinishA` already, and then the following assertions race against the release() call.

I probably could have gotten this out of our tsan bot, but it's not producing any sanitizer logs (I suspect D122251).

In any case, this particular race is definitely only in the test, so disabling it until we have a real fix next week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129100