[clang-tools-extra] r344741 - [clangd] Enforce rules around "initialize" request, and create ClangdServer lazily.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 18 07:41:51 PDT 2018
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"}
+
More information about the cfe-commits
mailing list