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 @@ -133,6 +133,9 @@ /// Language Server client. bool ShutdownRequestReceived = false; + /// Used to indicate the ClangdLSPServer is being destroyed. + std::atomic IsBeingDestroyed = {false}; + std::mutex FixItsMutex; typedef std::map, LSPDiagnosticCompare> DiagnosticToReplacementMap; @@ -146,9 +149,29 @@ clangd::Transport &Transp; class MessageHandler; 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 HandleReply = [](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)); + } + }; + callRaw(Method, std::move(Params), + Bind(std::move(HandleReply), std::move(CB))); + } + void callRaw(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,39 @@ bool onReply(llvm::json::Value ID, llvm::Expected Result) override { WithContext HandlerContext(handlerContext()); - // We ignore replies, just log them. - if (Result) + + Callback ReplyHandler = nullptr; + if (auto IntID = ID.getAsInteger()) { + std::lock_guard 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 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,36 @@ }; } + // 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. + int bindReply(Callback Reply) { + llvm::Optional>> OldestCB; + int ID = -1; + { + std::lock_guard 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() > MaxCallbacks) { + elog("more than {0} outstanding LSP calls, forgetting about {1}", + MaxCallbacks, 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))); + assert(ID != -1 && "ID must be set"); + return ID; + } + // Bind an LSP method name to a notification. template void bind(const char *Method, @@ -220,7 +278,8 @@ ReplyOnce &operator=(const ReplyOnce &) = delete; ~ReplyOnce() { - if (Server && !Replied) { + // If the server is being destroyed, we don't attempt to reply calls. + 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("server failed to reply", @@ -255,6 +314,16 @@ llvm::StringMap> Notifications; llvm::StringMap> Calls; + // The maximum number of callbacks hold 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 MaxCallbacks = 100; + mutable std::mutex CallMutex; + int NextCallID = 0; /* GUARDED_BY(CallMutex) */ + std::deque>> + 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 @@ -309,11 +378,13 @@ ClangdLSPServer &Server; }; +constexpr int ClangdLSPServer::MessageHandler::MaxCallbacks; + // 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 CB) { + int 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 Lock(TranspWriter); Transp.call(Method, std::move(Params), ID); } @@ -496,13 +567,28 @@ 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)); }; + // FIXME: this lambda is tangled and confusing, refactor it. + auto ReplyAfterApplyingEdit = + [](decltype(Reply) Reply, std::string SuccessMessage, + 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 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 +597,11 @@ // 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 +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 R) { + auto Action = [this, ApplyEdit, ReplyAfterApplyingEdit]( + decltype(Reply) Reply, URIForFile File, std::string Code, + llvm::Expected R) { if (!R) return Reply(R.takeError()); @@ -534,15 +620,16 @@ 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 +1138,7 @@ // clang-format on } -ClangdLSPServer::~ClangdLSPServer() = default; +ClangdLSPServer::~ClangdLSPServer() { IsBeingDestroyed = 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 = true; + 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,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":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": "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":5,"method":"shutdown"} +--- +{"jsonrpc":"2.0","method":"exit"}