diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/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 Diagnostics) override; @@ -133,6 +135,8 @@ /// Language Server client. bool ShutdownRequestReceived = false; + std::atomic Destructing = {false}; + std::mutex FixItsMutex; typedef std::map, LSPDiagnosticCompare> DiagnosticToReplacementMap; @@ -148,7 +152,27 @@ std::unique_ptr MsgHandler; std::atomic NextCallID = {0}; std::mutex TranspWriter; - void call(StringRef Method, llvm::json::Value Params); + + template + void call(StringRef Method, llvm::json::Value Params, Callback CB) { + // Wrap the callback with LSP conversion and error-handling. + auto Reply = [](decltype(CB) CB, + llvm::Expected 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("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 CB); void notify(StringRef Method, llvm::json::Value Params); const FileSystemProvider &FSProvider; diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -146,11 +146,46 @@ bool onReply(llvm::json::Value ID, llvm::Expected 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 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 FoundCB = nullptr; + if (auto IntID = ID.getAsInteger()) { + std::lock_guard 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 Reply) { + llvm::Optional OldestCB; + { + std::lock_guard 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 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("server failed to reply", @@ -255,6 +310,25 @@ llvm::StringMap> Notifications; llvm::StringMap> 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 CB; + ReplyCallback(int ID, Callback CB) + : RequestID(ID), CB(std::move(CB)) {} + }; + std::deque 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 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 Lock(TranspWriter); Transp.call(Method, std::move(Params), ID); } @@ -496,13 +571,27 @@ void ClangdLSPServer::onCommand(const ExecuteCommandParams &Params, Callback Reply) { - auto ApplyEdit = [this](WorkspaceEdit WE) { + auto ApplyEdit = [this](WorkspaceEdit WE, + Callback 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 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 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 +600,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 +611,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 R) { + auto Action = [this, ApplyEdit, ReplyApplyEdit]( + decltype(Reply) Reply, URIForFile File, std::string Code, + llvm::Expected R) { if (!R) return Reply(R.takeError()); @@ -534,15 +621,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 +1138,7 @@ // clang-format on } -ClangdLSPServer::~ClangdLSPServer() = default; +ClangdLSPServer::~ClangdLSPServer() { Destructing = true; } bool ClangdLSPServer::run() { // Run the Language Server loop. diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -873,6 +873,12 @@ }; llvm::json::Value toJSON(const ApplyWorkspaceEditParams &); +struct ApplyWorkspaceEditResponse { + bool applied; + llvm::Optional failureReason; +}; +bool fromJSON(const llvm::json::Value &, ApplyWorkspaceEditResponse &); + struct TextDocumentPositionParams { /// The text document. TextDocumentIdentifier textDocument; diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/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) && diff --git a/clang-tools-extra/clangd/test/fixits-command.test b/clang-tools-extra/clangd/test/fixits-command.test --- a/clang-tools-extra/clangd/test/fixits-command.test +++ b/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"} diff --git a/clang-tools-extra/clangd/test/request-reply.test b/clang-tools-extra/clangd/test/request-reply.test new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clangd/test/request-reply.test @@ -0,0 +1,40 @@ +# 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"}