Author: sammccall Date: Thu Oct 18 07:41:50 2018 New Revision: 344741 URL: http://llvm.org/viewvc/llvm-project?rev=344741&view=rev Log: [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 Added: clang-tools-extra/trunk/test/clangd/initialize-sequence.test Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp clang-tools-extra/trunk/clangd/ClangdLSPServer.h clang-tools-extra/trunk/clangd/ClangdServer.cpp clang-tools-extra/trunk/clangd/ClangdServer.h clang-tools-extra/trunk/test/clangd/exit-with-shutdown.test clang-tools-extra/trunk/test/clangd/exit-without-shutdown.test Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=344741&r1=344740&r2=344741&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original) +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Thu Oct 18 07:41:50 2018 @@ -87,6 +87,7 @@ CompletionItemKindBitset defaultCompleti // - 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<LSPError>("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<LSPError>("method not found", @@ -254,6 +261,11 @@ void ClangdLSPServer::reply(llvm::json:: void ClangdLSPServer::onInitialize(const InitializeParams &Params, Callback<json::Value> Reply) { + if (Server) + return Reply(make_error<LSPError>("server already initialized", + ErrorCode::InvalidRequest)); + Server.emplace(CDB.getCDB(), FSProvider, + static_cast<DiagnosticsConsumer &>(*this), ClangdServerOpts); if (Params.initializationOptions) { const ClangdInitializationOptions &Opts = *Params.initializationOptions; @@ -663,8 +675,7 @@ ClangdLSPServer::ClangdLSPServer(class T 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 T 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; } Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=344741&r1=344740&r2=344741&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdLSPServer.h (original) +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h Thu Oct 18 07:41:50 2018 @@ -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<ClangdServer> 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<ClangdServer> Server; }; } // namespace clangd } // namespace clang Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=344741&r1=344740&r2=344741&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original) +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Thu Oct 18 07:41:50 2018 @@ -103,7 +103,7 @@ ClangdServer::ClangdServer(const GlobalC 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, Modified: clang-tools-extra/trunk/clangd/ClangdServer.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=344741&r1=344740&r2=344741&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.h (original) +++ clang-tools-extra/trunk/clangd/ClangdServer.h Thu Oct 18 07:41:50 2018 @@ -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<StringRef> ResourceDir = llvm::None; + llvm::Optional<std::string> ResourceDir = llvm::None; /// Time to wait after a new file version before computing diagnostics. std::chrono::steady_clock::duration UpdateDebounce = Modified: clang-tools-extra/trunk/test/clangd/exit-with-shutdown.test URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/exit-with-shutdown.test?rev=344741&r1=344740&r2=344741&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clangd/exit-with-shutdown.test (original) +++ clang-tools-extra/trunk/test/clangd/exit-with-shutdown.test Thu Oct 18 07:41:50 2018 @@ -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"} Modified: clang-tools-extra/trunk/test/clangd/exit-without-shutdown.test URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/exit-without-shutdown.test?rev=344741&r1=344740&r2=344741&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clangd/exit-without-shutdown.test (original) +++ clang-tools-extra/trunk/test/clangd/exit-without-shutdown.test Thu Oct 18 07:41:50 2018 @@ -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"} Added: clang-tools-extra/trunk/test/clangd/initialize-sequence.test URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/initialize-sequence.test?rev=344741&view=auto ============================================================================== --- clang-tools-extra/trunk/test/clangd/initialize-sequence.test (added) +++ clang-tools-extra/trunk/test/clangd/initialize-sequence.test Thu Oct 18 07:41:50 2018 @@ -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"} + _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
