hokein updated this revision to Diff 212794.
hokein marked 2 inline comments as done.
hokein edited the summary of this revision.
hokein added a comment.
Herald added a subscriber: jfb.
- adjust ReplyOnce assertion logic
- add more tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65387/new/
https://reviews.llvm.org/D65387
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/test/fixits-command.test
clang-tools-extra/clangd/test/request-reply.test
Index: clang-tools-extra/clangd/test/request-reply.test
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/test/request-reply.test
@@ -0,0 +1,41 @@
+# RUN: clangd -log=verbose -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"auto i = 0;"}}}
+---
+---
+{"jsonrpc":"2.0","id":4,"method":"workspace/executeCommand","params":{"command":"clangd.applyTweak","arguments":[{"file":"file:///clangd-test/main.cpp","selection":{"end":{"character":4,"line":0},"start":{"character":0,"line":0}},"tweakID":"ExpandAutoType"}]}}
+# CHECK: "id": 0,
+# CHECK: "method": "workspace/applyEdit",
+# CHECK: "newText": "int",
+# CHECK-NEXT: "range": {
+# CHECK-NEXT: "end": {
+# CHECK-NEXT: "character": 4,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT: },
+# CHECK-NEXT: "start": {
+# CHECK-NEXT: "character": 0,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+---
+{"jsonrpc":"2.0","id":0,"result":{"applied":false}}
+# CHECK: "code": -32001,
+# CHECK-NEXT: "message": "edits fail to applied:
+---
+{"jsonrpc":"2.0","id":4,"method":"workspace/executeCommand","params":{"command":"clangd.applyTweak","arguments":[{"file":"file:///clangd-test/main.cpp","selection":{"end":{"character":4,"line":0},"start":{"character":0,"line":0}},"tweakID":"ExpandAutoType"}]}}
+# CHECK: "id": 1,
+# CHECK: "method": "workspace/applyEdit",
+---
+{"jsonrpc":"2.0","id":1,"result":{"applied":true}}
+# CHECK: "id": 4,
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": "Fix applied."
+---
+# a reply with an invalid id.
+{"jsonrpc":"2.0","id":"invalid","result":{"applied":true}}
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
+---
\ No newline at end of file
Index: clang-tools-extra/clangd/test/fixits-command.test
===================================================================
--- clang-tools-extra/clangd/test/fixits-command.test
+++ clang-tools-extra/clangd/test/fixits-command.test
@@ -165,10 +165,6 @@
# CHECK-NEXT: ]
---
{"jsonrpc":"2.0","id":4,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","arguments":[{"changes":{"test:///foo.c":[{"range":{"start":{"line":0,"character":32},"end":{"line":0,"character":32}},"newText":"("},{"range":{"start":{"line":0,"character":37},"end":{"line":0,"character":37}},"newText":")"}]}}]}}
-# CHECK: "id": 4,
-# CHECK-NEXT: "jsonrpc": "2.0",
-# CHECK-NEXT: "result": "Fix applied."
-#
# CHECK: "id": 0,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "method": "workspace/applyEdit",
@@ -207,6 +203,11 @@
# CHECK-NEXT: }
# CHECK-NEXT: }
---
+{"jsonrpc":"2.0","id":0,"result":{"applied":true}}
+# CHECK: "id": 4,
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": "Fix applied."
+---
{"jsonrpc":"2.0","id":4,"method":"shutdown"}
---
{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/Protocol.h
===================================================================
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -873,6 +873,12 @@
};
llvm::json::Value toJSON(const ApplyWorkspaceEditParams &);
+struct ApplyWorkspaceEditResponse {
+ bool applied;
+ llvm::Optional<std::string> failureReason;
+};
+bool fromJSON(const llvm::json::Value &, ApplyWorkspaceEditResponse &);
+
struct TextDocumentPositionParams {
/// The text document.
TextDocumentIdentifier textDocument;
Index: clang-tools-extra/clangd/Protocol.cpp
===================================================================
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -670,6 +670,15 @@
return llvm::json::Object{{"edit", Params.edit}};
}
+bool fromJSON(const llvm::json::Value &Response,
+ ApplyWorkspaceEditResponse &R) {
+ llvm::json::ObjectMapper O(Response);
+ if (!O || !O.map("applied", R.applied))
+ return false;
+ O.map("failureReason", R.failureReason);
+ return true;
+}
+
bool fromJSON(const llvm::json::Value &Params, TextDocumentPositionParams &R) {
llvm::json::ObjectMapper O(Params);
return O && O.map("textDocument", R.textDocument) &&
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -51,6 +51,8 @@
/// \return Whether we shut down cleanly with a 'shutdown' -> 'exit' sequence.
bool run();
+ bool isDestructing() const { return Destructing; }
+
private:
// Implement DiagnosticsConsumer.
void onDiagnosticsReady(PathRef File, std::vector<Diag> Diagnostics) override;
@@ -133,6 +135,8 @@
/// Language Server client.
bool ShutdownRequestReceived = false;
+ std::atomic<bool> Destructing = {false};
+
std::mutex FixItsMutex;
typedef std::map<clangd::Diagnostic, std::vector<Fix>, LSPDiagnosticCompare>
DiagnosticToReplacementMap;
@@ -148,7 +152,27 @@
std::unique_ptr<MessageHandler> MsgHandler;
std::atomic<int> NextCallID = {0};
std::mutex TranspWriter;
- void call(StringRef Method, llvm::json::Value Params);
+
+ template <typename Response>
+ void call(StringRef Method, llvm::json::Value Params, Callback<Response> CB) {
+ // Wrap the callback with LSP conversion and error-handling.
+ auto Reply = [](decltype(CB) CB,
+ llvm::Expected<llvm::json::Value> RawResponse) {
+ Response Rsp;
+ if (!RawResponse) {
+ CB(RawResponse.takeError());
+ } else if (fromJSON(*RawResponse, Rsp)) {
+ CB(std::move(Rsp));
+ } else {
+ elog("Failed to decode {0} response", *RawResponse);
+ CB(llvm::make_error<LSPError>("failed to decode reponse",
+ ErrorCode::InvalidParams));
+ }
+ };
+ callImpl(Method, std::move(Params), Bind(std::move(Reply), std::move(CB)));
+ }
+ void callImpl(StringRef Method, llvm::json::Value Params,
+ Callback<llvm::json::Value> CB);
void notify(StringRef Method, llvm::json::Value Params);
const FileSystemProvider &FSProvider;
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -146,11 +146,46 @@
bool onReply(llvm::json::Value ID,
llvm::Expected<llvm::json::Value> Result) override {
WithContext HandlerContext(handlerContext());
- // We ignore replies, just log them.
- if (Result)
- log("<-- reply({0})", ID);
- else
- log("<-- reply({0}) error: {1}", ID, llvm::toString(Result.takeError()));
+ // Log the reply and run the given callback.
+ auto LogAndRun = [&ID, &Result](Callback<llvm::json::Value> CB) {
+ if (Result) {
+ log("<-- reply({0})", ID);
+ CB(std::move(Result));
+ } else {
+ auto Err = Result.takeError();
+ log("<-- reply({0}) error: {1}", ID, Err);
+ CB(std::move(Err));
+ }
+ };
+
+ Callback<llvm::json::Value> FoundCB = nullptr;
+ if (auto IntID = ID.getAsInteger()) {
+ std::lock_guard<std::mutex> Mutex(ReplyCallbacksMutex);
+ // Find a corresponding callback for the request ID;
+ // We can't use binary search here as ReplyCallbacks is not guaranteed to
+ // be sorted in the multi-thread environment.
+ auto Found =
+ llvm::find_if(ReplyCallbacks, [&IntID](const ReplyCallback &RCB) {
+ return RCB.RequestID == *IntID;
+ });
+ if (Found != ReplyCallbacks.end()) {
+ FoundCB = std::move(Found->CB);
+ ReplyCallbacks.erase(Found); // remove the callback.
+ }
+ }
+
+ if (FoundCB) {
+ // Found a corresponding callback from the queue.
+ LogAndRun(std::move(FoundCB));
+ return true;
+ }
+
+ // No callback being found, run a default log callback.
+ LogAndRun([&ID](decltype(Result) Result) {
+ elog("received a reply with ID {0}, but there was no such call", ID);
+ if (!Result)
+ llvm::consumeError(Result.takeError());
+ });
return true;
}
@@ -171,6 +206,26 @@
};
}
+ // Bind a reply callback to a request.
+ // The callback will be invoked when clangd receives the reply from the LSP
+ // client.
+ void bindReply(int ID, Callback<llvm::json::Value> Reply) {
+ llvm::Optional<ReplyCallback> OldestCB;
+ {
+ std::lock_guard<std::mutex> Mutex(ReplyCallbacksMutex);
+ ReplyCallbacks.emplace_back(ID, std::move(Reply));
+ // take out and run the oldest pending callback if we overflow.
+ if (ReplyCallbacks.size() > MaxCallbacks) {
+ OldestCB = std::move(ReplyCallbacks.front());
+ ReplyCallbacks.pop_front();
+ }
+ }
+ if (OldestCB)
+ OldestCB->CB(
+ llvm::formatv("failed to receive a client reply for request ({0})",
+ OldestCB->RequestID));
+ }
+
// Bind an LSP method name to a notification.
template <typename Param>
void bind(const char *Method,
@@ -220,7 +275,7 @@
ReplyOnce &operator=(const ReplyOnce &) = delete;
~ReplyOnce() {
- if (Server && !Replied) {
+ if (Server && !Server->isDestructing() && !Replied) {
elog("No reply to message {0}({1})", Method, ID);
assert(false && "must reply to all calls!");
(*this)(llvm::make_error<LSPError>("server failed to reply",
@@ -255,6 +310,25 @@
llvm::StringMap<std::function<void(llvm::json::Value)>> Notifications;
llvm::StringMap<std::function<void(llvm::json::Value, ReplyOnce)>> Calls;
+ // The maximum number of callbacks hold in clangd.
+ //
+ // We bound the maximum size to the pending queue to prevent memory leakage
+ // for cases where LSP clients don't reply for the request.
+ //
+ // If the queue overflows, we assume that the client didn't reply the
+ // oldest request, and run the corresponding callback which replies an Error
+ // to the client.
+ static constexpr int MaxCallbacks = 100;
+ mutable std::mutex ReplyCallbacksMutex;
+ // A callback that will be executed when clangd receives a client reply for
+ // a corresponding request.
+ struct ReplyCallback {
+ int RequestID;
+ Callback<llvm::json::Value> CB;
+ ReplyCallback(int ID, Callback<llvm::json::Value> CB)
+ : RequestID(ID), CB(std::move(CB)) {}
+ };
+ std::deque<ReplyCallback> ReplyCallbacks;
// Method calls may be cancelled by ID, so keep track of their state.
// This needs a mutex: handlers may finish on a different thread, and that's
@@ -310,10 +384,11 @@
};
// call(), notify(), and reply() wrap the Transport, adding logging and locking.
-void ClangdLSPServer::call(llvm::StringRef Method, llvm::json::Value Params) {
+void ClangdLSPServer::callImpl(StringRef Method, llvm::json::Value Params,
+ Callback<llvm::json::Value> CB) {
auto ID = NextCallID++;
+ MsgHandler->bindReply(ID, std::move(CB));
log("--> {0}({1})", Method, ID);
- // We currently don't handle responses, so no need to store ID anywhere.
std::lock_guard<std::mutex> Lock(TranspWriter);
Transp.call(Method, std::move(Params), ID);
}
@@ -496,13 +571,26 @@
void ClangdLSPServer::onCommand(const ExecuteCommandParams &Params,
Callback<llvm::json::Value> Reply) {
- auto ApplyEdit = [this](WorkspaceEdit WE) {
+ auto ApplyEdit = [this](WorkspaceEdit WE,
+ Callback<ApplyWorkspaceEditResponse> Reply) {
ApplyWorkspaceEditParams Edit;
Edit.edit = std::move(WE);
- // Ideally, we would wait for the response and if there is no error, we
- // would reply success/failure to the original RPC.
- call("workspace/applyEdit", Edit);
+ call("workspace/applyEdit", std::move(Edit), std::move(Reply));
};
+ auto ReplyApplyEdit =
+ [](decltype(Reply) Reply,
+ llvm::Expected<ApplyWorkspaceEditResponse> Response) {
+ if (!Response)
+ return Reply(Response.takeError());
+ if (!Response->applied) {
+ std::string Reason =
+ Response->failureReason ? *Response->failureReason : "";
+ return Reply(llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ ("edits fail to applied: " + Reason).c_str()));
+ }
+ return Reply("Fix applied.");
+ };
if (Params.command == ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND &&
Params.workspaceEdit) {
// The flow for "apply-fix" :
@@ -511,11 +599,9 @@
// 3. We send code actions, with the fixit embedded as context
// 4. The user selects the fixit, the editor asks us to apply it
// 5. We unwrap the changes and send them back to the editor
- // 6. The editor applies the changes (applyEdit), and sends us a reply (but
- // we ignore it)
-
- Reply("Fix applied.");
- ApplyEdit(*Params.workspaceEdit);
+ // 6. The editor applies the changes (applyEdit), and sends us a reply
+ // 7. We unwrap the reply and send a reply to the editor.
+ ApplyEdit(*Params.workspaceEdit, Bind(ReplyApplyEdit, std::move(Reply)));
} else if (Params.command == ExecuteCommandParams::CLANGD_APPLY_TWEAK &&
Params.tweakArgs) {
auto Code = DraftMgr.getDraft(Params.tweakArgs->file.file());
@@ -524,9 +610,9 @@
llvm::inconvertibleErrorCode(),
"trying to apply a code action for a non-added file"));
- auto Action = [this, ApplyEdit](decltype(Reply) Reply, URIForFile File,
- std::string Code,
- llvm::Expected<Tweak::Effect> R) {
+ auto Action = [this, ApplyEdit, ReplyApplyEdit](
+ decltype(Reply) Reply, URIForFile File, std::string Code,
+ llvm::Expected<Tweak::Effect> R) {
if (!R)
return Reply(R.takeError());
@@ -534,15 +620,15 @@
WorkspaceEdit WE;
WE.changes.emplace();
(*WE.changes)[File.uri()] = replacementsToEdits(Code, *R->ApplyEdit);
- ApplyEdit(std::move(WE));
+ ApplyEdit(std::move(WE), Bind(ReplyApplyEdit, std::move(Reply)));
}
if (R->ShowMessage) {
ShowMessageParams Msg;
Msg.message = *R->ShowMessage;
Msg.type = MessageType::Info;
notify("window/showMessage", Msg);
+ Reply("Tweak applied.");
}
- Reply("Tweak applied.");
};
Server->applyTweak(Params.tweakArgs->file.file(),
Params.tweakArgs->selection, Params.tweakArgs->tweakID,
@@ -1051,7 +1137,7 @@
// clang-format on
}
-ClangdLSPServer::~ClangdLSPServer() = default;
+ClangdLSPServer::~ClangdLSPServer() { Destructing = true; }
bool ClangdLSPServer::run() {
// Run the Language Server loop.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
