This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ilya-biryukov on Nov 22 2018, 2:32 AM.

Details

Summary

Previously, removeDoc followed by an addDoc to TUScheduler resulted in
racy diagnostic responses, i.e. the old dianostics could be delivered
to the client after the new ones by TUScheduler.

To workaround this, we tracked a version number in ClangdServer and
discarded stale diagnostics. After this commit, the TUScheduler will
stop delivering diagnostics for removed files and the workaround in
ClangdServer is not required anymore.

Event Timeline

ilya-biryukov created this revision.Nov 22 2018, 2:32 AM
ilya-biryukov added inline comments.Nov 22 2018, 2:39 AM
clangd/TUScheduler.cpp
241

Note to myself: should reuse Done instead by making turning it into atomic<bool>

sammccall added inline comments.Nov 22 2018, 3:28 AM
clangd/TUScheduler.cpp
432–436

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.

438

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.

527

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).

  • Ensure consistency for diagnostic callbacks with a critical section.
ilya-biryukov marked 3 inline comments as done.
  • Added comments
ilya-biryukov added inline comments.Nov 22 2018, 5:24 AM
clangd/TUScheduler.cpp
432–436

Done. I've made the change before looking at the comment, so using ReportDiagnostics name instead of SendDiagnostics. Happy to change if you like the latter more.

438

Added a comment.

sammccall accepted this revision.Nov 22 2018, 5:50 AM
sammccall added inline comments.
clangd/ClangdServer.h
132

I'm not sure we actually want to *guarantee* this in the API: this implementation serializes removeDocument and the diagnostics callback, but only because it was the most convenient.

I'd suggest just "pending diagnostics for closed files may not be delivered". (and similarly for TUScheduler).

Up to you though.

clangd/TUScheduler.cpp
256

I think the second sentence is a bit obvious and doesn't get into the subtlety here. Maybe:

Used to prevent remove document + leading to out-of-order diagnostics:
The lifetime of the old/new ASTWorkers will overlap, but their handles don't.
When the old handle is destroyed, the old worker will stop reporting diagnostics.
537

Done is covered by RequestsCV, and ReportDiagnostics is not, it seems a little weird to have the diags critical section in between here.

(I don't have an opinion about the relative order of the sections, but notify_all should immediately follow the Mutex section for clarity, I think)

This revision is now accepted and ready to land.Nov 22 2018, 5:50 AM
ilya-biryukov marked 3 inline comments as done.
  • Address the comments
clangd/ClangdServer.h
132

Yeah, it might be a bit stronger than what we guaranteed before (which was "you won't get stale diagnostics for a file that you reopen")

This revision was automatically updated to reflect the committed changes.