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