[clangd] Implement hot index reloading for clangd-index-server

[kbobyrev]

Summary

This patch adds a mechanism to load new versions of index into
clangd-index-server using SwapIndex and FileStatus information about last
modification time without downtime.

[kadircet]

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

72–73

why do we have this extra indirection?

84

why not make this private? also keep using SymbolIndex. also you can make this a reference now.

221

why is this taking a double pointer? I think you can just use a reference here for Index (i.e. clangd::SwapIndex &

223

i think we should rather store the file_status from the original file in here. as we want to reload whenever files size/modification_time has changed.
it doesn't have to be moving forward let alone share the same timezone restrictions and such provided by system_clock::now.

Also it would be nice to use a VFS in here, that way we can move this logic into a helper in the future if other components need similar logic.

224

how's this loop terminated?

I would suggest factoring the loop body into a helper and then having a lambda for periodically running it, while looking for an exit signal.

257

i think hotReload thread creation should also be part of the main.

266

you either need to join a thread before its destruction or detach it from the main thread.

314–315

nit: auto Index = std::make_unique<clang::clangd::SwapIndex>(openIndex...);

[kbobyrev]

Address review comments.

[kbobyrev]

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

223

Can you elaborate more on VFS usage? I'm not sure how this would improve existing and potential future code over plain llvm::sys::* calls.

[kadircet]

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

223

it is mostly for unit testing the helper when there's a wider usage. also some users of clangd($Employer's internal editor) doesn't really have a real fs, they surface it through a VFS.

it also makes the code a little bit more easier to read, but definitely doesn't require immediate action, so up to you.

[kbobyrev]

Address remaining comments.

[kadircet]

Thanks! Mostly looks good, just couple of nits.

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

221

Again Index is a non-nullptr, so let's use a ref instead.

229

from previous comment:

i think we should rather store the file_status from the original file in here. as we want to reload whenever files size/modification_time has changed.
it doesn't have to be moving forward let alone share the same timezone restrictions and such provided by system_clock::now.

245

nit: maybe rename to runServerAndWait

326

instead of both asserting and bailing out, maybe just elog before bail-out?

+ I think what you rather want is grab the inital status outside this lambda. i.e:

clang::clangd::RealThreadsafeFS TFS;
auto FS = TFS...;
auto Status = ....;
if(!Status) {
  // elog and exit, file does not exist
}
auto Index = ...;

if(!Index) {
 // elog and exit, failed to load index 
}

std::thread ..([FS = TFS.view(), LastStatus = *Status, &Index]{ ... });
runServer();
joinThread();

330

nit: static constexpr auto RefreshFrequency, i.e. use constexpr and we don't have a different naming convention for constants. same for WatcherFrequency.

[kbobyrev]

Address a round of comments.

[kadircet]

oops, looks like i forgot to hit submit in the morning..

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

225

i beleive equivalent checks for inode number equivalence (at least on linux, using realfilesystem). That's not guranteed to change when you overwrite/modify a file.

Sorry if it felt like i meant this, but I was literally suggesting to use != instead of <= when comparing modifcation time, and also incorporating size into the check.

Also please let me know if there's something i am missing with the equivalent.

It would be also nice to have some tests, as we are starting to add non-trivial functionality to server but not blocking for this patch. (i suppose it would've catched the equivalent problem)

231

i think we should update LastStatus here, as we are going to fail loading the index unless the file changes. so there's no need to retry loading the index if the file hasn't changed.

233

i believe, this should be an elog.

237

nit: maybe also print the size.

318

nit: drop File

327

nit: i would first call hotReload and then sleep to get rid of one extra reload of the index when we are shutting down.

337

nit: return 0; ?

[kbobyrev]

Address another round of comments.

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

231

Sorry, the comment is off, it belongs to the if statement above. Here, the index file is already different and there is an attempt to reload it. If it succeeds, new index replaces the old one and LastStatus is updated.

337

No need for that in C++.

https://en.cppreference.com/w/cpp/language/main_function

4. The body of the main function does not need to contain the return statement: if control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

[kbobyrev]

Update comment.

[kadircet]

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

231

right and what I am saying is, even if we fail to load the the index we should still update the LastStatus to prevent redundant retries for a broken index file. e.g:

• For some reason a malformed index file is written with mod. time X and size Y.
• Hot reload logic picks it up, the file exists and everything is fine.
• When we try to read the index, we notice it is malformed or for whatever reason, deserialization fails.
• Now we exit without updating the LastStatus, hence in the next update all of this will happen again even though index loading is going to fail again (as malformed index is still the same).

We could prevent that redundant loads (and failure logs) by caching the LastStatus as soon as the file exists.

Does that make sense now?

337

right, hence the nit

[kbobyrev]

Save last status to prevent redundant updates.

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

231

Ahh, I see now, makes sense. Thanks for the explanation!

337

Hm, why do you think it is better then? I'm not sure if adding no-op lines of code will increase readability/clarity.

I'm curious now, is there any reason you prefer explicit return 0;?

[kadircet]

Thanks, LGTM!

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

337

I suppose, mostly aesthetics and being used to it.

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 "index/Index.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "support/Logger.h"
+#include "support/Shutdown.h"
 #include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Chrono.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/FileSystem.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 <memory>
+#include <thread>
 
 #include "Index.grpc.pb.h"
 
 namespace clang {
 namespace clangd {
 namespace remote {
 namespace {
 
llvm::cl::opt<bool> PrettyPrint{
     "pretty",
     llvm::cl::desc("Pretty-print JSON output in the trace"),
     llvm::cl::init(false),
 };
 
 llvm::cl::opt<std::string> ServerAddress(
     "server-address", llvm::cl::init("0.0.0.0:50051"),
     llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
 
-std::unique_ptr<clangd::SymbolIndex> openIndex(llvm::StringRef Index) {
-  return loadIndex(Index, /*UseIndex=*/true);
-}
-
 class RemoteIndexServer final : public SymbolIndex::Service {
 public:
-  RemoteIndexServer(std::unique_ptr<clangd::SymbolIndex> Index,
-                    llvm::StringRef IndexRoot)
-      : Index(std::move(Index)) {
+  RemoteIndexServer(clangd::SymbolIndex &Index, llvm::StringRef IndexRoot)
+      : Index(Index) {
     llvm::SmallString<256> NativePath = IndexRoot;
     llvm::sys::path::native(NativePath);
     ProtobufMarshaller = std::unique_ptr<Marshaller>(new Marshaller(
         /*RemoteIndexRoot=*/llvm::StringRef(NativePath),
         /*LocalIndexRoot=*/""));
   }
 
 private:
   grpc::Status Lookup(grpc::ServerContext *Context,
                       const LookupRequest *Request,
                       grpc::ServerWriter<LookupReply> *Reply) override {
     trace::Span Tracer("LookupRequest");
     auto Req = ProtobufMarshaller->fromProtobuf(Request);
     if (!Req) {
       elog("Can not parse LookupRequest from protobuf: {0}", Req.takeError());
       return grpc::Status::CANCELLED;
     }
     unsigned Sent = 0;
     unsigned FailedToSend = 0;
-    Index->lookup(*Req, [&](const clangd::Symbol &Item) {
+    Index.lookup(*Req, [&](const clangd::Symbol &Item) {
       auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
       if (!SerializedItem) {
         elog("Unable to convert Symbol to protobuf: {0}",
              SerializedItem.takeError());
         ++FailedToSend;
         return;
       }
       LookupReply NextMessage;
grpc::Status FuzzyFind(grpc::ServerContext *Context,
     auto Req = ProtobufMarshaller->fromProtobuf(Request);
     if (!Req) {
       elog("Can not parse FuzzyFindRequest from protobuf: {0}",
            Req.takeError());
       return grpc::Status::CANCELLED;
     }
     unsigned Sent = 0;
     unsigned FailedToSend = 0;
-    bool HasMore = Index->fuzzyFind(*Req, [&](const clangd::Symbol &Item) {
+    bool HasMore = Index.fuzzyFind(*Req, [&](const clangd::Symbol &Item) {
       auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
       if (!SerializedItem) {
         elog("Unable to convert Symbol to protobuf: {0}",
              SerializedItem.takeError());
         ++FailedToSend;
         return;
       }
       FuzzyFindReply NextMessage;
grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
     trace::Span Tracer("RefsRequest");
     auto Req = ProtobufMarshaller->fromProtobuf(Request);
     if (!Req) {
       elog("Can not parse RefsRequest from protobuf: {0}", Req.takeError());
       return grpc::Status::CANCELLED;
     }
     unsigned Sent = 0;
     unsigned FailedToSend = 0;
-    bool HasMore = Index->refs(*Req, [&](const clangd::Ref &Item) {
+    bool HasMore = Index.refs(*Req, [&](const clangd::Ref &Item) {
       auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);
       if (!SerializedItem) {
         elog("Unable to convert Ref to protobuf: {0}",
              SerializedItem.takeError());
         ++FailedToSend;
         return;
       }
       RefsReply NextMessage;
grpc::Status Relations(grpc::ServerContext *Context,
     auto Req = ProtobufMarshaller->fromProtobuf(Request);
     if (!Req) {
       elog("Can not parse RelationsRequest from protobuf: {0}",
            Req.takeError());
       return grpc::Status::CANCELLED;
     }
     unsigned Sent = 0;
     unsigned FailedToSend = 0;
-    Index->relations(
+    Index.relations(
         *Req, [&](const SymbolID &Subject, const clangd::Symbol &Object) {
           auto SerializedItem = ProtobufMarshaller->toProtobuf(Subject, Object);
           if (!SerializedItem) {
             elog("Unable to convert Relation to protobuf: {0}",
                  SerializedItem.takeError());
             ++FailedToSend;
             return;
           }
           RelationsReply NextMessage;
           *NextMessage.mutable_stream_result() = *SerializedItem;
           Reply->Write(NextMessage);
           ++Sent;
         });
     RelationsReply LastMessage;
     LastMessage.set_final_result(true);
     Reply->Write(LastMessage);
     SPAN_ATTACH(Tracer, "Sent", Sent);
     SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
     return grpc::Status::OK;
   }
 
-  std::unique_ptr<clangd::SymbolIndex> Index;
   std::unique_ptr<Marshaller> ProtobufMarshaller;
+  clangd::SymbolIndex &Index;
 };
 
-void runServer(std::unique_ptr<clangd::SymbolIndex> Index,
-               const std::string &ServerAddress) {
-  RemoteIndexServer Service(std::move(Index), IndexRoot);
+// Detect changes in \p IndexPath file and load new versions of the index
+// whenever they become available.
+void hotReload(clangd::SwapIndex *Index, llvm::StringRef IndexPath,
+               llvm::sys::fs::file_status &LastStatus) {
+  llvm::sys::fs::file_status Status;
+  const auto EC = llvm::sys::fs::status(IndexPath, Status, /*Follow=*/true);
+  if (EC)
+    return;
+  const auto NewModificationTime = Status.getLastModificationTime();
+  // Current index is newer than the one before: no reload is needed.
+  if (NewModificationTime <= LastStatus.getLastModificationTime())
+    return;
+  vlog("Found new index version: existing index was modified at {0}, new "
+       "index was modified at {1}. Attempting to reload.",
+       LastStatus.getLastModificationTime(), NewModificationTime);
+  std::unique_ptr<clang::clangd::SymbolIndex> NewIndex = loadIndex(IndexPath);
+  if (!NewIndex) {
+    vlog("Failed to load new index. Old index will be served.");
+    return;
+  }
+  Index->reset(std::move(NewIndex));
+  log("New index version loaded. Last modification time: {0}.",
+      NewModificationTime);
+  LastStatus = Status;
+}
+
+void runServer(clangd::SymbolIndex *Index, llvm::StringRef ServerAddress,
+               llvm::StringRef IndexPath) {
+  RemoteIndexServer Service(*Index, IndexRoot);
 
   grpc::EnableDefaultHealthCheckService(true);
   grpc::ServerBuilder Builder;
-  Builder.AddListeningPort(ServerAddress, grpc::InsecureServerCredentials());
+  Builder.AddListeningPort(ServerAddress.str(),
+                           grpc::InsecureServerCredentials());
   Builder.RegisterService(&Service);
   std::unique_ptr<grpc::Server> Server(Builder.BuildAndStart());
   log("Server listening on {0}", ServerAddress);
 
+  std::thread ServerShutdownWatcher([&]() {
+    const auto WATCHER_FREQUENCY = std::chrono::seconds(5);
+    while (!clang::clangd::shutdownRequested())
+      std::this_thread::sleep_for(WATCHER_FREQUENCY);
+    Server->Shutdown();
+  });
+
   Server->Wait();
+  ServerShutdownWatcher.join();
 }
 
 } // namespace
 } // namespace remote
 } // namespace clangd
 } // namespace clang
 
 using clang::clangd::elog;
 
 int main(int argc, char *argv[]) {
   using namespace clang::clangd::remote;
   llvm::cl::ParseCommandLineOptions(argc, argv, Overview);
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+  llvm::sys::SetInterruptFunction(&clang::clangd::requestShutdown);
 
   if (!llvm::sys::path::is_absolute(IndexRoot)) {
     llvm::errs() << "Index root should be an absolute path.\n";
     return -1;
   }
 
   llvm::errs().SetBuffered();
   // Don't flush stdout when logging for thread safety.
if (EC) {
       clang::clangd::vlog("Successfully created a tracer.");
     }
   }
 
   llvm::Optional<clang::clangd::trace::Session> TracingSession;
   if (Tracer)
     TracingSession.emplace(*Tracer);
 
-  std::unique_ptr<clang::clangd::SymbolIndex> Index = openIndex(IndexPath);
+  auto Index = std::make_unique<clang::clangd::SwapIndex>(
+      clang::clangd::loadIndex(IndexPath));
 
   if (!Index) {
     llvm::errs() << "Failed to open the index.\n";
     return -1;
   }
 
-  runServer(std::move(Index), ServerAddress);
+  std::thread HotReloadThread([&]() {
+    llvm::sys::fs::file_status LastStatus;
+    const auto EC =
+        llvm::sys::fs::status(IndexPath, LastStatus, /*Follow=*/true);
+    assert(!EC);
+    const auto REFRESH_FREQUENCY = std::chrono::seconds(90);
+    while (!clang::clangd::shutdownRequested()) {
+      std::this_thread::sleep_for(REFRESH_FREQUENCY);
+      hotReload(Index.get(), llvm::StringRef(IndexPath), LastStatus);
+    }
+  });
+
+  runServer(Index.get(), ServerAddress, IndexPath);
+
+  HotReloadThread.join();
 }