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;
}
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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
