[clangd-dev] Transport layer abstraction

[Jan Korous via clangd-dev]

Hi Sam,

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.

Transport::MessageHandler

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?

ClangdLSPServer::run()

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()). 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.

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().

WDYT?

Cheers,

Jan



P. S. For the record (if it would matter in the future) XPC interface is callback-based:

void XPCEntrypoint(xpc_connection_t connection) {
  xpc_connection_set_event_handler(connection, 
    ^(xpc_object_t message) {/* C block handling the message */ }
  );
  xpc_connection_resume(connection);
}

int main() {
  xpc_main(XPCEntrypoint);
}



> On Sep 12, 2018, at 11:17 PM, Sam McCall <sammccall at google.com> wrote:
> 
> On Wed, Sep 12, 2018 at 6:20 PM Jan Korous via clangd-dev <clangd-dev at lists.llvm.org> wrote:
> Hi all,
> 
> We had an internal discussion about XPC use with clangd and the result is that XPC-based transport layer in clangd is a hard requirement. This effectively means that the adapter approach is not feasible and I am looking into the transport layer abstraction Sam proposed earlier (https://reviews.llvm.org/D49389). Sorry for the trouble!
> No problem! (And sorry if I've been a bit unresponsive!)
> 
> There is a difficulty with designing an abstraction over transport layer due to XPC API requiring client code to hand control over. It's necessary for XPC server to call xpc_main() with a message handling callback:
> xpc_main(connection_handler);
> But XPC is running it's own message loop inside xpc_main() which is supposed to never return (in case of failure exit() is called) which makes it difficult to have common interface for JSON RPC and XPC.
> Unless I am mistaken that means that the message loop (Transport::loop(), ClangdLSPServer::run()) can't be part of generic code.
> 
> I am curious what are other people's thoughts on this?
> That makes sense to me, but I don't think a common interface is impossible.
> I noticed this in your patch, and in D49389 the idea (which I didn't spell out) was that the XPC transport's loop() would call xpc_main and never return.
> Transport::loop() is specific to a transport, it doesn't need to be part of generic code. ClangdLSPServer::run() ends up calling Transport::loop(), so it could be common code.
> 
> It'd look something like:
> 
> static Transport::MessageHandler *IncomingXPCMessages; // XPC -> clangd
> static xpc_connection_t *XPCConnection; // clangd -> XPC
> 
> XPCTransport::loop(MessageHandler &M) {
>   IncomingXPCMessages = &M;
>   xpc_main(&XPCEntrypoint);
> }
> 
> static void XPCEntrypoint(xpc_connection_t Conn) {
>   // I don't remember how an XPC server looks
>   // i guess you're reading editor->clangd messages in a loop?
>   for (;;) {
>     // lets say you parsed a notification
>     std::string name = ...;
>     json::Value args = xpcToJSON(...);
>     IncomingXPCMessages->notify(name, args); // send the notification to clangd
>   }
> }
> 
> XPCTransport::notify(StringRef name, json::Value args) {
>   xpc_object_t XPCNotification = encodeNotificationForXPCSomehow(name, jsonToXPC(args))
>   xpc_connection_send(*XPCConnection, XPCNotification);
> }
> 
> WDYT?
>  
> I tried to come up with some reasonable abstraction but I am probably already anchored by my previous work (https://reviews.llvm.org/D48559) - all my design ideas tend to go in the direction of abstraction over JsonOutput and custom dispatchers (StdIoDispatcher/XPCDispatcher).
> 
> Cheers,
> 
> Jan
> _______________________________________________
> clangd-dev mailing list
> clangd-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/clangd-dev