[PATCH] D79302: [clangd] Propogate context in LSPServer tests

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.

This enables users to wait until async request completes. Taking a
response isn't enough, as receiving a response doesn't imply destruction of the
reqeust context.

This also fixes the raciness in lspserver latency metric test. As it needs to
wait for context destruction, rather than receiving a reply. Happy to perform
that fix in CallResult instead, i.e change `LSPClient::call` to perform the
insertion of Notification into context and change `CallResult::take` to wait for
destruction of context rather than receiving a reply(calling set).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79302

Files:
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/LSPClient.cpp


Index: clang-tools-extra/clangd/unittests/LSPClient.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/LSPClient.cpp
+++ clang-tools-extra/clangd/unittests/LSPClient.cpp
@@ -5,6 +5,7 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "Transport.h"
+#include "support/Context.h"
 #include "support/Threading.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
@@ -68,7 +69,7 @@
   // A null action causes the transport to shut down.
   void enqueue(std::function<void(MessageHandler &)> Action) {
     std::lock_guard<std::mutex> Lock(Mu);
-    Actions.push(std::move(Action));
+    Requests.push({Context::current().clone(), std::move(Action)});
     CV.notify_all();
   }
 
@@ -112,20 +113,27 @@
   llvm::Error loop(MessageHandler &H) override {
     std::unique_lock<std::mutex> Lock(Mu);
     while (true) {
-      CV.wait(Lock, [&] { return !Actions.empty(); });
-      if (!Actions.front()) // Stop!
+      CV.wait(Lock, [&] { return !Requests.empty(); });
+      if (!Requests.front().Action) // Stop!
         return llvm::Error::success();
-      auto Action = std::move(Actions.front());
-      Actions.pop();
+      auto Req = std::move(Requests.front());
+      // Leave request on the queue so that waiters can see it.
       Lock.unlock();
-      Action(H);
+      WithContext Ctx(std::move(Req.Ctx));
+      Req.Action(H);
       Lock.lock();
+      Requests.pop();
     }
   }
 
+  struct Request {
+    Context Ctx;
+    std::function<void(Transport::MessageHandler &)> Action;
+  };
+
   std::mutex Mu;
   std::deque<CallResult> CallResults;
-  std::queue<std::function<void(Transport::MessageHandler &)>> Actions;
+  std::queue<Request> Requests;
   std::condition_variable CV;
   llvm::StringMap<std::vector<llvm::json::Value>> Notifications;
 };
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -13,8 +13,11 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "refactor/Rename.h"
+#include "support/Context.h"
 #include "support/Logger.h"
 #include "support/TestTracer.h"
+#include "support/Threading.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
@@ -149,6 +152,21 @@
               llvm::ValueIs(testing::ElementsAre(
                   DiagMessage("Use of undeclared identifier 'changed'"))));
 }
+
+TEST_F(LSPTest, RecordsLatencies) {
+  trace::TestTracer Tracer;
+  auto &Client = start();
+  llvm::StringLiteral MethodName = "method_name";
+  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), testing::SizeIs(0));
+  Notification CallFinished;
+  {
+    WithContextValue Ctx(
+        llvm::make_scope_exit([&CallFinished] { CallFinished.notify(); }));
+    llvm::consumeError(Client.call(MethodName, {}).take().takeError());
+  }
+  CallFinished.wait();
+  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), testing::SizeIs(1));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D79302.261688.patch
Type: text/x-patch
Size: 3232 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200503/142c4f9b/attachment.bin>