[clangd] abort if shutdown takes more than a minute.

[sammccall]

Summary

A certain class of bug (e.g. infloop on an AST worker thread) currently means
clangd never terminates, even if the editor shuts down the protocol and closes
our stdin, and the main thread recognizes that.

Instead, let's wait 60 seconds for threads to finish cleanly, and then crash
if they haven't.

(Obviously, we should still fix these bugs).

[ilya-biryukov]

Not sure 60 is enough, e.g. building a preamble can often take more than a minute and the users will see clangd crashing for (seemingly) no reason on shutdown.
In the long run, we should probably attempt to cancel long-running operations within a much shorter timeframe and a timeout of a minute would make more sense, though.

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

689

Did you mean 60 seconds?

[merge_guards_bot]

Build result: fail - 59551 tests passed, 1 failed and 805 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[sammccall]

change timeout to 5 min

[merge_guards_bot]

Build result: fail - 59568 tests passed, 1 failed and 805 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[ilya-biryukov]

Adding myself to the reviewers since I've already looked at it. Unless anyone objects.

clang-tools-extra/clangd/ClangdLSPServer.h

219

Maybe call Server.reset() in destructor?
This ensures ClangdServer is terminated first even if the members are re-ordered later

[kadircet]

Sorry I somehow missed the fact that I was a reviewer :D

LGTM

clang-tools-extra/clangd/ClangdLSPServer.h

219

+1

clang-tools-extra/clangd/ClangdLSPServer.h

public:
   /// for compile_commands.json in all parent directories of each file.
   /// If UseDirBasedCDB is false, compile commands are not read from disk.
   // FIXME: Clean up signature around CDBs.
   ClangdLSPServer(Transport &Transp, const FileSystemProvider &FSProvider,
                   const clangd::CodeCompleteOptions &CCOpts,
                   llvm::Optional<Path> CompileCommandsDir, bool UseDirBasedCDB,
                   llvm::Optional<OffsetEncoding> ForcedOffsetEncoding,
                   const ClangdServer::Options &Opts);
+  /// The destructor blocks on any outstanding background tasks.
   ~ClangdLSPServer();
 
   /// Run LSP server loop, communicating with the Transport provided in the
   /// constructor. This method must not be executed more than once.
   ///
   /// \return Whether we shut down cleanly with a 'shutdown' -> 'exit' sequence.
   bool run();
 
private:
   DraftStore DraftMgr;
 
   // The CDB is created by the "initialize" LSP method.
   bool UseDirBasedCDB;                     // FIXME: make this a capability.
   llvm::Optional<Path> CompileCommandsDir; // FIXME: merge with capability?
   std::unique_ptr<GlobalCompilationDatabase> BaseCDB;
   // CDB is BaseCDB plus any comands overridden via LSP extensions.
   llvm::Optional<OverlayCDB> CDB;
-  // The ClangdServer is created by the "initialize" LSP method.
-  // It is destroyed before run() returns, to ensure worker threads exit.
   ClangdServer::Options ClangdServerOpts;
-  llvm::Optional<ClangdServer> Server;
   llvm::Optional<OffsetEncoding> NegotiatedOffsetEncoding;
+  // The ClangdServer is created by the "initialize" LSP method.
+  llvm::Optional<ClangdServer> Server;
 };
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDLSPSERVER_H

clang-tools-extra/clangd/ClangdLSPServer.cpp

ClangdLSPServer::ClangdLSPServer(
   MsgHandler->bind("workspace/didChangeConfiguration", &ClangdLSPServer::onChangeConfiguration);
   MsgHandler->bind("textDocument/symbolInfo", &ClangdLSPServer::onSymbolInfo);
   MsgHandler->bind("textDocument/typeHierarchy", &ClangdLSPServer::onTypeHierarchy);
   MsgHandler->bind("typeHierarchy/resolve", &ClangdLSPServer::onResolveTypeHierarchy);
   MsgHandler->bind("textDocument/selectionRange", &ClangdLSPServer::onSelectionRange);
   // clang-format on
 }
 
-ClangdLSPServer::~ClangdLSPServer() { IsBeingDestroyed = true; }
+ClangdLSPServer::~ClangdLSPServer() { IsBeingDestroyed = true;
+  // Explicitly destroy ClangdServer first, blocking on threads it owns.
+  // This ensures they don't access any other members.
+  Server.reset();
+}
 
 bool ClangdLSPServer::run() {
   // Run the Language Server loop.
   bool CleanExit = true;
   if (auto Err = Transp.loop(*MsgHandler)) {
     elog("Transport error: {0}", std::move(Err));
     CleanExit = false;
   }
 
-  // Destroy ClangdServer to ensure all worker threads finish.
-  Server.reset();
   return CleanExit && ShutdownRequestReceived;
 }
 
 std::vector<Fix> ClangdLSPServer::getFixes(llvm::StringRef File,
                                            const clangd::Diagnostic &D) {
   std::lock_guard<std::mutex> Lock(FixItsMutex);
   auto DiagToFixItsIter = FixItsMap.find(File);
   if (DiagToFixItsIter == FixItsMap.end())

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

 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
+#include <chrono>
 #include <cstdlib>
 #include <iostream>
 #include <memory>
 #include <mutex>
 #include <string>
 #include <thread>
 
 namespace clang {
#endif
   llvm::Optional<OffsetEncoding> OffsetEncodingFromFlag;
   if (ForceOffsetEncoding != OffsetEncoding::UnsupportedEncoding)
     OffsetEncodingFromFlag = ForceOffsetEncoding;
   ClangdLSPServer LSPServer(
       *TransportLayer, FSProvider, CCOpts, CompileCommandsDirPath,
       /*UseDirBasedCDB=*/CompileArgsFrom == FilesystemCompileArgs,
       OffsetEncodingFromFlag, Opts);
   llvm::set_thread_name("clangd.main");
-  return LSPServer.run() ? 0
-                         : static_cast<int>(ErrorResultCode::NoShutdownRequest);
+  int ExitCode = LSPServer.run()
+                     ? 0
+  log("LSP finished, exiting with status {0}", ExitCode);
+
+  // There may still be lingering background threads (e.g. slow requests
+  // whose results will be dropped, background index shutting down).
+  //
+  // These should terminate quickly, and ~ClangdLSPServer blocks on them.
+  // However if a bug causes them to run forever, we want to ensure the process
+  // eventually exits. As clangd isn't directly user-facing, an editor can
+  // "leak" clangd processes. Crashing in this case contains the damage.
+  //
+  // This is more portable than sys::WatchDog, and yields a stack trace.
+  std::thread([] {
+    std::this_thread::sleep_for(std::chrono::minutes(5));
+    std::abort();
+  }).detach();
+
+  return ExitCode;
 }