[PATCH] D92198: [clangd] Implement remote index handshake

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

It'd be nice to know what problem this is trying to solve :-)
I can guess, but don't feel entirely sure here.

In D92198#2420203 <https://reviews.llvm.org/D92198#2420203>, @kadircet wrote:

> Haven't checked the details but is there a specific reason for implementing a custom protocol rather than making use of `NotifyOnStateChange` (https://grpc.github.io/grpc/cpp/classgrpc_1_1_channel_interface.html) or even `WaitForStateChange` if we really want to block the channel creation ?

Right, this is usually the way to go: we should definitely log NotifyOnStateChange events (note that the connection can go between available/unavailable many times over the life of the program). WaitForStateChange would be useful for blocking, but in a program where we want to proceed even if the connection is down (which is AFAIK the plan here) it's usually best not to block, and just handle fast-failure because the channel is down the same way as any other RPC error.

(I can imagine scenarios where we'd want to notify the user with showMessage or so whether the index is connected, but this requires more design)



================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:166
+  const uint32_t Checksum = 42;
+  const bool OK = Idx->handshake(Checksum);
+  if (!OK) {
----------------
This causes `getClient` to become blocking.

Granted, we expect this to be used from ProjectAwareIndex which currently places it on a separate thread anyway. But in general, what's the advantage of blocking here?


================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:170
+         "remote index.");
+    return nullptr;
+  }
----------------
this means if we're offline or so when clangd starts up, the remote index will not work until restarting clangd. Do we want this behavior?


================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:14
+message HandshakeRequest {
+  required uint32 checksum = 1;
+}
----------------
I find this protocol confusing, e.g. the "checksum" isn't actually a checksum of anything.
But this wolud be easier to assess if we knew what this is for.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92198/new/

https://reviews.llvm.org/D92198