Thanks for the response.
I think I understand your intentions better now. The Transport abstraction looks good enough to be used to me so let me just explain what I though are imperfections that I know about (no big deals though). If these are ok with you I’d go ahead and work on it. Another thing is that you guys already have another transport layer implementation so if this abstraction fits nicely 2 out of 3 implementations with XPC being kind of little less aligned I’d consider it still pragmatic to use it.
So Transport isn’t implemented or even really fully fleshed out. We should design it (or something else) to bridge the JSON/XPC divide well, though perfect is always hard.
Our “other transport layer” is really a different protocol, it doesn’t use ClangdLSPServer but wraps ClangdServer instead. The protocol happens to follow LSP fairly closely (at least in spirit), but doesn’t try to reuse any of our LSP code.
Not sure whether this was the right call, but they’re not likely to switch to any Transport abstraction we add.
This is certainly an option for XCode too, but then the most obvious path is write a wrapper around ClangdServer, and ignore ClangdLSPServer/ClangdMain.
I just wonder if this isn’t actually the “reverse” of desired interface. It seems to me that MessageHandler is abstracting clangd implementation details when used in specific Transport implementation. For example it would help if we ever decide to swap clangd for another LSP implementation or clangd-ng. But it doesn’t abstract the transport layer itself. What benefit does it grant instead of calling for example ClangdLSPServer::notify() directly?
Yeah, this is mostly just ad-hoc decoupling, not real abstraction. It buys us some things:
- transports can be tested without involving the behavior of all of clangd
- ClangdLSPServer’s notify() can be private (using interface inheritance, or a nested class, or something)
- transports don’t have to depend on ClangdLSPServer, if we decide to start caring more about layering (interesting for out-of-tree transports!)
IIUIC purpose of this method is to have abstraction over different Transport implementations with common strategies for program return value (ShutdownRequestReceived) and error handling (return value of Transport.loop()).
Well, the purpose of the current ClangdLSPServer::run() is set up the dispatcher right, and to put the error handling code somewhere.
Maybe it’s totally redundant and main should just call Transport.loop().
(FWIW I think the error handling on exit isn’t an important part of the protocol and we could take some shortcuts, especially in an XPC context).
Yet since we can’t do this for XPC (as xpc_main doesn’t return) we’d have to duplicate the code (~10 LOC, no big deal) but it would mean that it wouldn’t be always possible to just blindly swap implementations of this interface.
In other words it’s a violation of Liskov substitution principle which in itself doesn’t necessarily mean it’s a big deal - I am just wondering if it makes sense to have such abstraction.
So I’m not sure how big a deal this is. It would indeed be cleaner not to have a loop() that sometimes returns and sometimes doesn’t. Pragmatically, I think we can arrange to handle it calling exit instead, I guess.
How do you want clangd shutdown to work in an XPC context? Reading the XPC docs suggest services just run until they get SIGKILL. In which case a Transport::loop() that never returns seems completely appropriate to me: the transport dictates that the stream of events never ends.
This is basically what I meant that it’s difficult to have common abstraction over the “input direction” of transport layer (since Transport::MessageHandler looks more like an abstraction over ClangdLSPServer input handling) and because of that I’d consider different dispatchers instead of Transport::loop().
I’m not sure what distinction is being drawn here, I think we’ll end up with some function like loop() that returns in JSON and not with XPC.
Where is my logic confused?
- JSONRPCDispatcher does some JSON stuff and some generic “message handling” stuff
- we want the “message handling” stuff to be common
- the JSON stuff needs to be split from the message handling stuff, so it can be replaced by XPC (in the sketch I called this part “Transport”)
- we want a common abstraction (e.g. interface) over the XPC and JSON-specific code
- XPC needs somewhere to call xpc_main(), which doesn’t return
- JSON needs every function to return, because it supports clean exit
- therefore some XPC function doesn’t return, when the corresponding JSON one does
There are ways around this I guess (call xpc_main on a thread? have the JSON code clean up and call exit()?) but they seem bad worse than just having a transport that never stops looping…