[clangd] Shutdown cleanly on signals.
authorSam McCall <sam.mccall@gmail.com>
Mon, 25 Nov 2019 18:51:07 +0000 (19:51 +0100)
committerSam McCall <sam.mccall@gmail.com>
Wed, 27 Nov 2019 11:38:49 +0000 (12:38 +0100)
Summary:
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)

Reviewers: kadircet, jfb

Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, jfb, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D70684

clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/JSONTransport.cpp
clang-tools-extra/clangd/Shutdown.cpp [new file with mode: 0644]
clang-tools-extra/clangd/Shutdown.h [new file with mode: 0644]
clang-tools-extra/clangd/test/exit-eof.test [new file with mode: 0644]
clang-tools-extra/clangd/test/exit-signal.test [new file with mode: 0644]
clang-tools-extra/clangd/tool/ClangdMain.cpp

index 8ab2ae6..c1aea3b 100644 (file)
@@ -69,6 +69,7 @@ add_clang_library(clangDaemon
   Selection.cpp
   SemanticHighlighting.cpp
   SemanticSelection.cpp
+  Shutdown.cpp
   SourceCode.cpp
   QueryDriverDatabase.cpp
   Threading.cpp
index 4921035..6351b80 100644 (file)
@@ -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 @@ public:
 
   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 @@ bool JSONTransport::handleMessage(llvm::json::Value Message,
 }
 
 // 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 @@ bool readLine(std::FILE *In, std::string &Out) {
   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 @@ bool readLine(std::FILE *In, std::string &Out) {
 }
 
 // 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 @@ llvm::Optional<std::string> JSONTransport::readStandardMessage() {
   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 @@ llvm::Optional<std::string> JSONTransport::readStandardMessage() {
 // - 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 @@ llvm::Optional<std::string> JSONTransport::readDelimitedMessage() {
     JSON += Line;
   }
 
+  if (shutdownRequested())
+    return llvm::None;
   if (ferror(In)) {
     elog("Input error while reading message!");
     return llvm::None;
diff --git a/clang-tools-extra/clangd/Shutdown.cpp b/clang-tools-extra/clangd/Shutdown.cpp
new file mode 100644 (file)
index 0000000..dfea46d
--- /dev/null
@@ -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
+
diff --git a/clang-tools-extra/clangd/Shutdown.h b/clang-tools-extra/clangd/Shutdown.h
new file mode 100644 (file)
index 0000000..3097f6a
--- /dev/null
@@ -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
diff --git a/clang-tools-extra/clangd/test/exit-eof.test b/clang-tools-extra/clangd/test/exit-eof.test
new file mode 100644 (file)
index 0000000..06d2ea8
--- /dev/null
@@ -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).
+
diff --git a/clang-tools-extra/clangd/test/exit-signal.test b/clang-tools-extra/clangd/test/exit-signal.test
new file mode 100644 (file)
index 0000000..15029b0
--- /dev/null
@@ -0,0 +1,32 @@
+# 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 -f %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 stop reading (before closing its input!)
+RUN:   while not grep "LSP finished" %t.err; do :; done; \
+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 608a2da..b8385a0 100644 (file)
@@ -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"
 #include <string>
 #include <thread>
 
+#ifndef _WIN32
+#include <unistd.h>
+#endif
+
 namespace clang {
 namespace clangd {
 namespace {
@@ -445,6 +450,7 @@ int main(int argc, char *argv[]) {
 
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+  llvm::sys::SetInterruptFunction(&requestShutdown);
   llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
     OS << clang::getClangToolFullVersion("clangd") << "\n";
   });
@@ -541,6 +547,10 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
   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))
@@ -694,12 +704,7 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
   // 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;
 }