[clangd] Serialize onDiagnosticsReady callbacks for the same file.

[ilya-biryukov]

Summary

Calls to onDiagnosticsReady were done concurrently before. This sometimes
led to older versions of diagnostics being reported to the user after
the newer versions.

[ilya-biryukov]

• Removed an incidental Mutex.unlock(), a leftover from previous drafts.

[klimek]

clangd/ClangdServer.cpp

321–324 ↗

(On Diff #115821)

Why is this a FIXME? Do we intend to use a different mechanism? 2^64 versions of a file seem to be a lot?

unittests/clangd/ClangdTests.cpp

912 ↗

(On Diff #115821)

Why not hand in a signal to wait for?

[ilya-biryukov]

clangd/ClangdServer.cpp

321–324 ↗

(On Diff #115821)

I think we can get rid of it after refactoring threading. But we should definitely be fine with 2^64 versions.

unittests/clangd/ClangdTests.cpp

912 ↗

(On Diff #115821)

The signal should be fired on a second call to onDiagnosticsReady, but the second call won't happen before the fist one returns.
Is there some other way to fire the signal that I'm missing?

[klimek]

LG

clangd/ClangdServer.h

287 ↗

(On Diff #115821)

Comment what it maps from.

unittests/clangd/ClangdTests.cpp

912 ↗

(On Diff #115821)

Thinking a bit more about it, I can't find a better way.

[ilya-biryukov]

• Added a comment to version map.
• Fixed compilation after rebase.

[ilya-biryukov]

Thanks for the review!

clang-tools-extra/trunk/clangd/ClangdServer.h

private:
   CppFileCollection Units;
   std::string ResourceDir;
   std::shared_ptr<PCHContainerOperations> PCHs;
   // WorkScheduler has to be the last member, because its destructor has to be
   // called before all other members to stop the worker thread that references
   // ClangdServer
   ClangdScheduler WorkScheduler;
   bool SnippetCompletions;
+
+  /// Used to serialize diagnostic callbacks.
+  /// FIXME(ibiryukov): get rid of an extra map and put all version counters
+  /// into CppFile.
+  std::mutex DiagnosticsMutex;
+  /// Maps from a filename to the latest version of reported diagnostics.
+  llvm::StringMap<DocVersion> ReportedDiagnosticVersions;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/trunk/clangd/ClangdServer.cpp

auto ReparseAndPublishDiags =
 
     auto CurrentVersion = DraftMgr.getVersion(FileStr);
     if (CurrentVersion != Version)
       return; // This request is outdated
 
     auto Diags = DeferredRebuild.get();
     if (!Diags)
       return; // A new reparse was requested before this one completed.
+
+    // We need to serialize access to resulting diagnostics to avoid calling
+    // `onDiagnosticsReady` in the wrong order.
+    std::lock_guard<std::mutex> DiagsLock(DiagnosticsMutex);
+    DocVersion &LastReportedDiagsVersion = ReportedDiagnosticVersions[FileStr];
+    // FIXME(ibiryukov): get rid of '<' comparison here. In the current
+    // implementation diagnostics will not be reported after version counters'
+    // overflow. This should not happen in practice, since DocVersion is a
+    // 64-bit unsigned integer.
+    if (Version < LastReportedDiagsVersion)
+      return;
+    LastReportedDiagsVersion = Version;
+
     DiagConsumer.onDiagnosticsReady(FileStr,
                                     make_tagged(std::move(*Diags), Tag));
   };
 
   WorkScheduler.addToFront(std::move(ReparseAndPublishDiags),
                            std::move(DeferredRebuild), std::move(DonePromise));
   return DoneFuture;
 }

clang-tools-extra/trunk/clangd/DraftStore.h

 //===--- DraftStore.h - File contents container -----------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_DRAFTSTORE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_DRAFTSTORE_H
 
 #include "Path.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringMap.h"
+#include <cstdint>
 #include <mutex>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
-/// Using 'unsigned' here to avoid undefined behaviour on overflow.
-typedef unsigned DocVersion;
+/// Using unsigned int type here to avoid undefined behaviour on overflow.
+typedef uint64_t DocVersion;
 
 /// Document draft with a version of this draft.
 struct VersionedDraft {
   DocVersion Version;
   /// If the value of the field is None, draft is now deleted
   llvm::Optional<std::string> Draft;
 };
 

clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "gtest/gtest.h"
 #include <algorithm>
 #include <chrono>
 #include <iostream>
 #include <random>
 #include <string>
+#include <thread>
 #include <vector>
 
 namespace clang {
 namespace vfs {
 
 /// An implementation of vfs::FileSystem that only allows access to
 /// files and folders inside a set of whitelisted directories.
 ///
if (!ReqStats[I].FileIsRemoved) {
                 ReqStats[I].LastContentsHadErrors);
     }
 
     ASSERT_LE(Stats[I].HitsWithErrors, ReqStats[I].RequestsWithErrors);
     ASSERT_LE(Stats[I].HitsWithoutErrors, ReqStats[I].RequestsWithoutErrors);
   }
 }
 
+TEST_F(ClangdThreadingTest, NoConcurrentDiagnostics) {
+  class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
+  public:
+    NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse)
+        : StartSecondReparse(std::move(StartSecondReparse)) {}
+
+    void onDiagnosticsReady(
+        PathRef File,
+        Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+
+      std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock_t());
+      ASSERT_TRUE(Lock.owns_lock())
+          << "Detected concurrent onDiagnosticsReady calls for the same file.";
+      if (FirstRequest) {
+        FirstRequest = false;
+        StartSecondReparse.set_value();
+        // Sleep long enough for the second request to be processed.
+        std::this_thread::sleep_for(std::chrono::milliseconds(50));
+      }
+    }
+
+  private:
+    std::mutex Mutex;
+    bool FirstRequest = true;
+    std::promise<void> StartSecondReparse;
+  };
+
+  const auto SourceContentsWithoutErrors = R"cpp(
+int a;
+int b;
+int c;
+int d;
+)cpp";
+
+  const auto SourceContentsWithErrors = R"cpp(
+int a = x;
+int b;
+int c;
+int d;
+)cpp";
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  llvm::StringMap<std::string> FileContents;
+  FileContents[FooCpp] = "";
+  ConstantFSProvider FS(buildTestFS(FileContents));
+
+  std::promise<void> StartSecondReparsePromise;
+  std::future<void> StartSecondReparse = StartSecondReparsePromise.get_future();
+
+  NoConcurrentAccessDiagConsumer DiagConsumer(
+      std::move(StartSecondReparsePromise));
+
+  MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true);
+  ClangdServer Server(CDB, DiagConsumer, FS, 4, /*SnippetCompletions=*/false,
+                      EmptyLogger::getInstance());
+  Server.addDocument(FooCpp, SourceContentsWithErrors);
+  StartSecondReparse.wait();
+
+  auto Future = Server.addDocument(FooCpp, SourceContentsWithoutErrors);
+  Future.wait();
+}
+
 } // namespace clangd
 } // namespace clang