hokein updated this revision to Diff 212586.
hokein marked 11 inline comments as done.
hokein added a comment.
address comments:
- move the reply callback handling code to MessageHandler;
- prevent memory leakage when LSP clients don't send back the reply;
- use the new machanism for applyTweak and apply fix;
- update all relevant 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/code-action-request.test
clang-tools-extra/clangd/test/execute-command.test
clang-tools-extra/clangd/test/fixits-command.test
clang-tools-extra/clangd/test/tweaks-format.test
Index: clang-tools-extra/clangd/test/tweaks-format.test
===================================================================
--- clang-tools-extra/clangd/test/tweaks-format.test
+++ clang-tools-extra/clangd/test/tweaks-format.test
@@ -6,6 +6,7 @@
{"jsonrpc":"2.0","id":5,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///main.cc"},"range":{"start":{"line":0,"character":11},"end":{"line":0,"character":11}},"context":{"diagnostics":[]}}}
---
{"jsonrpc":"2.0","id":6,"method":"workspace/executeCommand","params":{"command":"clangd.applyTweak","arguments":[{"file":"test:///main.cc","selection":{"end":{"character":11,"line":0},"start":{"character":11,"line":0}},"tweakID":"SwapIfBranches"}]}}
+# CHECK: "id": 0,
# CHECK: "newText": "\n ",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
@@ -45,6 +46,8 @@
# CHECK-NEXT: }
# CHECK-NEXT: }
---
+{"jsonrpc":"2.0","id":0,"result":{"applied":true}}
+---
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
---
{"jsonrpc":"2.0","method":"exit"}
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/test/execute-command.test
===================================================================
--- clang-tools-extra/clangd/test/execute-command.test
+++ clang-tools-extra/clangd/test/execute-command.test
@@ -33,6 +33,8 @@
---
{"jsonrpc":"2.0","id":5,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","custom":"foo", "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":")"}]}}]}}
---
+{"jsonrpc":"2.0","id":0,"result":{"applied":false}}
+---
# Arguments not a sequence.
{"jsonrpc":"2.0","id":6,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","arguments":"foo"}}
---
@@ -45,6 +47,8 @@
# Custom field in WorkspaceEdit
{"jsonrpc":"2.0","id":9,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","arguments":[{"custom":"foo", "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":")"}]}}]}}
---
+{"jsonrpc":"2.0","id":1,"result":{"applied":false}}
+---
# changes in WorkspaceEdit with no mapping node
{"jsonrpc":"2.0","id":10,"method":"workspace/executeCommand","params":{"command":"clangd.applyFix","arguments":[{"changes":"foo"}]}}
---
@@ -61,8 +65,10 @@
{"jsonrpc":"2.0","id":14,"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":"","newText":")"}]}}]}}
---
# Command name after arguments
-{"jsonrpc":"2.0","id":9,"method":"workspace/executeCommand","params":{"arguments":[{"custom":"foo", "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":")"}]}}],"command":"clangd.applyFix"}}
+{"jsonrpc":"2.0","id":15,"method":"workspace/executeCommand","params":{"arguments":[{"custom":"foo", "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":")"}]}}],"command":"clangd.applyFix"}}
+---
+{"jsonrpc":"2.0","id":2,"result":{"applied":false}}
---
-{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+{"jsonrpc":"2.0","id":16,"method":"shutdown"}
---
{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/code-action-request.test
===================================================================
--- clang-tools-extra/clangd/test/code-action-request.test
+++ clang-tools-extra/clangd/test/code-action-request.test
@@ -64,7 +64,9 @@
# CHECK-NEXT: }
# CHECK-NEXT: }
---
+{"jsonrpc":"2.0","id":0,"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/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
@@ -146,7 +146,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,41 @@
bool onReply(llvm::json::Value ID,
llvm::Expected<llvm::json::Value> Result) override {
WithContext HandlerContext(handlerContext());
- // We ignore replies, just log them.
- if (Result)
+ // A default "log" callback for cases where there is no callback for the
+ // reply.
+ Callback<llvm::json::Value> CB = [&ID](decltype(Result) Result) {
+ elog("received a reply with ID {0}, but there was no such call", ID);
+ if (!Result)
+ llvm::consumeError(Result.takeError());
+ };
+
+ if (auto IntID = ID.getAsInteger()) {
+ // Find a corresponding callback for the request ID;
+ std::lock_guard<std::mutex> Mutex(ReplyCallbacksMutex);
+ int Index = -1;
+ // We can't use binary search here as ReplyCallbacks is not guaranteed to
+ // be sorted in the multi-thread environment.
+ for (size_t I = 0; I < ReplyCallbacks.size() && Index == -1; ++I) {
+ if (ReplyCallbacks[I].first == *IntID)
+ Index = I;
+ }
+ if (Index != -1) { // found
+ CB = std::move(ReplyCallbacks[Index].second);
+ ReplyCallbacks.erase(ReplyCallbacks.begin() +
+ Index); // remove the entry.
+ }
+ }
+
+ // Log and run the callback.
+ if (Result) {
log("<-- reply({0})", ID);
- else
- log("<-- reply({0}) error: {1}", ID, llvm::toString(Result.takeError()));
+ CB(std::move(Result));
+ } else {
+ auto Err = Result.takeError();
+ log("<-- reply({0}) error: {1}", ID, Err);
+ CB(std::move(Err));
+ }
+
return true;
}
@@ -171,6 +201,17 @@
};
}
+ // Bind a reply to a callback.
+ // The callback will be invoked when clangd receives the reply from LSP
+ // client.
+ void bindReply(int ID, Callback<llvm::json::Value> Reply) {
+ std::lock_guard<std::mutex> Mutex(ReplyCallbacksMutex);
+ ReplyCallbacks.emplace_back(ID, std::move(Reply));
+ // Drop the oldest pending callback if we overflow.
+ if (ReplyCallbacks.size() > MaxCallbacks)
+ ReplyCallbacks.pop_front();
+ }
+
// Bind an LSP method name to a notification.
template <typename Param>
void bind(const char *Method,
@@ -255,6 +296,16 @@
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.
+ //
+ // LSP clients should reply for each request, if the clients don't to that,
+ // clangd will end up memory leakage (the callback will never called and
+ // removed). To prevent it, we bound the maximum size and drop the oldest
+ // pending callback if we overflow.
+ static constexpr int MaxCallbacks = 100;
+ mutable std::mutex ReplyCallbacksMutex;
+ std::deque<std::pair</* request id */ int, Callback<llvm::json::Value>>>
+ 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 +361,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 +548,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 +576,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 +587,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 +597,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,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
