From 3d0adbe63664ac4f7d7f6b4d74851a840097de2b Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Thu, 18 Oct 2018 14:41:50 +0000 Subject: [PATCH] [clangd] Enforce rules around "initialize" request, and create ClangdServer lazily. Summary: LSP is a slightly awkward map to C++ object lifetimes: the initialize request is part of the protocol and provides information that doesn't change over the lifetime of the server. Until now, we handled this by initializing ClangdServer and ClangdLSPServer right away, and making anything that can be set in the "initialize" request mutable. With this patch, we create ClangdLSPServer immediately, but defer creating ClangdServer until "initialize". This opens the door to passing the relevant initialize params in the constructor and storing them immutably. (That change isn't actually done in this patch). To make this safe, we have the MessageDispatcher enforce that the "initialize" method is called before any other (as required by LSP). That way each method handler can assume Server is initialized, as today. As usual, while implementing this I found places where our test cases violated the protocol. Reviewers: ioeric Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits Differential Revision: https://reviews.llvm.org/D53398 llvm-svn: 344741 --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 22 +++++++++++++------ clang-tools-extra/clangd/ClangdLSPServer.h | 10 ++++----- clang-tools-extra/clangd/ClangdServer.cpp | 2 +- clang-tools-extra/clangd/ClangdServer.h | 2 +- .../test/clangd/exit-with-shutdown.test | 2 ++ .../test/clangd/exit-without-shutdown.test | 2 ++ .../test/clangd/initialize-sequence.test | 21 ++++++++++++++++++ 7 files changed, 46 insertions(+), 15 deletions(-) create mode 100644 clang-tools-extra/test/clangd/initialize-sequence.test diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 19324174b056..46edf5732dd9 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -87,6 +87,7 @@ CompletionItemKindBitset defaultCompletionItemKinds() { // - logging of inbound messages // - cancellation handling // - basic call tracing +// MessageHandler ensures that initialize() is called before any other handler. class ClangdLSPServer::MessageHandler : public Transport::MessageHandler { public: MessageHandler(ClangdLSPServer &Server) : Server(Server) {} @@ -95,7 +96,9 @@ public: log("<-- {0}", Method); if (Method == "exit") return false; - if (Method == "$/cancelRequest") + if (!Server.Server) + elog("Notification {0} before initialization", Method); + else if (Method == "$/cancelRequest") onCancel(std::move(Params)); else if (auto Handler = Notifications.lookup(Method)) Handler(std::move(Params)); @@ -106,7 +109,11 @@ public: bool onCall(StringRef Method, json::Value Params, json::Value ID) override { log("<-- {0}({1})", Method, ID); - if (auto Handler = Calls.lookup(Method)) + if (!Server.Server && Method != "initialize") { + elog("Call {0} before initialization.", Method); + Server.reply(ID, make_error("server not initialized", + ErrorCode::ServerNotInitialized)); + } else if (auto Handler = Calls.lookup(Method)) Handler(std::move(Params), std::move(ID)); else Server.reply(ID, llvm::make_error("method not found", @@ -254,6 +261,11 @@ void ClangdLSPServer::reply(llvm::json::Value ID, void ClangdLSPServer::onInitialize(const InitializeParams &Params, Callback Reply) { + if (Server) + return Reply(make_error("server already initialized", + ErrorCode::InvalidRequest)); + Server.emplace(CDB.getCDB(), FSProvider, + static_cast(*this), ClangdServerOpts); if (Params.initializationOptions) { const ClangdInitializationOptions &Opts = *Params.initializationOptions; @@ -663,8 +675,7 @@ ClangdLSPServer::ClangdLSPServer(class Transport &Transp, std::move(CompileCommandsDir))), CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()), SupportedCompletionItemKinds(defaultCompletionItemKinds()), - Server(new ClangdServer(CDB.getCDB(), FSProvider, /*DiagConsumer=*/*this, - Opts)) { + ClangdServerOpts(Opts) { // clang-format off MsgHandler->bind("initialize", &ClangdLSPServer::onInitialize); MsgHandler->bind("shutdown", &ClangdLSPServer::onShutdown); @@ -694,8 +705,6 @@ ClangdLSPServer::ClangdLSPServer(class Transport &Transp, ClangdLSPServer::~ClangdLSPServer() = default; bool ClangdLSPServer::run() { - assert(Server); - // Run the Language Server loop. bool CleanExit = true; if (auto Err = Transp.loop(*MsgHandler)) { @@ -705,7 +714,6 @@ bool ClangdLSPServer::run() { // Destroy ClangdServer to ensure all worker threads finish. Server.reset(); - return CleanExit && ShutdownRequestReceived; } diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h index c43a8b2e8c36..c24c770ec8d6 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -183,12 +183,10 @@ private: // Store of the current versions of the open documents. DraftStore DraftMgr; - // Server must be the last member of the class to allow its destructor to exit - // the worker thread that may otherwise run an async callback on partially - // destructed instance of ClangdLSPServer. - // Set in construtor and destroyed when run() finishes. To ensure all worker - // threads exit before run() returns. - std::unique_ptr Server; + // The ClangdServer is created by the "initialize" LSP method. + // It is destroyed before run() returns, to ensure worker threads exit. + ClangdServer::Options ClangdServerOpts; + llvm::Optional Server; }; } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 0dff487ab483..577222c20b65 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -103,7 +103,7 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB, DiagnosticsConsumer &DiagConsumer, const Options &Opts) : CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider), - ResourceDir(Opts.ResourceDir ? Opts.ResourceDir->str() + ResourceDir(Opts.ResourceDir ? *Opts.ResourceDir : getStandardResourceDir()), DynamicIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex(Opts.URISchemes, diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index cf9236068125..400e464a17bb 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -90,7 +90,7 @@ public: /// defaults and -resource-dir compiler flag). /// If None, ClangdServer calls CompilerInvocation::GetResourcePath() to /// obtain the standard resource directory. - llvm::Optional ResourceDir = llvm::None; + llvm::Optional ResourceDir = llvm::None; /// Time to wait after a new file version before computing diagnostics. std::chrono::steady_clock::duration UpdateDebounce = diff --git a/clang-tools-extra/test/clangd/exit-with-shutdown.test b/clang-tools-extra/test/clangd/exit-with-shutdown.test index 41c3b993b476..99e412c7b92c 100644 --- a/clang-tools-extra/test/clangd/exit-with-shutdown.test +++ b/clang-tools-extra/test/clangd/exit-with-shutdown.test @@ -1,4 +1,6 @@ # RUN: clangd -lit-test < %s +{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}} +--- {"jsonrpc":"2.0","id":3,"method":"shutdown"} --- {"jsonrpc":"2.0","method":"exit"} diff --git a/clang-tools-extra/test/clangd/exit-without-shutdown.test b/clang-tools-extra/test/clangd/exit-without-shutdown.test index 106e578b6e24..7b22d0597d62 100644 --- a/clang-tools-extra/test/clangd/exit-without-shutdown.test +++ b/clang-tools-extra/test/clangd/exit-without-shutdown.test @@ -1,2 +1,4 @@ # RUN: not clangd -lit-test < %s +{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}} +--- {"jsonrpc":"2.0","method":"exit"} diff --git a/clang-tools-extra/test/clangd/initialize-sequence.test b/clang-tools-extra/test/clangd/initialize-sequence.test new file mode 100644 index 000000000000..d1b82470281c --- /dev/null +++ b/clang-tools-extra/test/clangd/initialize-sequence.test @@ -0,0 +1,21 @@ +# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s +{"jsonrpc":"2.0","id":0,"method":"workspace/symbol","params":{"query":"std::basic_ostringstream"}} +# CHECK: "error": { +# CHECK-NEXT: "code": -32002 +# CHECK-NEXT: "message": "server not initialized" +# CHECK-NEXT: }, +# CHECK-NEXT: "id": 0, +--- +{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}} +--- +{"jsonrpc":"2.0","id":2,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}} +# CHECK: "error": { +# CHECK-NEXT: "code": -32600 +# CHECK-NEXT: "message": "server already initialized" +# CHECK-NEXT: }, +# CHECK-NEXT: "id": 2, +--- +{"jsonrpc":"2.0","id":3,"method":"shutdown"} +--- +{"jsonrpc":"2.0","method":"exit"} +