[clang-tools-extra] r313754 - [clangd] Serialize onDiagnosticsReady callbacks for the same file.

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Wed Sep 20 05:58:55 2017
New Revision: 313754

URL: http://llvm.org/viewvc/llvm-project?rev=313754&view=rev
Log:
[clangd] Serialize onDiagnosticsReady callbacks for the same file.

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.

Reviewers: klimek, bkramer, krasimir

Reviewed By: klimek

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D38032

Modified:
    clang-tools-extra/trunk/clangd/ClangdServer.cpp
    clang-tools-extra/trunk/clangd/ClangdServer.h
    clang-tools-extra/trunk/clangd/DraftStore.h
    clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=313754&r1=313753&r2=313754&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Wed Sep 20 05:58:55 2017
@@ -315,6 +315,19 @@ std::future<void> ClangdServer::schedule
     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));
   };

Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=313754&r1=313753&r2=313754&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.h Wed Sep 20 05:58:55 2017
@@ -284,6 +284,13 @@ private:
   // 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

Modified: clang-tools-extra/trunk/clangd/DraftStore.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/DraftStore.h?rev=313754&r1=313753&r2=313754&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/DraftStore.h (original)
+++ clang-tools-extra/trunk/clangd/DraftStore.h Wed Sep 20 05:58:55 2017
@@ -13,6 +13,7 @@
 #include "Path.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/StringMap.h"
+#include <cstdint>
 #include <mutex>
 #include <string>
 #include <vector>
@@ -20,8 +21,8 @@
 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 {

Modified: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp?rev=313754&r1=313753&r2=313754&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp Wed Sep 20 05:58:55 2017
@@ -22,6 +22,7 @@
 #include <iostream>
 #include <random>
 #include <string>
+#include <thread>
 #include <vector>
 
 namespace clang {
@@ -898,5 +899,67 @@ int d;
   }
 }
 
+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