[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/TUScheduler.cpp:424
     if (*AST) {
       OnUpdated((*AST)->getDiagnostics());
       trace::Span Span("Running main AST callback");
----------------
as discussed offline, this doesn't guarantee we're not going to send OnUpdated calls for destroyed FileData objects. Intuitively because ShuttingDown may be set e.g. while building the AST, more subtly because atomic only prevents reordering with respect to other atomic operations.

I believe we need to hold a lock:
```
{
  mutex_lock Lock(SendDiagnosticsMu);
  if (SendDiagnostics)
    OnUpdated(...);
}
```
and set SendDiagnostics to false under the same lock.


================
Comment at: clangd/TUScheduler.cpp:426
       trace::Span Span("Running main AST callback");
       Callbacks.onMainAST(FileName, **AST);
       DiagsWereReported = true;
----------------
This patch prevents the AST from being indexed after removal - for the same reasons, right?

But we don't prevent onPreambleAST from being called...
Not sure if it makes sense to fix this, but deserves at least a comment I think.


================
Comment at: clangd/TUScheduler.cpp:515
 
 void ASTWorker::stop() {
+  ShuttingDown = true;
----------------
I think we need to add public docs to TUScheduler::remove and ClangdServer::removeDocument indicating that diagnostics may not be delivered after documents are removed (regardless of WantDiagnostics).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54829