Author: Sam McCall Date: 2019-10-23T17:52:59+02:00 New Revision: 8bda5f20674df1765bce8f0866204dff93ed244c
URL: https://github.com/llvm/llvm-project/commit/8bda5f20674df1765bce8f0866204dff93ed244c DIFF: https://github.com/llvm/llvm-project/commit/8bda5f20674df1765bce8f0866204dff93ed244c.diff LOG: [clangd] abort if shutdown takes more than a minute. 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). Reviewers: kadircet Subscribers: MaskRay, jkorous, arphaman, jfb, usaxena95, cfe-commits, ilya-biryukov Tags: #clang Differential Revision: https://reviews.llvm.org/D69329 Added: Modified: clang-tools-extra/clangd/ClangdLSPServer.cpp clang-tools-extra/clangd/ClangdLSPServer.h clang-tools-extra/clangd/tool/ClangdMain.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 3f825a6febdf..9e24878dc8f6 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -1231,7 +1231,11 @@ ClangdLSPServer::ClangdLSPServer( // 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. @@ -1241,8 +1245,6 @@ bool ClangdLSPServer::run() { CleanExit = false; } - // Destroy ClangdServer to ensure all worker threads finish. - Server.reset(); return CleanExit && ShutdownRequestReceived; } diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h index dbd90064537e..f1ed317f6bad 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -44,6 +44,7 @@ class ClangdLSPServer : private DiagnosticsConsumer { 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,10 @@ class ClangdLSPServer : private DiagnosticsConsumer { 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 diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp index 4cb6604072a3..2639df31dbe8 100644 --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/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 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var /*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::minutes(5)); + std::abort(); + }).detach(); + + return ExitCode; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
