<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Sam,<div class=""><br class=""></div><div class="">My work is unfortunately stuck at the moment - I have been enjoying a sinus infection for almost two weeks now, expecting to go back to work in another week.</div><div class=""><br class=""></div><div class="">I put together crude implementation (lots of TODO everywhere and definitely not finished) using your old proposal. If that’s of any interest you can find it here:</div><div class=""><a href="https://reviews.llvm.org/D53290" class="">https://reviews.llvm.org/D53290</a></div><div class=""><br class=""></div><div class="">Since this is very important for us I am definitely going to work on transport layer when I am back at work. I will take a proper look and try to “merge” our approaches.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class=""><br class=""></div><div class="">Jan <br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Oct 15, 2018, at 1:54 PM, Sam McCall <<a href="mailto:sammccall@google.com" class="">sammccall@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div dir="ltr" class="">Hi Jan,<div class=""><br class=""></div><div class="">Wondering how your work here is going :-)</div><div class="">I revived the Transport idea and put together <a href="https://reviews.llvm.org/D53286" class="">https://reviews.llvm.org/D53286</a> where everything seems to work.</div><div class="">(There's some cruft left in JSONRPCDispatcher that can be removed afterwards).</div><div class=""><br class=""></div><div class="">I think this is worthwhile independent of XPC (e.g. for testing ClangdLSPServer) but of course it has to be compatible with what you need.</div><div class="">Let me know if you see any blockers here or if something has changed!</div><div class=""><br class=""></div><div class="">Cheers, Sam</div></div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Mon, Sep 17, 2018 at 3:46 PM Jan Korous <<a href="mailto:jkorous@apple.com" class="">jkorous@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space" class=""><div dir="auto" style="word-wrap:break-word;line-break:after-white-space" class=""><div dir="auto" style="word-wrap:break-word;line-break:after-white-space" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On 13 Sep 2018, at 15:15, Sam McCall <<a href="mailto:sammccall@google.com" target="_blank" class="">sammccall@google.com</a>> wrote:</div><br class="m_848994753540278772Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Thu, Sep 13, 2018 at 3:21 PM Jan Korous <<a href="mailto:jkorous@apple.com" target="_blank" class="">jkorous@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Sam,<br class="">
<br class="">
Thanks for the response.<br class="">
<br class="">
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.<br class=""></blockquote><div class="">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.</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">Not sure whether this was the right call, but they're not likely to switch to any Transport abstraction we add.</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">Got it.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class="">This is certainly an option for XCode too, but then the most obvious path is write a wrapper around ClangdServer, and ignore ClangdLSPServer/ClangdMain.</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">True. We should be good with just custom transport layer implementation though.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Transport::MessageHandler<br class="">
<br class="">
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?<br class=""></blockquote><div class="">Yeah, this is mostly just ad-hoc decoupling, not real abstraction. It buys us some things:</div><div class=""> - transports can be tested without involving the behavior of all of clangd</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">Oh, I totally missed that. I am sold now!</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""> - ClangdLSPServer's notify() can be private (using interface inheritance, or a nested class, or something)</div><div class=""> - transports don't have to depend on ClangdLSPServer, if we decide to start caring more about layering (interesting for out-of-tree transports!)</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">Also a fair point.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
ClangdLSPServer::run()<br class="">
<br class="">
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()).</blockquote><div class="">Well, the purpose of the *current* ClangdLSPServer::run() is set up the dispatcher right, and to put the error handling code somewhere.</div><div class="">Maybe it's totally redundant and main should just call Transport.loop().</div><div class=""><br class=""></div><div class="">(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).</div><div class=""> <br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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.<br class="">
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.<br class=""></blockquote><div class="">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.</div><div class=""><br class=""></div><div class="">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.</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">Check if the client request is exit notification in the XPC message handling C block and conditionally call exit(). Would just need to get access to ClangdLSPServer::ShutdownRequestReceived from there.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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().<br class=""></blockquote><div class="">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.</div><div class="">Where is my logic confused?</div><div class=""> - JSONRPCDispatcher does some JSON stuff and some generic "message handling" stuff</div><div class=""> - we want the "message handling" stuff to be common</div><div class=""> - 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")</div><div class=""> - we want a common abstraction (e.g. interface) over the XPC and JSON-specific code</div><div class=""> - XPC needs somewhere to call xpc_main(), which doesn't return</div><div class=""> - JSON needs every function to return, because it supports clean exit</div><div class=""> - therefore some XPC function doesn't return, when the corresponding JSON one does</div><div class="">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…</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">My point of view was that rather than having part of abstraction that is working only for 1 out of 2 implementations it is supposed to abstract I would consider removing it from the interface (less code, less complexity) and keeping it as part of JSON-RPC specific code. But I see the benefit of not having duplicated callbacks to dispatcher registration so I am happy to go with the Transport::loop().</div><div class=""><br class=""></div><div class="">Cheers,</div><div class=""><br class=""></div><div class="">Jan</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class=""> <br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="">
WDYT?<br class="">
<br class="">
Cheers,<br class="">
<br class="">
Jan<br class="">
<br class="">
<br class="">
<br class="">
P. S. For the record (if it would matter in the future) XPC interface is callback-based:<br class="">
<br class="">
void XPCEntrypoint(xpc_connection_t connection) {<br class="">
xpc_connection_set_event_handler(connection, <br class="">
^(xpc_object_t message) {/* C block handling the message */ }<br class="">
);<br class="">
xpc_connection_resume(connection);<br class="">
}<br class="">
<br class="">
int main() {<br class="">
xpc_main(XPCEntrypoint);<br class="">
}<br class="">
<br class="">
<br class="">
<br class="">
> On Sep 12, 2018, at 11:17 PM, Sam McCall <<a href="mailto:sammccall@google.com" target="_blank" class="">sammccall@google.com</a>> wrote:<br class="">
> <br class="">
> On Wed, Sep 12, 2018 at 6:20 PM Jan Korous via clangd-dev <<a href="mailto:clangd-dev@lists.llvm.org" target="_blank" class="">clangd-dev@lists.llvm.org</a>> wrote:<br class="">
> Hi all,<br class="">
> <br class="">
> 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 (<a href="https://reviews.llvm.org/D49389" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D49389</a>). Sorry for the trouble!<br class="">
> No problem! (And sorry if I've been a bit unresponsive!)<br class="">
> <br class="">
> 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:<br class="">
> xpc_main(connection_handler);<br class="">
> 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.<br class="">
> Unless I am mistaken that means that the message loop (Transport::loop(), ClangdLSPServer::run()) can't be part of generic code.<br class="">
> <br class="">
> I am curious what are other people's thoughts on this?<br class="">
> That makes sense to me, but I don't think a common interface is impossible.<br class="">
> 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.<br class="">
> 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.<br class="">
> <br class="">
> It'd look something like:<br class="">
> <br class="">
> static Transport::MessageHandler *IncomingXPCMessages; // XPC -> clangd<br class="">
> static xpc_connection_t *XPCConnection; // clangd -> XPC<br class="">
> <br class="">
> XPCTransport::loop(MessageHandler &M) {<br class="">
> IncomingXPCMessages = &M;<br class="">
> xpc_main(&XPCEntrypoint);<br class="">
> }<br class="">
> <br class="">
> static void XPCEntrypoint(xpc_connection_t Conn) {<br class="">
> // I don't remember how an XPC server looks<br class="">
> // i guess you're reading editor->clangd messages in a loop?<br class="">
> for (;;) {<br class="">
> // lets say you parsed a notification<br class="">
> std::string name = ...;<br class="">
> json::Value args = xpcToJSON(...);<br class="">
> IncomingXPCMessages->notify(name, args); // send the notification to clangd<br class="">
> }<br class="">
> }<br class="">
> <br class="">
> XPCTransport::notify(StringRef name, json::Value args) {<br class="">
> xpc_object_t XPCNotification = encodeNotificationForXPCSomehow(name, jsonToXPC(args))<br class="">
> xpc_connection_send(*XPCConnection, XPCNotification);<br class="">
> }<br class="">
> <br class="">
> WDYT?<br class="">
> <br class="">
> I tried to come up with some reasonable abstraction but I am probably already anchored by my previous work (<a href="https://reviews.llvm.org/D48559" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D48559</a>) - all my design ideas tend to go in the direction of abstraction over JsonOutput and custom dispatchers (StdIoDispatcher/XPCDispatcher).<br class="">
> <br class="">
> Cheers,<br class="">
> <br class="">
> Jan<br class="">
> _______________________________________________<br class="">
> clangd-dev mailing list<br class="">
> <a href="mailto:clangd-dev@lists.llvm.org" target="_blank" class="">clangd-dev@lists.llvm.org</a><br class="">
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/clangd-dev" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/clangd-dev</a><br class="">
<br class="">
</blockquote></div></div>
</div></blockquote></div><br class=""></div></div></div></blockquote></div>
</div></blockquote></div><br class=""></div></body></html>