[clang-tools-extra] r367845 - [clangd] Add a callback mechanism for handling responses from client.
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 5 05:48:09 PDT 2019
Author: hokein
Date: Mon Aug 5 05:48:09 2019
New Revision: 367845
URL: http://llvm.org/viewvc/llvm-project?rev=367845&view=rev
Log:
[clangd] Add a callback mechanism for handling responses from client.
Summary:
The callback will be invoked in clangd when we receive a reply from the client.
This is a prerequisite of implementing a generic mechanism for chainable
refactorings (e.g. extract variable and rename), this would allow server to
trigger a new request to the LSP client after receiving a reply from the client.
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D65387
Added:
clang-tools-extra/trunk/clangd/test/request-reply.test
Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/clangd/test/fixits-command.test
Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=367845&r1=367844&r2=367845&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Mon Aug 5 05:48:09 2019
@@ -146,11 +146,39 @@ public:
bool onReply(llvm::json::Value ID,
llvm::Expected<llvm::json::Value> Result) override {
WithContext HandlerContext(handlerContext());
- // We ignore replies, just log them.
- if (Result)
+
+ Callback<llvm::json::Value> ReplyHandler = nullptr;
+ if (auto IntID = ID.getAsInteger()) {
+ std::lock_guard<std::mutex> Mutex(CallMutex);
+ // Find a corresponding callback for the request ID;
+ for (size_t Index = 0; Index < ReplyCallbacks.size(); ++Index) {
+ if (ReplyCallbacks[Index].first == *IntID) {
+ ReplyHandler = std::move(ReplyCallbacks[Index].second);
+ ReplyCallbacks.erase(ReplyCallbacks.begin() +
+ Index); // remove the entry
+ break;
+ }
+ }
+ }
+
+ if (!ReplyHandler) {
+ // No callback being found, use a default log callback.
+ ReplyHandler = [&ID](llvm::Expected<llvm::json::Value> Result) {
+ elog("received a reply with ID {0}, but there was no such call", ID);
+ if (!Result)
+ llvm::consumeError(Result.takeError());
+ };
+ }
+
+ // Log and run the reply handler.
+ if (Result) {
log("<-- reply({0})", ID);
- else
- log("<-- reply({0}) error: {1}", ID, llvm::toString(Result.takeError()));
+ ReplyHandler(std::move(Result));
+ } else {
+ auto Err = Result.takeError();
+ log("<-- reply({0}) error: {1}", ID, Err);
+ ReplyHandler(std::move(Err));
+ }
return true;
}
@@ -171,6 +199,35 @@ public:
};
}
+ // Bind a reply callback to a request. The callback will be invoked when
+ // clangd receives the reply from the LSP client.
+ // Return a call id of the request.
+ llvm::json::Value bindReply(Callback<llvm::json::Value> Reply) {
+ llvm::Optional<std::pair<int, Callback<llvm::json::Value>>> OldestCB;
+ int ID;
+ {
+ std::lock_guard<std::mutex> Mutex(CallMutex);
+ ID = NextCallID++;
+ ReplyCallbacks.emplace_back(ID, std::move(Reply));
+
+ // 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.
+ if (ReplyCallbacks.size() > MaxReplayCallbacks) {
+ elog("more than {0} outstanding LSP calls, forgetting about {1}",
+ MaxReplayCallbacks, ReplyCallbacks.front().first);
+ OldestCB = std::move(ReplyCallbacks.front());
+ ReplyCallbacks.pop_front();
+ }
+ }
+ if (OldestCB)
+ OldestCB->second(llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ llvm::formatv("failed to receive a client reply for request ({0})",
+ OldestCB->first)));
+ return ID;
+ }
+
// Bind an LSP method name to a notification.
template <typename Param>
void bind(const char *Method,
@@ -220,7 +277,13 @@ private:
ReplyOnce &operator=(const ReplyOnce &) = delete;
~ReplyOnce() {
- if (Server && !Replied) {
+ // There's one legitimate reason to never reply to a request: clangd's
+ // request handler send a call to the client (e.g. applyEdit) and the
+ // client never replied. In this case, the ReplyOnce is owned by
+ // ClangdLSPServer's reply callback table and is destroyed along with the
+ // server. We don't attempt to send a reply in this case, there's little
+ // to be gained from doing so.
+ if (Server && !Server->IsBeingDestroyed && !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 +318,16 @@ private:
llvm::StringMap<std::function<void(llvm::json::Value)>> Notifications;
llvm::StringMap<std::function<void(llvm::json::Value, ReplyOnce)>> Calls;
+ // The maximum number of callbacks held in clangd.
+ //
+ // We bound the maximum size to the pending map to prevent memory leakage
+ // for cases where LSP clients don't reply for the request.
+ static constexpr int MaxReplayCallbacks = 100;
+ mutable std::mutex CallMutex;
+ int NextCallID = 0; /* GUARDED_BY(CallMutex) */
+ std::deque<std::pair</*RequestID*/ int,
+ /*ReplyHandler*/ Callback<llvm::json::Value>>>
+ ReplyCallbacks; /* GUARDED_BY(CallMutex) */
// 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
@@ -308,12 +381,13 @@ private:
ClangdLSPServer &Server;
};
+constexpr int ClangdLSPServer::MessageHandler::MaxReplayCallbacks;
// call(), notify(), and reply() wrap the Transport, adding logging and locking.
-void ClangdLSPServer::call(llvm::StringRef Method, llvm::json::Value Params) {
- auto ID = NextCallID++;
+void ClangdLSPServer::callRaw(StringRef Method, llvm::json::Value Params,
+ Callback<llvm::json::Value> CB) {
+ auto ID = MsgHandler->bindReply(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 +570,28 @@ void ClangdLSPServer::onFileEvent(const
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));
};
+ // FIXME: this lambda is tangled and confusing, refactor it.
+ auto ReplyAfterApplyingEdit =
+ [](decltype(Reply) Reply, std::string SuccessMessage,
+ llvm::Expected<ApplyWorkspaceEditResponse> Response) {
+ if (!Response)
+ return Reply(Response.takeError());
+ if (!Response->applied) {
+ std::string Reason = Response->failureReason
+ ? *Response->failureReason
+ : "unknown reason";
+ return Reply(llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ ("edits were not applied: " + Reason).c_str()));
+ }
+ return Reply(SuccessMessage);
+ };
if (Params.command == ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND &&
Params.workspaceEdit) {
// The flow for "apply-fix" :
@@ -511,11 +600,11 @@ void ClangdLSPServer::onCommand(const Ex
// 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(ReplyAfterApplyingEdit, std::move(Reply),
+ std::string("Fix applied.")));
} else if (Params.command == ExecuteCommandParams::CLANGD_APPLY_TWEAK &&
Params.tweakArgs) {
auto Code = DraftMgr.getDraft(Params.tweakArgs->file.file());
@@ -524,9 +613,9 @@ void ClangdLSPServer::onCommand(const Ex
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, ReplyAfterApplyingEdit](
+ decltype(Reply) Reply, URIForFile File, std::string Code,
+ llvm::Expected<Tweak::Effect> R) {
if (!R)
return Reply(R.takeError());
@@ -534,15 +623,16 @@ void ClangdLSPServer::onCommand(const Ex
WorkspaceEdit WE;
WE.changes.emplace();
(*WE.changes)[File.uri()] = replacementsToEdits(Code, *R->ApplyEdit);
- ApplyEdit(std::move(WE));
+ ApplyEdit(std::move(WE), Bind(ReplyAfterApplyingEdit, std::move(Reply),
+ std::string("Tweak applied.")));
}
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 +1141,7 @@ ClangdLSPServer::ClangdLSPServer(
// clang-format on
}
-ClangdLSPServer::~ClangdLSPServer() = default;
+ClangdLSPServer::~ClangdLSPServer() { IsBeingDestroyed = true; }
bool ClangdLSPServer::run() {
// Run the Language Server loop.
Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=367845&r1=367844&r2=367845&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h Mon Aug 5 05:48:09 2019
@@ -133,6 +133,9 @@ private:
/// Language Server client.
bool ShutdownRequestReceived = false;
+ /// Used to indicate the ClangdLSPServer is being destroyed.
+ std::atomic<bool> IsBeingDestroyed = {false};
+
std::mutex FixItsMutex;
typedef std::map<clangd::Diagnostic, std::vector<Fix>, LSPDiagnosticCompare>
DiagnosticToReplacementMap;
@@ -146,9 +149,29 @@ private:
clangd::Transport &Transp;
class MessageHandler;
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 HandleReply = [](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));
+ }
+ };
+ callRaw(Method, std::move(Params),
+ Bind(std::move(HandleReply), std::move(CB)));
+ }
+ void callRaw(StringRef Method, llvm::json::Value Params,
+ Callback<llvm::json::Value> CB);
void notify(StringRef Method, llvm::json::Value Params);
const FileSystemProvider &FSProvider;
Modified: clang-tools-extra/trunk/clangd/Protocol.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Protocol.cpp?rev=367845&r1=367844&r2=367845&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Protocol.cpp (original)
+++ clang-tools-extra/trunk/clangd/Protocol.cpp Mon Aug 5 05:48:09 2019
@@ -670,6 +670,15 @@ llvm::json::Value toJSON(const ApplyWork
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) &&
Modified: clang-tools-extra/trunk/clangd/Protocol.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Protocol.h?rev=367845&r1=367844&r2=367845&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Protocol.h (original)
+++ clang-tools-extra/trunk/clangd/Protocol.h Mon Aug 5 05:48:09 2019
@@ -873,6 +873,12 @@ struct ApplyWorkspaceEditParams {
};
llvm::json::Value toJSON(const ApplyWorkspaceEditParams &);
+struct ApplyWorkspaceEditResponse {
+ bool applied = true;
+ llvm::Optional<std::string> failureReason;
+};
+bool fromJSON(const llvm::json::Value &, ApplyWorkspaceEditResponse &);
+
struct TextDocumentPositionParams {
/// The text document.
TextDocumentIdentifier textDocument;
Modified: clang-tools-extra/trunk/clangd/test/fixits-command.test
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/test/fixits-command.test?rev=367845&r1=367844&r2=367845&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/test/fixits-command.test (original)
+++ clang-tools-extra/trunk/clangd/test/fixits-command.test Mon Aug 5 05:48:09 2019
@@ -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"}
Added: clang-tools-extra/trunk/clangd/test/request-reply.test
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/test/request-reply.test?rev=367845&view=auto
==============================================================================
--- clang-tools-extra/trunk/clangd/test/request-reply.test (added)
+++ clang-tools-extra/trunk/clangd/test/request-reply.test Mon Aug 5 05:48:09 2019
@@ -0,0 +1,43 @@
+# 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: "error": {
+# CHECK-NEXT: "code": -32001,
+# CHECK-NEXT: "message": "edits were not applied: unknown reason"
+# CHECK-NEXT: },
+# CHECK-NEXT: "id": 4,
+---
+{"jsonrpc":"2.0","id":5,"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": 5,
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": "Tweak applied."
+---
+# a reply with an invalid id.
+{"jsonrpc":"2.0","id":"invalid","result":{"applied":true}}
+# clangd doesn't reply, just emits an elog.
+---
+{"jsonrpc":"2.0","id":6,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
More information about the cfe-commits
mailing list