This revision was automatically updated to reflect the committed changes.
Closed by commit rL344741: [clangd] Enforce rules around "initialize"
request, and create ClangdServer… (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D53398
Files:
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
clang-tools-extra/trunk/test/clangd/initialize-sequence.test
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
===================================================================
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h
+++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/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;
}
Index: clang-tools-extra/trunk/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.h
+++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/test/clangd/initialize-sequence.test
===================================================================
--- clang-tools-extra/trunk/test/clangd/initialize-sequence.test
+++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/test/clangd/exit-without-shutdown.test
===================================================================
--- clang-tools-extra/trunk/test/clangd/exit-without-shutdown.test
+++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/test/clangd/exit-with-shutdown.test
===================================================================
--- clang-tools-extra/trunk/test/clangd/exit-with-shutdown.test
+++ clang-tools-extra/trunk/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"}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
