sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ilya-biryukov.
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53398
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
test/clangd/exit-with-shutdown.test
test/clangd/exit-without-shutdown.test
test/clangd/initialize-sequence.test
Index: test/clangd/initialize-sequence.test
===================================================================
--- /dev/null
+++ 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"}
+
Index: test/clangd/exit-without-shutdown.test
===================================================================
--- test/clangd/exit-without-shutdown.test
+++ 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"}
Index: test/clangd/exit-with-shutdown.test
===================================================================
--- test/clangd/exit-with-shutdown.test
+++ 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"}
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -90,7 +90,7 @@
/// 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 =
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -103,7 +103,7 @@
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,
Index: clangd/ClangdLSPServer.h
===================================================================
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -183,12 +183,10 @@
// 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
Index: clangd/ClangdLSPServer.cpp
===================================================================
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -87,15 +87,18 @@
// - 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) {}
bool onNotify(StringRef Method, json::Value Params) override {
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 @@
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::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 @@
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() = default;
bool ClangdLSPServer::run() {
- assert(Server);
-
// Run the Language Server loop.
bool CleanExit = true;
if (auto Err = Transp.loop(*MsgHandler)) {
@@ -705,7 +714,6 @@
// Destroy ClangdServer to ensure all worker threads finish.
Server.reset();
-
return CleanExit && ShutdownRequestReceived;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
