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

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, jfb, arphaman, jkorous, MaskRay.
Herald added a project: clang.

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).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69329

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===================================================================
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -27,6 +27,7 @@
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
+#include <chrono>
 #include <cstdlib>
 #include <iostream>
 #include <memory>
@@ -670,6 +671,24 @@
       /*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
+                     : static_cast<int>(ErrorResultCode::NoShutdownRequest);
+  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::seconds(6));
+    std::abort();
+  }).detach();
+
+  return ExitCode;
 }
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -44,6 +44,7 @@
                   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
@@ -211,11 +212,11 @@
   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. It should be
+  // destroyed first, to ensure worker threads don't access other members.
+  llvm::Optional<ClangdServer> Server;
 };
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1241,8 +1241,6 @@
     CleanExit = false;
   }
 
-  // Destroy ClangdServer to ensure all worker threads finish.
-  Server.reset();
   return CleanExit && ShutdownRequestReceived;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D69329.226100.patch
Type: text/x-patch
Size: 3305 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191023/2eeb5131/attachment-0001.bin>