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
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
