[clangd] Handle exit notification (proper shutdown)

[rwols]

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.

[sammccall]

test/clangd/authority-less-uri.test

33

Having the shutdown/exit boilerplate in every test seems unfortunate.
(I realize this isn't new here, but it's getting worse!)

Does clangd exit cleanly on stdin EOF? If it could be made to do so, maybe we could just have e.g. the initialize test do the full sequence, and the other tests omit shutdown.

[rwols]

test/clangd/authority-less-uri.test

33

Yeah, we could do that. Just send {"jsonrpc":"2.0":"method":"exit"} and don't care about the exit status. Everyone can then omit the shutdown request. I have a little more changes coming up for this diff: we should exit with exit status 0 if shutdown was received before exit, otherwise we should return 1.

[rwols]

Return 0 if shutdown was received before exit, otherwise return 1

The protocol.test test fails now.

[rwols]

I couldn't get the protocol.test to succeed; clangd returns 1 now and that trips up lit. I think I have to use XFAIL from lit somehow but didn't figure it out.

[sammccall]

So I'm not totally convinced that treating an abrupt exit or EOF as an error is worth the hassle (others might disagree), but it's certainly reasonable.
Could you add a test for the new behavior?
something like

RUN: not clangd -run-synchronously < %s
<initialize and exit (no shutdown)>

RUN: not clangd -run-synchronously < %s
<initialize, and nothing else (stdin will just get eof)>

[rwols]

• Add test shutdown-with-exit.test
• Add test shutdown-without-exit.test
• Make protocol.test pass by inverting exit code ("# RUN: not ...")

[rwols]

So I'm not totally convinced that treating an abrupt exit or EOF as an error is worth the hassle

Agreed, at least from the POV of a client clangd shuts down gracefully now.

All tests pass.

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

59–61

The comment is a bit misleading. I'd simply say we don't do need to do anything on shutdown at this point.

offtopic:
Preambles is not something we'd want to serialise (they are rather fast to build, and we only need them to make code completion fast), properly saving state of various indexes when we'll have them is something we'll definitely need to do on shutdown, though.

clangd/tool/ClangdMain.cpp

117 ↗

(On Diff #119370)

Maybe add a const NoShutdownRequestErrorCode = 1; and use it here instead of 1?
Or add a comment on why we exit with error code. (It's not entirely obvious to someone who hasn't read this part of LSP spec)

test/clangd/protocol.test

1 ↗

(On Diff #119370)

Maybe add a comment why we use not here?
It's not obvious for someone who hasn't read the whole test file.

[ilya-biryukov]

Oh, and other than those NITs, LGTM.

@rwols, you have commit access now, right?)

[rwols]

• Clarify why we invert the tests in protocol.test,
• Use a variable name to clarify why we exit with status code 1 (when run() returns true),
• Reword misleading comment in onShutdown method.

[rwols]

you have commit access now, right?

I'll have to think about if I want that responsibility!

[malaperle]

Nice! I had a fix locally for the same purpose but this is much better!

[ilya-biryukov]

In D38939#904318, @rwols wrote:

I'll have to think about if I want that responsibility!

While you're at it, I'll submit this patch for you. But I'd definitely encourage you to get commit access. You've been contributing a bunch of useful stuff to clangd (BTW, thanks for the contributions!)

clangd/ClangdLSPServer.h

private:
   // Implement DiagnosticsConsumer.
   virtual void
   onDiagnosticsReady(PathRef File,
                      Tagged<std::vector<DiagWithFixIts>> Diagnostics) override;
 
   // 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;
   void
   onDocumentOnTypeFormatting(Ctx C,
                              DocumentOnTypeFormattingParams &Params) override;
   void
   onDocumentRangeFormatting(Ctx C,

clangd/ClangdLSPServer.cpp

C.reply(
         }})");
   if (Params.rootUri && !Params.rootUri->file.empty())
     Server.setRootPath(Params.rootUri->file);
   else if (Params.rootPath && !Params.rootPath->empty())
     Server.setRootPath(*Params.rootPath);
 }
 
 void ClangdLSPServer::onShutdown(Ctx C, ShutdownParams &Params) {
-  IsDone = true;
+  // Before we reply, we could serialize the preambles to disk. For now, let's
+  // just say we're ready to exit.
+  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())
     CDB.setExtraFlagsForFile(Params.textDocument.uri.file,
                              std::move(Params.metadata->extraFlags));
   Server.addDocument(Params.textDocument.uri.file, Params.textDocument.text);
 }
 

clangd/Protocol.h

 
 struct NoParams {
   static llvm::Optional<NoParams> parse(llvm::yaml::MappingNode *Params,
                                         Logger &Logger) {
     return NoParams{};
   }
 };
 using ShutdownParams = NoParams;
+using ExitParams = NoParams;
 
 struct InitializeParams {
   /// The process Id of the parent process that started
   /// the server. Is null if the process has not been started by another
   /// process. If the parent process is not alive then the server should exit
   /// (see exit notification) its process.
   llvm::Optional<int> processId;
 

clangd/ProtocolHandlers.h

 // The interface implemented by ClangLSPServer to handle incoming requests.
 class ProtocolCallbacks {
 public:
   using Ctx = RequestContext;
   virtual ~ProtocolCallbacks() = default;
 
   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;
   virtual void onDocumentDidClose(Ctx C,
                                   DidCloseTextDocumentParams &Params) = 0;
   virtual void onDocumentFormatting(Ctx C,
                                     DocumentFormattingParams &Params) = 0;
   virtual void

clangd/ProtocolHandlers.cpp

 
 void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
                                       JSONOutput &Out,
                                       ProtocolCallbacks &Callbacks) {
   HandlerRegisterer Register{Dispatcher, &Out, &Callbacks};
 
   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);
   Register("textDocument/rangeFormatting",
            &ProtocolCallbacks::onDocumentRangeFormatting);
   Register("textDocument/onTypeFormatting",
            &ProtocolCallbacks::onDocumentOnTypeFormatting);
   Register("textDocument/formatting", &ProtocolCallbacks::onDocumentFormatting);
   Register("textDocument/codeAction", &ProtocolCallbacks::onCodeAction);
   Register("textDocument/completion", &ProtocolCallbacks::onCompletion);
   Register("textDocument/signatureHelp", &ProtocolCallbacks::onSignatureHelp);
   Register("textDocument/definition", &ProtocolCallbacks::onGoToDefinition);
   Register("textDocument/switchSourceHeader",
            &ProtocolCallbacks::onSwitchSourceHeader);
   Register("workspace/didChangeWatchedFiles", &ProtocolCallbacks::onFileEvent);
 }

test/clangd/authority-less-uri.test

 # Test params parsing in the presence of a 1.x-compatible client (inlined "uri")
 #
 # CHECK: {"jsonrpc":"2.0","id":2,"result":[
 # CHECK-DAG: {"label":"a","kind":5,"detail":"int","sortText":"000035a","filterText":"a","insertText":"a","insertTextFormat":1}
 # CHECK: ]}
 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"}

test/clangd/completion-priorities.test

 # CHECK-DAG: {"label":"pub()","kind":2,"detail":"void","sortText":"000034pub","filterText":"pub","insertText":"pub","insertTextFormat":1}
 # CHECK-DAG: {"label":"prot()","kind":2,"detail":"void","sortText":"200034prot","filterText":"prot","insertText":"prot","insertTextFormat":1}
 # CHECK-DAG: {"label":"priv()","kind":2,"detail":"void","sortText":"200034priv","filterText":"priv","insertText":"priv","insertTextFormat":1}
 # CHECK: ]}
 
 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"}

test/clangd/completion-qualifiers.test

 # CHECK: {"jsonrpc":"2.0","id":2,"result":[
 # CHECK-DAG: {"label":"foo() const","kind":2,"detail":"int","sortText":"200035foo","filterText":"foo","insertText":"foo","insertTextFormat":1}
 # CHECK-DAG: {"label":"bar() const","kind":2,"detail":"int","sortText":"000037bar","filterText":"bar","insertText":"bar","insertTextFormat":1}
 # CHECK-DAG: {"label":"Foo::foo() const","kind":2,"detail":"int","sortText":"000037foo","filterText":"foo","insertText":"foo","insertTextFormat":1}
 # CHECK: ]}
 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"}

test/clangd/completion-snippet.test

 # Repeat the completion request, expect the same results.
 #
 # CHECK: {"jsonrpc":"2.0","id":3,"result":[
 # CHECK-DAG: {"label":"func()","kind":2,"detail":"int (*)(int, int)","sortText":"000034func","filterText":"func","insertText":"func()","insertTextFormat":1}
 # CHECK: ]}
 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"}

test/clangd/completion.test

 # Repeat the completion request, expect the same results.
 #
 # CHECK: {"jsonrpc":"2.0","id":3,"result":[
 # CHECK-DAG: {"label":"func()","kind":2,"detail":"int (*)(int, int)","sortText":"000034func","filterText":"func","insertText":"func","insertTextFormat":1}
 # CHECK: ]}
 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"}

test/clangd/definitions.test

 
 {"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":4,"character":7}}}
 # Go to macro, being undefined
 # CHECK: {"jsonrpc":"2.0","id":1,"result":[{"uri": "file:///main.cpp", "range": {"start": {"line": 2, "character": 8}, "end": {"line": 2, "character": 13}}}]}
 
 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"}

test/clangd/diagnostics-preamble.test

 # RUN: clangd -run-synchronously < %s | FileCheck %s
 # It is absolutely vital that this file has CRLF line endings.
 #
 Content-Length: 125
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 
 Content-Length: 206
 
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#ifndef FOO\n#define FOO\nint a;\n#else\nint a = b;#endif\n\n\n"}}}
 # CHECK: {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///main.cpp","diagnostics":[]}}
 
 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"}

test/clangd/diagnostics.test

 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"void main() {}"}}}
 #
 # CHECK: {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///foo.c","diagnostics":[{"range":{"start": {"line": 0, "character": 1}, "end": {"line": 0, "character": 1}},"severity":2,"message":"return type of 'main' is not 'int'"},{"range":{"start": {"line": 0, "character": 1}, "end": {"line": 0, "character": 1}},"severity":3,"message":"change return type to 'int'"}]}}
 #
 #
 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"}

test/clangd/did-change-watch-files.test

 # Custom field
 Content-Length: 86
 
 {"jsonrpc":"2.0","method":"workspace/didChangeWatchedFiles","params":{"custom":"foo"}}
 # STDERR: Ignored unknown field "custom"
 Content-Length: 44
 
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}

test/clangd/extra-flags.test

 Content-Length: 175
 
 {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///foo.c","version":2},"contentChanges":[{"text":"int main() { int i; return i; }"}]}}
 # CHECK: {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///foo.c","diagnostics":[{"range":{"start": {"line": 0, "character": 28}, "end": {"line": 0, "character": 28}},"severity":2,"message":"variable 'i' is uninitialized when used here"},{"range":{"start": {"line": 0, "character": 19}, "end": {"line": 0, "character": 19}},"severity":3,"message":"initialize the variable 'i' to silence this warning"}]}}
 #
 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"}
 
 

test/clangd/fixits.test

 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
 # CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Apply FixIt 'place parentheses around the assignment to silence this warning'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 32}}, "newText": "("},{"range": {"start": {"line": 0, "character": 37}, "end": {"line": 0, "character": 37}}, "newText": ")"}]]},{"title":"Apply FixIt 'use '==' to turn this assignment into an equality comparison'", "command": "clangd.applyFix", "arguments": ["file:///foo.c", [{"range": {"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}}, "newText": "=="}]]}]
 #
 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"}

test/clangd/formatting.test

 
 {"jsonrpc":"2.0","id":5,"method":"textDocument/onTypeFormatting","params":{"textDocument":{"uri":"file:///foo.c"},"position":{"line":3,"character":1},"ch":"}","options":{"tabSize":4,"insertSpaces":true}}}
 # CHECK: {"jsonrpc":"2.0","id":5,"result":[{"range": {"start": {"line": 0, "character": 7}, "end": {"line": 0, "character": 8}}, "newText": ""},{"range": {"start": {"line": 0, "character": 9}, "end": {"line": 0, "character": 10}}, "newText": ""},{"range": {"start": {"line": 0, "character": 15}, "end": {"line": 0, "character": 16}}, "newText": ""}]}
 #
 
 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"}

test/clangd/initialize-params-invalid.test

 # CHECK:   "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
 # CHECK:   "signatureHelpProvider": {"triggerCharacters": ["(",","]},
 # CHECK:   "definitionProvider": true
 # CHECK: }}}
 #
 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"}

test/clangd/initialize-params.test

 # CHECK:   "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
 # CHECK:   "signatureHelpProvider": {"triggerCharacters": ["(",","]},
 # CHECK:   "definitionProvider": true
 # CHECK: }}}
 #
 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"}

test/clangd/input-mirror.test

 Content-Length: 148
 
 {"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":4,"character":7}}}
 # Go to macro, being undefined
 
 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"}

test/clangd/signature-help.test

 # CHECK-DAG: {"label":"bar(int x, int y = 0) -> void","parameters":[{"label":"int x"},{"label":"int y = 0"}]}
 # CHECK-DAG: {"label":"bar(float x = 0, int y = 42) -> void","parameters":[{"label":"float x = 0"},{"label":"int y = 42"}]}
 # CHECK: ]}
 
 # Shutdown.
 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"}

test/clangd/unsupported-method.test

 Content-Length: 92
 
 {"jsonrpc":"2.0","id":1,"method":"textDocument/jumpInTheAirLikeYouJustDontCare","params":{}}
 # CHECK: {"jsonrpc":"2.0","id":1,"error":{"code":-32601,"message":"method not found"}}
 
 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"}