Page MenuHomePhabricator

[clangd] Handle exit notification (proper shutdown)
ClosedPublic

Authored by rwols on Oct 15 2017, 2:59 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

rwols created this revision.Oct 15 2017, 2:59 PM
sammccall accepted this revision.Oct 16 2017, 12:41 AM
sammccall added inline comments.
test/clangd/authority-less-uri.test
33 ↗(On Diff #119099)

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.

This revision is now accepted and ready to land.Oct 16 2017, 12:41 AM
rwols added inline comments.Oct 16 2017, 11:22 AM
test/clangd/authority-less-uri.test
33 ↗(On Diff #119099)

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 updated this revision to Diff 119198.Oct 16 2017, 12:54 PM

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

The protocol.test test fails now.

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 accepted this revision.Oct 17 2017, 12:43 AM

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 updated this revision to Diff 119370.Oct 17 2017, 12:29 PM
  • Add test shutdown-with-exit.test
  • Add test shutdown-without-exit.test
  • Make protocol.test pass by inverting exit code ("# RUN: not ...")
rwols added a comment.EditedOct 17 2017, 12:30 PM

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 added inline comments.Oct 23 2017, 1:25 AM
clangd/ClangdLSPServer.cpp
59 ↗(On Diff #119370)

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 accepted this revision.Oct 23 2017, 5:44 AM

Oh, and other than those NITs, LGTM.

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

rwols updated this revision to Diff 119941.Oct 23 2017, 2:49 PM
  • 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 marked 3 inline comments as done.Oct 23 2017, 2:59 PM

you have commit access now, right?

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

malaperle accepted this revision.Oct 24 2017, 9:37 PM

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

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!)

This revision was automatically updated to reflect the committed changes.