Page MenuHomePhabricator

[clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
ClosedPublic

Authored by sammccall on Oct 15 2018, 5:41 AM.

Details

Summary

This paves the way for alternative transports (mac XPC, maybe messagepack?),
and also generally improves layering: testing ClangdLSPServer becomes less of
a pipe dream, we split up the JSONOutput monolith, etc.

This isn't a final state, much of what remains in JSONRPCDispatcher can go away,
handlers can call reply() on the transport directly, JSONOutput can be renamed
to StreamLogger and removed, etc. But this patch is sprawling already.

The main observable change (see tests) is that hitting EOF on input is now an
error: the client should send the 'exit' notification.
This is defensible: the protocol doesn't spell this case out. Reproducing the
current behavior for all combinations of shutdown/exit/EOF clutters interfaces.
We can iterate on this if desired.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Oct 15 2018, 5:41 AM

This patch is big and hard to navigate. I tried to keep it contained, but JSONRPCDispatcher is pretty tangled.

The main parts are:

  • Transport.h is the key new abstraction that should be understood first
  • JSONTransport.cpp is its standard implementation. The raw IO stuff in there is just lifted from the old JSONRPCDispatcher, and isn't very interesting. Everything else should be though - it shows how the new interface works.
  • The ClangdLSPServer and ClangdMain changes show how the new interface is used
  • JSONRPCDispatcher has had most of its guts ripped out. There's more that can be done there, that file may go away completely.
  • The test changes just reflect the shutdown getting a little bit stricter (they also pass before this patch)

For reference, @jkorous has a WIP in D53290.
It's scope is a superset, and I think everything in common is basically the same (they were both based on a common prototype).
Jan is out at the moment, so I think it makes sense to move ahead with this piece - it's progress, and the smaller scope makes landing the change easier.

I just realized though there are no JSONTransport unit tests in this patch, I'll add those.

sammccall updated this revision to Diff 169807.Oct 16 2018, 4:37 AM

Add unit test for JSON transport.

Nice! The code looks much clearer. Just some nits

clangd/ClangdLSPServer.cpp
156 ↗(On Diff #169807)

?

clangd/ClangdLSPServer.h
45 ↗(On Diff #169807)

doc is outdated now

clangd/JSONRPCDispatcher.cpp
218 ↗(On Diff #169807)

The comment should also be moved?

clangd/JSONRPCDispatcher.h
29 ↗(On Diff #169807)

nit: make the FIXME more concrete?

clangd/JSONTransport.cpp
23 ↗(On Diff #169807)

Is E guaranteed to be an LSPError? Do we need a handler for other errors?

83 ↗(On Diff #169807)

Maybe add a comment here that the return value of handleMessage indicates whether to shutdown. It' not clear without context.

129 ↗(On Diff #169807)

nit: add 2.0 to the log?

clangd/Transport.h
50 ↗(On Diff #169807)

nit: maybe handleNotify to make the two directions more distinguishable? This can get a bit counter-intuitive in the call sites.

unittests/clangd/JSONTransportTests.cpp
106 ↗(On Diff #169807)

Would operator<< be used when the error is real (i.e. check passes)?

sammccall updated this revision to Diff 169835.Oct 16 2018, 9:25 AM
sammccall marked 9 inline comments as done.

Addressed review comments.
Also inverted the sense of the boolean return from onNotify etc, was "should
exit" and is now "should continue", which I think is slightly clearer.

sammccall marked 9 inline comments as done.Oct 16 2018, 9:25 AM
sammccall added inline comments.
clangd/ClangdLSPServer.cpp
156 ↗(On Diff #169807)

Fixed this comment.
I'm not totally happy with the shutdown mechanism, it's noisier and more magic than I'd like. May find something nicer to replace it with.

clangd/JSONTransport.cpp
23 ↗(On Diff #169807)

This actually handles generic errors as well, Error just has a really confusing API.
Rewrote this to be more direct.

clangd/Transport.h
50 ↗(On Diff #169807)

onNotify etc

unittests/clangd/JSONTransportTests.cpp
106 ↗(On Diff #169807)

No, EXPECT_* are magic macros that only evaluate the right hand side of << if they fire.

EXPECT_FALSE(expr) expands to something like if (expr) CreateErrorStream("expr"), so the << blah becomes part of the if statement.
(And the temporary stream returned from CreateErrorStream does the actual logging/mark-the-test-as-failed in its destructor).

ioeric accepted this revision.Oct 16 2018, 9:34 AM
ioeric added inline comments.
clangd/JSONRPCDispatcher.h
70 ↗(On Diff #169835)

s/true/false/

This revision is now accepted and ready to land.Oct 16 2018, 9:34 AM
This revision was automatically updated to reflect the committed changes.
sammccall marked 5 inline comments as done.