sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, jfb, arphaman, jkorous,
MaskRay, ilya-biryukov, mgorny.
Herald added a reviewer: jfb.
Herald added a project: clang.
This avoids leaking PCH files if editors don't use the LSP shutdown protocol.
This is one fix for https://github.com/clangd/clangd/issues/209
(Though I think we should *also* be unlinking the files)
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D70684
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/JSONTransport.cpp
clang-tools-extra/clangd/Shutdown.cpp
clang-tools-extra/clangd/Shutdown.h
clang-tools-extra/clangd/test/exit-eof.test
clang-tools-extra/clangd/test/exit-signal.test
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
@@ -11,6 +11,7 @@
#include "Features.inc"
#include "Path.h"
#include "Protocol.h"
+#include "Shutdown.h"
#include "Trace.h"
#include "Transport.h"
#include "index/Background.h"
@@ -35,6 +36,10 @@
#include <string>
#include <thread>
+#ifndef _WIN32
+#include <unistd.h>
+#endif
+
namespace clang {
namespace clangd {
namespace {
@@ -435,6 +440,7 @@
llvm::InitializeAllTargetInfos();
llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+ llvm::sys::SetInterruptFunction(&requestShutdown);
llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
OS << clang::getClangToolFullVersion("clangd") << "\n";
});
@@ -531,6 +537,10 @@
LoggingSession LoggingSession(Logger);
// Write some initial logs before we start doing any real work.
log("{0}", clang::getClangToolFullVersion("clangd"));
+// FIXME: abstract this better, and print PID on windows too.
+#ifndef _WIN32
+ log("PID: {0}", getpid());
+#endif
{
SmallString<128> CWD;
if (auto Err = llvm::sys::fs::current_path(CWD))
@@ -683,12 +693,7 @@
// 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();
+ abortAfterTimeout(std::chrono::minutes(5));
return ExitCode;
}
Index: clang-tools-extra/clangd/test/exit-signal.test
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/test/exit-signal.test
@@ -0,0 +1,31 @@
+# This is a fiddly signal test, we need POSIX and a real shell.
+UNSUPPORTED: win32
+REQUIRES: shell
+
+# Our goal is:
+# 1) spawn clangd
+# 2) wait for it to start up (install signal handlers)
+# 3) send SIGTERM
+# 4) wait for clangd to shut down (nonzero exit for a signal)
+# 4) verify the shutdown was clean
+
+RUN: rm %t.err
+ # To keep clangd running, we need to hold its input stream open.
+ # We redirect its input to a subshell that waits for it to start up.
+RUN: not clangd 2> %t.err < <( \
+ # Loop waiting for clangd to start up, so signal handlers are installed.
+ # Reading the PID line ensures this, and lets us send a signal.
+RUN: while true; do \
+ # Relevant log line is I[timestamp] PID: <NNN>
+RUN: CLANGD_PID=$(grep -a -m 1 "PID:" %t.err | cut -d' ' -f 3); \
+RUN: [ ! -z "$CLANGD_PID" ] && break; \
+RUN: done; \
+RUN: kill $CLANGD_PID \
+ # Now wait for clangd to exit.
+RUN: )
+
+# Check that clangd caught the signal and shut down cleanly.
+RUN: FileCheck %s < %t.err
+CHECK: Transport error: Got signal
+CHECK: LSP finished
+
Index: clang-tools-extra/clangd/test/exit-eof.test
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/test/exit-eof.test
@@ -0,0 +1,7 @@
+# RUN: not clangd -sync < %s 2> %t.err
+# RUN: FileCheck %s < %t.err
+#
+# No LSP messages here, just let clangd see the end-of-file
+# CHECK: Transport error:
+# (Typically "Transport error: Input/output error" but platform-dependent).
+
Index: clang-tools-extra/clangd/Shutdown.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/Shutdown.h
@@ -0,0 +1,84 @@
+//===--- Shutdown.h - Unclean exit scenarios --------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+// LSP specifies a protocol for shutting down: a `shutdown` request followed
+// by an `exit` notification. If this protocol is followed, clangd should
+// finish outstanding work and exit with code 0.
+//
+// The way this works in the happy case:
+// - when ClangdLSPServer gets `shutdown`, it sets a flag
+// - when ClangdLSPServer gets `exit`, it returns false to indicate end-of-LSP
+// - Transport::loop() returns with no error
+// - ClangdServer::run() checks the shutdown flag and returns with no error.
+// - we `return 0` from main()
+// - destructor of ClangdServer and other main()-locals runs.
+// This blocks until outstanding requests complete (results are ignored)
+// - global destructors run, such as fallback deletion of temporary files
+//
+// There are a number of things that can go wrong. Some are handled here, and
+// some elsewhere.
+// - `exit` notification with no `shutdown`:
+// ClangdServer::run() sees this and returns false, main() returns nonzero.
+// - stdin/stdout are closed
+// The Transport detects this while doing IO and returns an error from loop()
+// ClangdServer::run() logs a message and then returns false, etc
+// - a request thread gets stuck, so the ClangdServer destructor hangs.
+// Before returning from main(), we start a watchdog thread to abort() the
+// process if it takes too long to exit. See abortAfterTimeout().
+// - clangd crashes (e.g. segfault or assertion)
+// A fatal signal is sent (SEGV, ABRT, etc)
+// The installed signal handler prints a stack trace and exits.
+// - parent process goes away or tells us to shut down
+// A "graceful shutdown" signal is sent (TERM, HUP, etc).
+// The installed signal handler calls requestShutdown() which sets a flag.
+// The Transport IO is interrupted, and Transport::loop() checks the flag and
+// returns an error, etc.
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SHUTDOWN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SHUTDOWN_H
+
+#include <cerrno>
+#include <chrono>
+
+namespace clang {
+namespace clangd {
+
+/// Causes this process to crash if still running after Timeout.
+void abortAfterTimeout(std::chrono::seconds Timeout);
+
+/// Sets a flag to indicate that clangd was sent a shutdown signal, and the
+/// transport loop should exit at the next opportunity.
+/// If shutdown was already requested, aborts the process.
+/// This function is threadsafe and signal-safe.
+void requestShutdown();
+/// Checks whether requestShutdown() was called.
+/// This function is threadsafe and signal-safe.
+bool shutdownRequested();
+
+/// Retry an operation if it gets interrupted by a signal.
+/// This is like llvm::sys::RetryAfterSignal, except that if shutdown was
+/// requested (which interrupts IO), we'll fail rather than retry.
+template <typename Fun, typename Ret = decltype(std::declval<Fun>()())>
+Ret retryAfterSignalUnlessShutdown(
+ const typename std::enable_if<true, Ret>::type &Fail, // Suppress deduction.
+ const Fun &F) {
+ Ret Res;
+ do {
+ if (shutdownRequested())
+ return Fail;
+ errno = 0;
+ Res = F();
+ } while (Res == Fail && errno == EINTR);
+ return Res;
+}
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clang-tools-extra/clangd/Shutdown.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/Shutdown.cpp
@@ -0,0 +1,39 @@
+//===--- Shutdown.cpp - Unclean exit scenarios ----------------------------===//
+//
+// 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 "Shutdown.h"
+
+#include <atomic>
+#include <thread>
+
+namespace clang {
+namespace clangd {
+
+void abortAfterTimeout(std::chrono::seconds Timeout) {
+ // This is more portable than sys::WatchDog, and yields a stack trace.
+ std::thread([Timeout] {
+ std::this_thread::sleep_for(Timeout);
+ std::abort();
+ }).detach();
+}
+
+static std::atomic<bool> ShutdownRequested = {false};
+
+void requestShutdown() {
+ if (ShutdownRequested.exchange(true))
+ // This is the second shutdown request. Exit hard.
+ std::abort();
+}
+
+bool shutdownRequested() {
+ return ShutdownRequested;
+}
+
+} // namespace clangd
+} // namespace clang
+
Index: clang-tools-extra/clangd/JSONTransport.cpp
===================================================================
--- clang-tools-extra/clangd/JSONTransport.cpp
+++ clang-tools-extra/clangd/JSONTransport.cpp
@@ -7,8 +7,10 @@
//===----------------------------------------------------------------------===//
#include "Logger.h"
#include "Protocol.h" // For LSPError
+#include "Shutdown.h"
#include "Transport.h"
#include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
namespace clang {
namespace clangd {
@@ -81,6 +83,10 @@
llvm::Error loop(MessageHandler &Handler) override {
while (!feof(In)) {
+ if (shutdownRequested())
+ return llvm::createStringError(
+ std::make_error_code(std::errc::operation_canceled),
+ "Got signal, shutting down");
if (ferror(In))
return llvm::errorCodeToError(
std::error_code(errno, std::system_category()));
@@ -167,7 +173,7 @@
}
// Tries to read a line up to and including \n.
-// If failing, feof() or ferror() will be set.
+// If failing, feof(), ferror(), or shutdownRequested() will be set.
bool readLine(std::FILE *In, std::string &Out) {
static constexpr int BufSize = 1024;
size_t Size = 0;
@@ -175,7 +181,8 @@
for (;;) {
Out.resize(Size + BufSize);
// Handle EINTR which is sent when a debugger attaches on some platforms.
- if (!llvm::sys::RetryAfterSignal(nullptr, ::fgets, &Out[Size], BufSize, In))
+ if (!retryAfterSignalUnlessShutdown(
+ nullptr, [&] { return std::fgets(&Out[Size], BufSize, In); }))
return false;
clearerr(In);
// If the line contained null bytes, anything after it (including \n) will
@@ -190,7 +197,7 @@
}
// Returns None when:
-// - ferror() or feof() are set.
+// - ferror(), feof(), or shutdownRequested() are set.
// - Content-Length is missing or empty (protocol error)
llvm::Optional<std::string> JSONTransport::readStandardMessage() {
// A Language Server Protocol message starts with a set of HTTP headers,
@@ -244,8 +251,9 @@
std::string JSON(ContentLength, '\0');
for (size_t Pos = 0, Read; Pos < ContentLength; Pos += Read) {
// Handle EINTR which is sent when a debugger attaches on some platforms.
- Read = llvm::sys::RetryAfterSignal(0u, ::fread, &JSON[Pos], 1,
- ContentLength - Pos, In);
+ Read = retryAfterSignalUnlessShutdown(0, [&]{
+ return std::fread(&JSON[Pos], 1, ContentLength - Pos, In);
+ });
if (Read == 0) {
elog("Input was aborted. Read only {0} bytes of expected {1}.", Pos,
ContentLength);
@@ -263,7 +271,7 @@
// - messages are delimited by '---' on a line by itself
// - lines starting with # are ignored.
// This is a testing path, so favor simplicity over performance here.
-// When returning None, feof() or ferror() will be set.
+// When returning None, feof(), ferror(), or shutdownRequested() will be set.
llvm::Optional<std::string> JSONTransport::readDelimitedMessage() {
std::string JSON;
std::string Line;
@@ -280,6 +288,8 @@
JSON += Line;
}
+ if (shutdownRequested())
+ return llvm::None;
if (ferror(In)) {
elog("Input error while reading message!");
return llvm::None;
Index: clang-tools-extra/clangd/CMakeLists.txt
===================================================================
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -69,6 +69,7 @@
Selection.cpp
SemanticHighlighting.cpp
SemanticSelection.cpp
+ Shutdown.cpp
SourceCode.cpp
QueryDriverDatabase.cpp
Threading.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
