[clangd] Log remote index connectivity status

[kbobyrev]

This is not working properly yet.

[kbobyrev]

Remove some logs.

[kbobyrev]

This is working now. However, the handshake is actually deferred for
ProjectAwareIndex until the first request. I tried to invoke it via something
like Opts.StaticIdx->estimateMemoryUsage() in ClangdMain.cpp but the
problem is getIndex() will return nullptr until the Config::current is
ready. I'm not entirely sure when this happens so I can't promote handshake to
some sensible time point yet.

[kbobyrev]

Remove unused header.

[kadircet]

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 ?

Also if there's a need to make this an RPC request, there's the default HealthCheck request we can send, as described in https://github.com/grpc/grpc/blob/master/doc/health-checking.md. This is already enabled for remote-index-server in https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/index/remote/server/Server.cpp#L310, but I suppose registering a callback for statechange and logging that should be enough for a "debugging" use case (not sure what the motivation currently is).

[sammccall]

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, @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)

clang-tools-extra/clangd/index/remote/Client.cpp

169

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?

173

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?

clang-tools-extra/clangd/index/remote/Index.proto

14

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.

[kbobyrev]

Use WaitForStateChange instead. Simplify code structure.

[kbobyrev]

Yes, the idea here is to provide logging for gRPC server state. My original thought was that default HealthCheck might not be enough and we might want to add more things in the mechanism but I guess it's not obvious what these extensions might be and getting grpc_connectivity_state might be enough.

[sammccall]

Thanks! Hooking into the native status stuff seems much nicer!

Sorry if it seems like we're going in circles here, I think we should be careful about adding background threads so want to look at an alternative too.

clang-tools-extra/clangd/index/remote/Client.cpp

26

this seems pedantic, but we really shouldn't specialize templates we don't own for types we don't own (ODR concerns)

I'd just make this function (state -> StringRef) and call it

32

any reason not to make these a bit more human-readable? connecting/ready/transient failure/shutdown?

86

The thread is a bit surprising here. I was assuming we'd use some callback-based API to watch rather than spawn a thread to do the "work" of blocking on a change.
Also this is going to "wake up" periodically to refresh the watch, which seems undesirable if we want to go truly idle.

Ah, so there's no callback-type connection-changed API! Sorry to mislead you :-( I hadn't read the API carefully. (I think this is coming as some point - they're talking about replacing CompletionQueue with EventManager which is a callback-based abstraction)

Seems the "async" option is to subscribe on a CompletionQueue, but then we have to poll it. We can use a zero timeout so the poll doesn't block... But then we might as well poll GetStatus directly.

Then the question is where to do that from. What if we called GetStatus before/after each request, and reported whenever it has changed? That means:

• no idle background work
• we know the connection state for each request
• we know when a request triggered a connection, and whether that connection succeeded in time for the request
• if a connection goes idle, we don't know *when*, it gets reported on the next request

95

This is spinning in the background keeping the channel alive (true)... I'm actually not sure that's advisable.

If the connection is being used, it'll stay alive in the same way. So true here affects idle (and init) behavior.

Specifically, we maintain the TCP connection for the idle period to reduce latency of the first couple of requests after waking up (reestablish connection). I'm not sure this is a good trade, idle could easily be long and I don't think we know better than the OS when to drop a connection. (Cost might be battery life, background CPU load... IDK but probably something)

I think we should ask the connection to establish on startup (*high* chance we need it soon) but otherwise only in response to requests

96

Can we include the remote address and prior status?

e.g. "Remote index ({0}): {1} => {2}"

[kbobyrev]

Need to resolve review comments.

[kbobyrev]

Resolve the comments.

clang-tools-extra/clangd/index/remote/Client.cpp

86

("resolved" in this version)

I can understand your point and I support the arguments but this would mean that we only get status during requests and can't query it at arbitrary time which seems like a loss. Maybe it's OK because there might not be many interesting cases when we want to query the server status but not do any requests and your proposal makes it easier to implement & maintain. But also maybe it could be nice to have some kind of callback on status change.

As you mentioned, I was looking for something like this in gRPC itself but couldn't find it there outside the CompletionQueue and EvenManager things. Sadly, I don't think we'd be in a position to use the replacement even when it's there since we have to support older gRPC versions :(

When I see the logs and the server connectivity updates are following the requests, it looks OK but I'm not entirely sure giving up on _watching_ the status is great. But it still makes life easier so maybe this change should be about it.

96

I can see how it'd be nice but I'm not sure how to do this properly. My only idea is to query prior status in the beginning of the request? Otherwise it needs to be stored somewhere and gRPC interfaces require const functions, so IndexClient does not change the state within requests. I could either wrap it into something or have global grpc_connnectivity_state for the last observed status but neither look OK. Maybe querying the prior status right before the request is OK (that's what I've done here) but I'm not sure if that's exactly what you suggested.

[sammccall]

clang-tools-extra/clangd/index/remote/Client.cpp

74

we might want to put the server address at the end in [], mirroring D92181

86

I agree, it's absolutely a loss vs watching the state continuously.
But I think simplicity needs to win out here. Imagine if we have trouble coordinating shutdown in certain configurations because the thread gets blocked (grpc bug?)... this would not be much fun to debug.

In principle we could #ifdef based on GRPC version in the future, in practice you're probably right that we just suffer without this :-(

96

TL;DR: mutable atomic

Using a mutable member is the usual solution to "I want to be stateful but have to adhere to a const interface.
We usually have some philosophical debate about whether the function is still "conceptually const" but it certainly is here as the state in question is only affecting logging.

With mutable you usually have to add a mutex to provide the usual thread-safety guarantees (const = threadsafe). However in this case we just want to detect transitions, and atomics are great at this:

auto NewStatus = getStatus();
auto OldStatus = last_status_.exchange(NewStatus);
if (NewStatus != OldStatus)
  log(...);

This is guaranteed to log each time last_status_ changes value. (I think this will correspond closely enough to actual status changes, though calling using a mutex and calling getStatus() under the mutex would be a little more accurate)

Note that there's no difference between what you want to do before vs after an RPC - both are just chances to detect a transition - so pull out a function and call it in both places.

(I'm fairly sure atomic<grpc_connectivity_state> should be directly usable, on any decent impl this should be the same implementation as atomic<underlying_type<grpc_connectivity_state>>)

[kbobyrev]

Resolve review comments.

[sammccall]

LG with one fix around the atomic

clang-tools-extra/clangd/index/remote/Client.cpp

34

you access ConnectionStatus twice, and have a race between them.

auto OldStatus = ConnectionStatus.exchange(NewStatus);
if (OldStatus != NewStatus)
  log(...)

[kbobyrev]

Remove the race between ConnectionStatus access.

[kbobyrev]

Cleanup includes.

[kbobyrev]

Cleanup rebase artifact.

[kbobyrev]

Actually clean up rebase artifacts.

clang-tools-extra/clangd/index/Index.h

public:
   /// each.
   virtual void relations(
       const RelationsRequest &Req,
       llvm::function_ref<void(const SymbolID &Subject, const Symbol &Object)>
           Callback) const = 0;
 
   /// Returns estimated size of index (in bytes).
   virtual size_t estimateMemoryUsage() const = 0;
+
+  enum class Kind : uint8_t {
+    Unknown = 0,
+    Remote,
+  };
+
+  Kind Kind = Kind::Unknown;
 };
 
 // Delegating implementation of SymbolIndex whose delegate can be swapped out.
 class SwapIndex : public SymbolIndex {
 public:
   // If an index is not provided, reset() must be called.
   SwapIndex(std::unique_ptr<SymbolIndex> Index = nullptr)
       : Index(std::move(Index)) {}

clang-tools-extra/clangd/index/remote/Client.h

 ///
 /// This method attempts to resolve the address and establish the connection.
 ///
 /// \returns nullptr if the address is not resolved during the function call or
 /// if the project was compiled without Remote Index support.
 std::unique_ptr<clangd::SymbolIndex> getClient(llvm::StringRef Address,
                                                llvm::StringRef IndexRoot);
 
+/// Sends data to the server and compares it against response.
+///
+/// \returns true if Index is an instance of IndexClient and response matches
+/// sent data.
+bool handshake(const clangd::SymbolIndex *Index);
+
 } // namespace remote
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REMOTE_INDEX_H

clang-tools-extra/clangd/index/remote/Client.cpp

 //===--- Client.cpp ----------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <grpc++/grpc++.h>
 
 #include "Client.h"
 #include "Service.grpc.pb.h"
 #include "index/Index.h"
 #include "marshalling/Marshalling.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
+#include "clang/Basic/LLVM.h"
 #include "clang/Basic/Version.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 
 #include <chrono>
+#include <cstdint>
+#include <memory>
 
 namespace clang {
 namespace clangd {
 namespace remote {
 namespace {
 
 class IndexClient : public clangd::SymbolIndex {
   template <typename RequestT, typename ReplyT>
   using StreamingCall = std::unique_ptr<grpc::ClientReader<ReplyT>> (
       remote::v1::SymbolIndex::Stub::*)(grpc::ClientContext *,
                                         const RequestT &);
 
   template <typename RequestT, typename ReplyT, typename ClangdRequestT,
             typename CallbackT>
   bool streamRPC(ClangdRequestT Request,
                  StreamingCall<RequestT, ReplyT> RPCCall,
                  CallbackT Callback) const {
     bool FinalResult = false;
while (Reader->Read(&Reply)) {
         continue;
       }
       Callback(*Response);
       ++Successful;
     }
     SPAN_ATTACH(Tracer, "Status", Reader->Finish().ok());
     SPAN_ATTACH(Tracer, "Successful", Successful);
     SPAN_ATTACH(Tracer, "Failed to parse", FailedToParse);
     return FinalResult;
   }
 
 public:
   IndexClient(
       std::shared_ptr<grpc::Channel> Channel, llvm::StringRef ProjectRoot,
       std::chrono::milliseconds DeadlineTime = std::chrono::milliseconds(1000))
       : Stub(remote::v1::SymbolIndex::NewStub(Channel)),
         ProtobufMarshaller(new Marshaller(/*RemoteIndexRoot=*/"",
                                           /*LocalIndexRoot=*/ProjectRoot)),
         DeadlineWaitingTime(DeadlineTime) {
     assert(!ProjectRoot.empty());
+    this->Kind = Kind::Remote;
+  }
+
+  // Returns true if received checksum matches given one.
+  bool handshake(uint32_t Checksum) const {
+    trace::Span Tracer(HandshakeRequest::descriptor()->name());
+    HandshakeRequest RPCRequest;
+    RPCRequest.set_checksum(Checksum);
+    SPAN_ATTACH(Tracer, "Request", RPCRequest.DebugString());
+    grpc::ClientContext Context;
+    Context.AddMetadata("version", clang::getClangToolFullVersion("clangd"));
+    const std::chrono::system_clock::time_point StartTime =
+        std::chrono::system_clock::now();
+    const auto Deadline = StartTime + DeadlineWaitingTime;
+    Context.set_deadline(Deadline);
+    vlog("Sending RPC Request {0}: {1}", HandshakeRequest::descriptor()->name(),
+         RPCRequest.DebugString());
+    HandshakeReply Reply;
+    const grpc::Status Status =
+        Stub.get()->Handshake(&Context, RPCRequest, &Reply);
+    assert(Checksum == Reply.checksum() && "Received checksum should match.");
+    const auto Millis = std::chrono::duration_cast<std::chrono::milliseconds>(
+                            std::chrono::system_clock::now() - StartTime)
+                            .count();
+    const std::string ChecksumMatches =
+        Checksum == Reply.checksum() ? "OK" : "CHECKSUM MISMATCH";
+    vlog("RPC {0} => {1}: {2}ms", HandshakeRequest::descriptor()->name(),
+         ChecksumMatches, Reply.checksum(), Millis);
+    SPAN_ATTACH(Tracer, "Status", Status.ok());
+    return Checksum == Reply.checksum();
   }
 
   void lookup(const clangd::LookupRequest &Request,
               llvm::function_ref<void(const clangd::Symbol &)> Callback)
       const override {
     streamRPC(Request, &remote::v1::SymbolIndex::Stub::Lookup, Callback);
   }
 

 std::unique_ptr<clangd::SymbolIndex> getClient(llvm::StringRef Address,
                                                llvm::StringRef ProjectRoot) {
   const auto Channel =
       grpc::CreateChannel(Address.str(), grpc::InsecureChannelCredentials());
   Channel->GetState(true);
   return std::unique_ptr<clangd::SymbolIndex>(
       new IndexClient(Channel, ProjectRoot));
 }
 
+bool handshake(const clangd::SymbolIndex *Index) {
+  const auto *Client = dyn_cast_or_null<IndexClient>(Index);
+  if (!Client)
+    return false;
+  const uint32_t Checksum = 42;
+  return Client->handshake(Checksum);
+}
+
 } // namespace remote
 } // namespace clangd
 } // namespace clang
+
+namespace llvm {
+template <>
+struct llvm::isa_impl<clang::clangd::remote::IndexClient,
+                      clang::clangd::SymbolIndex> {
+  static inline bool doit(const clang::clangd::SymbolIndex &Idx) {
+    return Idx.Kind == clang::clangd::SymbolIndex::Kind::Remote;
+  }
+};
+
+} // namespace llvm

clang-tools-extra/clangd/index/remote/Index.proto

 //===--- Index.proto - Remote index Protocol Buffers definition -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 syntax = "proto2";
 
 package clang.clangd.remote;
 
+message HandshakeRequest {
+  required uint32 checksum = 1;
+}
+
+message HandshakeReply {
+  required uint32 checksum = 1;
+}
+
 // Common final result for streaming requests.
 message FinalResult { optional bool has_more = 1; }
 
 message LookupRequest { repeated string ids = 1; }
 
 // The response is a stream of symbol messages and the terminating message
 // indicating the end of stream.
 message LookupReply {

clang-tools-extra/clangd/index/remote/Service.proto

 
 package clang.clangd.remote.v1;
 
 import "Index.proto";
 
 // Semantics of SymbolIndex match clangd::SymbolIndex with all required
 // structures corresponding to their clangd::* counterparts.
 service SymbolIndex {
+  rpc Handshake(HandshakeRequest) returns (HandshakeReply) {}
+
   rpc Lookup(LookupRequest) returns (stream LookupReply) {}
 
   rpc FuzzyFind(FuzzyFindRequest) returns (stream FuzzyFindReply) {}
 
   rpc Refs(RefsRequest) returns (stream RefsReply) {}
 
   rpc Relations(RelationsRequest) returns (stream RelationsReply) {}
 }
 

clang-tools-extra/clangd/index/remote/server/Server.cpp

 //===--- Server.cpp - gRPC-based Remote Index Server  ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Index.pb.h"
 #include "Service.grpc.pb.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "support/Context.h"
 #include "support/Logger.h"
 #include "support/Shutdown.h"

 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/VirtualFileSystem.h"
 
 #include <chrono>
 #include <grpc++/grpc++.h>
 #include <grpc++/health_check_service_interface.h>
+#include <grpcpp/impl/codegen/server_context.h>
 #include <memory>
 #include <thread>
 
 namespace clang {
 namespace clangd {
 namespace remote {
 namespace {
 
RemoteIndexServer(clangd::SymbolIndex &Index, llvm::StringRef IndexRoot)
     ProtobufMarshaller = std::unique_ptr<Marshaller>(new Marshaller(
         /*RemoteIndexRoot=*/llvm::StringRef(NativePath),
         /*LocalIndexRoot=*/""));
   }
 
 private:
   using stopwatch = std::chrono::steady_clock;
 
+  grpc::Status Handshake(grpc::ServerContext *Context,
+                         const remote::HandshakeRequest *Request,
+                         HandshakeReply *Reply) override {
+    WithContextValue WithRequestContext(CurrentRequest, Context);
+    logRequest(*Request);
+    trace::Span Tracer("LookupRequest");
+    Reply->set_checksum(Request->checksum());
+    logResponse(*Reply);
+    SPAN_ATTACH(Tracer, "Checksum", Reply->checksum());
+    return grpc::Status::OK;
+  }
+
   grpc::Status Lookup(grpc::ServerContext *Context,
                       const LookupRequest *Request,
                       grpc::ServerWriter<LookupReply> *Reply) override {
     auto StartTime = stopwatch::now();
     WithContextValue WithRequestContext(CurrentRequest, Context);
     logRequest(*Request);
     trace::Span Tracer("LookupRequest");
     auto Req = ProtobufMarshaller->fromProtobuf(Request);

clang-tools-extra/clangd/index/remote/unimplemented/UnimplementedClient.cpp

 namespace remote {
 
 std::unique_ptr<clangd::SymbolIndex> getClient(llvm::StringRef Address,
                                                llvm::StringRef IndexRoot) {
   elog("Can't create SymbolIndex client without Remote Index support.");
   return nullptr;
 }
 
+bool handshake(const clangd::SymbolIndex *Index) {
+  elog("Can't perform a handshake without Remote Index support.");
+  return false;
+}
+
 } // namespace remote
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/tool/ClangdMain.cpp

#endif
   }
   Opts.CodeComplete.SpeculativeIndexRequest = Opts.StaticIndex;
   Opts.CodeComplete.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
   Opts.CodeComplete.AllScopes = AllScopesCompletion;
   Opts.CodeComplete.RunParser = CodeCompletionParse;
   Opts.CodeComplete.RankingModel = RankingModel;
   Opts.CodeComplete.DecisionForestBase = DecisionForestBase;
 
+#if CLANGD_ENABLE_REMOTE
+  // clang::clangd::remote::handshake(Opts.StaticIndex);
+#endif
+
   RealThreadsafeFS TFS;
   std::vector<std::unique_ptr<config::Provider>> ProviderStack;
   std::unique_ptr<config::Provider> Config;
   if (EnableConfig) {
     ProviderStack.push_back(
         config::Provider::fromAncestorRelativeYAMLFiles(".clangd", TFS));
     llvm::SmallString<256> UserConfig;
     if (llvm::sys::path::user_config_directory(UserConfig)) {