From 0d9b8a3ee8df63fdd9cd8428ac74df0acb8afca7 Mon Sep 17 00:00:00 2001 From: Ilya Biryukov Date: Wed, 25 Oct 2017 08:45:41 +0000 Subject: [PATCH] [clangd] Handle exit notification (proper shutdown) Summary: This changes the onShutdown handler to do essentially nothing (for now), and instead exits the runloop when we receive the exit notification from the client. Some clients may wait on the reply from the shutdown request before sending an exit notification. If we exit the runloop already in the shutdown request, a client might block forever. This also gives us the opportunity to do any global cleanups and/or serializations of PCH preambles to disk, but I've left that out for now. See the LSP protocol documentation for details. Reviewers: malaperle, krasimir, bkramer, sammccall, ilya-biryukov Reviewed By: malaperle, sammccall, ilya-biryukov Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D38939 llvm-svn: 316564 --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 10 ++++++++-- clang-tools-extra/clangd/ClangdLSPServer.h | 9 ++++++++- clang-tools-extra/clangd/Protocol.h | 1 + clang-tools-extra/clangd/ProtocolHandlers.cpp | 1 + clang-tools-extra/clangd/ProtocolHandlers.h | 1 + clang-tools-extra/clangd/tool/ClangdMain.cpp | 4 +++- clang-tools-extra/test/clangd/authority-less-uri.test | 4 ++++ clang-tools-extra/test/clangd/completion-priorities.test | 4 ++++ clang-tools-extra/test/clangd/completion-qualifiers.test | 4 ++++ clang-tools-extra/test/clangd/completion-snippet.test | 4 ++++ clang-tools-extra/test/clangd/completion.test | 4 ++++ clang-tools-extra/test/clangd/definitions.test | 4 ++++ clang-tools-extra/test/clangd/diagnostics-preamble.test | 4 ++++ clang-tools-extra/test/clangd/diagnostics.test | 4 ++++ clang-tools-extra/test/clangd/did-change-watch-files.test | 3 +++ clang-tools-extra/test/clangd/extra-flags.test | 4 ++++ clang-tools-extra/test/clangd/fixits.test | 4 ++++ clang-tools-extra/test/clangd/formatting.test | 4 ++++ clang-tools-extra/test/clangd/initialize-params-invalid.test | 4 ++++ clang-tools-extra/test/clangd/initialize-params.test | 4 ++++ clang-tools-extra/test/clangd/input-mirror.test | 4 ++++ clang-tools-extra/test/clangd/protocol.test | 6 ++++-- clang-tools-extra/test/clangd/shutdown-with-exit.test | 9 +++++++++ clang-tools-extra/test/clangd/shutdown-without-exit.test | 6 ++++++ clang-tools-extra/test/clangd/signature-help.test | 4 ++++ clang-tools-extra/test/clangd/unsupported-method.test | 4 ++++ 26 files changed, 108 insertions(+), 6 deletions(-) create mode 100644 clang-tools-extra/test/clangd/shutdown-with-exit.test create mode 100644 clang-tools-extra/test/clangd/shutdown-without-exit.test diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 96e2296..ca95fd8 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -56,9 +56,13 @@ void ClangdLSPServer::onInitialize(Ctx C, InitializeParams &Params) { } void ClangdLSPServer::onShutdown(Ctx C, ShutdownParams &Params) { - IsDone = true; + // Do essentially nothing, just say we're ready to exit. + ShutdownRequestReceived = true; + C.reply("null"); } +void ClangdLSPServer::onExit(Ctx C, ExitParams &Params) { IsDone = true; } + void ClangdLSPServer::onDocumentDidOpen(Ctx C, DidOpenTextDocumentParams &Params) { if (Params.metadata && !Params.metadata->extraFlags.empty()) @@ -197,7 +201,7 @@ ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount, /*EnableSnippetsAndCodePatterns=*/SnippetCompletions), /*Logger=*/Out, ResourceDir) {} -void ClangdLSPServer::run(std::istream &In) { +bool ClangdLSPServer::run(std::istream &In) { assert(!IsDone && "Run was called before"); // Set up JSONRPCDispatcher. @@ -213,6 +217,8 @@ void ClangdLSPServer::run(std::istream &In) { // Make sure IsDone is set to true after this method exits to ensure assertion // at the start of the method fires if it's ever executed again. IsDone = true; + + return ShutdownRequestReceived; } std::vector diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h index ebddd6b..12533c9 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -39,7 +39,9 @@ public: /// opened in binary mode. Output will be written using Out variable passed to /// class constructor. This method must not be executed more than once for /// each instance of ClangdLSPServer. - void run(std::istream &In); + /// + /// \return Wether we received a 'shutdown' request before an 'exit' request + bool run(std::istream &In); private: // Implement DiagnosticsConsumer. @@ -50,6 +52,7 @@ private: // Implement ProtocolCallbacks. void onInitialize(Ctx C, InitializeParams &Params) override; void onShutdown(Ctx C, ShutdownParams &Params) override; + void onExit(Ctx C, ExitParams &Params) override; void onDocumentDidOpen(Ctx C, DidOpenTextDocumentParams &Params) override; void onDocumentDidChange(Ctx C, DidChangeTextDocumentParams &Params) override; void onDocumentDidClose(Ctx C, DidCloseTextDocumentParams &Params) override; @@ -79,6 +82,10 @@ private: JSONOutput &Out; /// Used to indicate that the 'shutdown' request was received from the /// Language Server client. + bool ShutdownRequestReceived = false; + + /// Used to indicate that the 'exit' notification was received from the + /// Language Server client. /// It's used to break out of the LSP parsing loop. bool IsDone = false; diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index 48a5875..421fa02 100644 --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -173,6 +173,7 @@ struct NoParams { } }; using ShutdownParams = NoParams; +using ExitParams = NoParams; struct InitializeParams { /// The process Id of the parent process that started diff --git a/clang-tools-extra/clangd/ProtocolHandlers.cpp b/clang-tools-extra/clangd/ProtocolHandlers.cpp index 7891e5b..6dcc3f5 100644 --- a/clang-tools-extra/clangd/ProtocolHandlers.cpp +++ b/clang-tools-extra/clangd/ProtocolHandlers.cpp @@ -52,6 +52,7 @@ void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, Register("initialize", &ProtocolCallbacks::onInitialize); Register("shutdown", &ProtocolCallbacks::onShutdown); + Register("exit", &ProtocolCallbacks::onExit); Register("textDocument/didOpen", &ProtocolCallbacks::onDocumentDidOpen); Register("textDocument/didClose", &ProtocolCallbacks::onDocumentDidClose); Register("textDocument/didChange", &ProtocolCallbacks::onDocumentDidChange); diff --git a/clang-tools-extra/clangd/ProtocolHandlers.h b/clang-tools-extra/clangd/ProtocolHandlers.h index ac819ba..bf307c8 100644 --- a/clang-tools-extra/clangd/ProtocolHandlers.h +++ b/clang-tools-extra/clangd/ProtocolHandlers.h @@ -34,6 +34,7 @@ public: virtual void onInitialize(Ctx C, InitializeParams &Params) = 0; virtual void onShutdown(Ctx C, ShutdownParams &Params) = 0; + virtual void onExit(Ctx C, ExitParams &Params) = 0; virtual void onDocumentDidOpen(Ctx C, DidOpenTextDocumentParams &Params) = 0; virtual void onDocumentDidChange(Ctx C, DidChangeTextDocumentParams &Params) = 0; diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp index cd7d44b..49781d2 100644 --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -114,5 +114,7 @@ int main(int argc, char *argv[]) { /// Initialize and run ClangdLSPServer. ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets, ResourceDirRef, CompileCommandsDirPath); - LSPServer.run(std::cin); + + constexpr int NoShutdownRequestErrorCode = 1; + return LSPServer.run(std::cin) ? 0 : NoShutdownRequestErrorCode; } diff --git a/clang-tools-extra/test/clangd/authority-less-uri.test b/clang-tools-extra/test/clangd/authority-less-uri.test index 1a21c10..494f691 100644 --- a/clang-tools-extra/test/clangd/authority-less-uri.test +++ b/clang-tools-extra/test/clangd/authority-less-uri.test @@ -30,3 +30,7 @@ Content-Length: 172 Content-Length: 44 {"jsonrpc":"2.0","id":3,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":3,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/completion-priorities.test b/clang-tools-extra/test/clangd/completion-priorities.test index ed90225..35ecd61 100644 --- a/clang-tools-extra/test/clangd/completion-priorities.test +++ b/clang-tools-extra/test/clangd/completion-priorities.test @@ -34,3 +34,7 @@ Content-Length: 151 Content-Length: 58 {"jsonrpc":"2.0","id":4,"method":"shutdown","params":null} +# CHECK: {"jsonrpc":"2.0","id":4,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/completion-qualifiers.test b/clang-tools-extra/test/clangd/completion-qualifiers.test index 52b7639..97cc002 100644 --- a/clang-tools-extra/test/clangd/completion-qualifiers.test +++ b/clang-tools-extra/test/clangd/completion-qualifiers.test @@ -16,3 +16,7 @@ Content-Length: 151 Content-Length: 44 {"jsonrpc":"2.0","id":4,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":4,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/completion-snippet.test b/clang-tools-extra/test/clangd/completion-snippet.test index 4a468a1..fdf797d 100644 --- a/clang-tools-extra/test/clangd/completion-snippet.test +++ b/clang-tools-extra/test/clangd/completion-snippet.test @@ -52,3 +52,7 @@ Content-Length: 148 Content-Length: 44 {"jsonrpc":"2.0","id":4,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":4,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/completion.test b/clang-tools-extra/test/clangd/completion.test index f15664d..bc2b530 100644 --- a/clang-tools-extra/test/clangd/completion.test +++ b/clang-tools-extra/test/clangd/completion.test @@ -52,3 +52,7 @@ Content-Length: 148 Content-Length: 44 {"jsonrpc":"2.0","id":4,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":4,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/definitions.test b/clang-tools-extra/test/clangd/definitions.test index 8e305bf..7d09c74 100644 --- a/clang-tools-extra/test/clangd/definitions.test +++ b/clang-tools-extra/test/clangd/definitions.test @@ -166,3 +166,7 @@ Content-Length: 148 Content-Length: 44 {"jsonrpc":"2.0","id":3,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":3,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/diagnostics-preamble.test b/clang-tools-extra/test/clangd/diagnostics-preamble.test index ac68f05..c4adbe3 100644 --- a/clang-tools-extra/test/clangd/diagnostics-preamble.test +++ b/clang-tools-extra/test/clangd/diagnostics-preamble.test @@ -13,3 +13,7 @@ Content-Length: 206 Content-Length: 58 {"jsonrpc":"2.0","id":2,"method":"shutdown","params":null} +# CHECK: {"jsonrpc":"2.0","id":2,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/diagnostics.test b/clang-tools-extra/test/clangd/diagnostics.test index f5ef394..41838c4 100644 --- a/clang-tools-extra/test/clangd/diagnostics.test +++ b/clang-tools-extra/test/clangd/diagnostics.test @@ -15,3 +15,7 @@ Content-Length: 152 Content-Length: 44 {"jsonrpc":"2.0","id":5,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":5,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/did-change-watch-files.test b/clang-tools-extra/test/clangd/did-change-watch-files.test index aabe1eb..d29184d7 100644 --- a/clang-tools-extra/test/clangd/did-change-watch-files.test +++ b/clang-tools-extra/test/clangd/did-change-watch-files.test @@ -59,3 +59,6 @@ Content-Length: 86 Content-Length: 44 {"jsonrpc":"2.0","id":3,"method":"shutdown"} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/extra-flags.test b/clang-tools-extra/test/clangd/extra-flags.test index 4e5fcc9..fe70379 100644 --- a/clang-tools-extra/test/clangd/extra-flags.test +++ b/clang-tools-extra/test/clangd/extra-flags.test @@ -18,5 +18,9 @@ Content-Length: 175 Content-Length: 44 {"jsonrpc":"2.0","id":5,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":5,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/fixits.test b/clang-tools-extra/test/clangd/fixits.test index a12892d..ab5b71e 100644 --- a/clang-tools-extra/test/clangd/fixits.test +++ b/clang-tools-extra/test/clangd/fixits.test @@ -26,3 +26,7 @@ Content-Length: 771 Content-Length: 44 {"jsonrpc":"2.0","id":3,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":3,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/formatting.test b/clang-tools-extra/test/clangd/formatting.test index a2a490a..09068fc 100644 --- a/clang-tools-extra/test/clangd/formatting.test +++ b/clang-tools-extra/test/clangd/formatting.test @@ -65,3 +65,7 @@ Content-Length: 204 Content-Length: 44 {"jsonrpc":"2.0","id":6,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":6,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/initialize-params-invalid.test b/clang-tools-extra/test/clangd/initialize-params-invalid.test index e20b9e2..77520d6 100644 --- a/clang-tools-extra/test/clangd/initialize-params-invalid.test +++ b/clang-tools-extra/test/clangd/initialize-params-invalid.test @@ -20,3 +20,7 @@ Content-Length: 142 Content-Length: 44 {"jsonrpc":"2.0","id":3,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":3,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/initialize-params.test b/clang-tools-extra/test/clangd/initialize-params.test index 1b91a94..5756232 100644 --- a/clang-tools-extra/test/clangd/initialize-params.test +++ b/clang-tools-extra/test/clangd/initialize-params.test @@ -20,3 +20,7 @@ Content-Length: 143 Content-Length: 44 {"jsonrpc":"2.0","id":3,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":3,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/input-mirror.test b/clang-tools-extra/test/clangd/input-mirror.test index 388f190..db54bcf 100644 --- a/clang-tools-extra/test/clangd/input-mirror.test +++ b/clang-tools-extra/test/clangd/input-mirror.test @@ -152,3 +152,7 @@ Content-Length: 148 Content-Length: 44 {"jsonrpc":"2.0","id":3,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":3,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/protocol.test b/clang-tools-extra/test/clangd/protocol.test index dc36145..4dd5508 100644 --- a/clang-tools-extra/test/clangd/protocol.test +++ b/clang-tools-extra/test/clangd/protocol.test @@ -1,8 +1,10 @@ -# RUN: clangd -run-synchronously < %s | FileCheck %s -# RUN: clangd -run-synchronously < %s 2>&1 | FileCheck -check-prefix=STDERR %s +# RUN: not clangd -run-synchronously < %s | FileCheck %s +# RUN: not clangd -run-synchronously < %s 2>&1 | FileCheck -check-prefix=STDERR %s # vim: fileformat=dos # It is absolutely vital that this file has CRLF line endings. # +# Note that we invert the test because we intent to let clangd exit prematurely. +# # Test protocol parsing Content-Length: 125 Content-Type: application/vscode-jsonrpc; charset-utf-8 diff --git a/clang-tools-extra/test/clangd/shutdown-with-exit.test b/clang-tools-extra/test/clangd/shutdown-with-exit.test new file mode 100644 index 0000000..3426099 --- /dev/null +++ b/clang-tools-extra/test/clangd/shutdown-with-exit.test @@ -0,0 +1,9 @@ +# RUN: clangd -run-synchronously < %s +# vim: fileformat=dos +# It is absolutely vital that this file has CRLF line endings. +Content-Length: 44 + +{"jsonrpc":"2.0","id":3,"method":"shutdown"} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/shutdown-without-exit.test b/clang-tools-extra/test/clangd/shutdown-without-exit.test new file mode 100644 index 0000000..869fcd1 --- /dev/null +++ b/clang-tools-extra/test/clangd/shutdown-without-exit.test @@ -0,0 +1,6 @@ +# RUN: not clangd -run-synchronously < %s +# vim: fileformat=dos +# It is absolutely vital that this file has CRLF line endings. +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/signature-help.test b/clang-tools-extra/test/clangd/signature-help.test index ba4f246..fc3b070 100644 --- a/clang-tools-extra/test/clangd/signature-help.test +++ b/clang-tools-extra/test/clangd/signature-help.test @@ -40,3 +40,7 @@ Content-Length: 151 Content-Length: 49 {"jsonrpc":"2.0","id":100000,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":100000,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} diff --git a/clang-tools-extra/test/clangd/unsupported-method.test b/clang-tools-extra/test/clangd/unsupported-method.test index 200bae0..cccbb5c 100644 --- a/clang-tools-extra/test/clangd/unsupported-method.test +++ b/clang-tools-extra/test/clangd/unsupported-method.test @@ -17,3 +17,7 @@ Content-Length: 92 Content-Length: 44 {"jsonrpc":"2.0","id":2,"method":"shutdown"} +# CHECK: {"jsonrpc":"2.0","id":2,"result":null} +Content-Length: 33 + +{"jsonrpc":"2.0":"method":"exit"} -- 2.7.4