From 29d9a88af6c8f2954e19744df745bd943ba50754 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 4 Jul 2025 21:12:12 +0200 Subject: [PATCH] libutil: handle sigint only once per thread many a cleanup path has been broken by interruptions being thrown every time checkInterrupt is called. we should only throw *once* though; more than one Interrupted exception for the same event is not only confusing but also breaks all cleanup paths at the first checkInterrupt call site (e.g. #900, the cgroup cleanup saga, temp dirs not being removed, etc). Change-Id: Ibfabf7f6af6ac2b78ad93582c254bbc48fcb3073 --- lix/libstore/daemon.cc | 5 +--- lix/libstore/filetransfer.cc | 46 ++++++++++++++++++++++++------------ lix/libutil/signals.cc | 29 +++++++++++++++++++---- lix/libutil/signals.hh | 20 ++++++++++++++-- 4 files changed, 75 insertions(+), 25 deletions(-) diff --git a/lix/libstore/daemon.cc b/lix/libstore/daemon.cc index f1db08e52..eb4491dae 100644 --- a/lix/libstore/daemon.cc +++ b/lix/libstore/daemon.cc @@ -853,10 +853,7 @@ void processConnection( unsigned int opCount = 0; - Finally finally([&]() { - _isInterrupted = false; - printMsgUsing(prevLogger, lvlDebug, "%d operations", opCount); - }); + Finally finally([&]() { printMsgUsing(prevLogger, lvlDebug, "%d operations", opCount); }); // FIXME: what is *supposed* to be in this even? if (readInt(from)) { diff --git a/lix/libstore/filetransfer.cc b/lix/libstore/filetransfer.cc index beb8b41fc..fdd7b12a0 100644 --- a/lix/libstore/filetransfer.cc +++ b/lix/libstore/filetransfer.cc @@ -366,11 +366,10 @@ struct curlFileTransfer : public FileTransfer int progressCallback(curl_off_t dltotal, curl_off_t dlnow) { try { - act.progress(dlnow, dltotal); + act.progress(dlnow, dltotal); } catch (nix::Interrupted &) { - assert(_isInterrupted); } - return _isInterrupted; + return isInterrupted(); } static int progressCallbackWrapper(void * userp, curl_off_t dltotal, curl_off_t dlnow, curl_off_t ultotal, curl_off_t ulnow) @@ -466,19 +465,36 @@ struct curlFileTransfer : public FileTransfer return curl_easy_strerror(code); } }; - auto exc = - code == CURLE_ABORTED_BY_CALLBACK && _isInterrupted - ? FileTransferError(Interrupted, std::move(response), "%s of '%s' was interrupted", verb(), uri) + auto exc = code == CURLE_ABORTED_BY_CALLBACK && isInterrupted() + ? FileTransferError( + Interrupted, + std::move(response), + "%s of '%s' was interrupted", + verb(), + uri + ) : httpStatus != 0 - ? FileTransferError(err, - std::move(response), - "unable to %s '%s': HTTP error %d (%s)%s", - verb(), uri, httpStatus, statusMsg, - code == CURLE_OK ? "" : fmt(" (curl error code=%d: %s)", code, textualError(errbuf, code))) - : FileTransferError(err, - std::move(response), - "unable to %s '%s': %s (curl error code=%d)", - verb(), uri, textualError(errbuf, code), code); + ? FileTransferError( + err, + std::move(response), + "unable to %s '%s': HTTP error %d (%s)%s", + verb(), + uri, + httpStatus, + statusMsg, + code == CURLE_OK + ? "" + : fmt(" (curl error code=%d: %s)", code, textualError(errbuf, code)) + ) + : FileTransferError( + err, + std::move(response), + "unable to %s '%s': %s (curl error code=%d)", + verb(), + uri, + textualError(errbuf, code), + code + ); fail(std::move(exc)); } diff --git a/lix/libutil/signals.cc b/lix/libutil/signals.cc index 993e52d7a..d77a0e392 100644 --- a/lix/libutil/signals.cc +++ b/lix/libutil/signals.cc @@ -5,6 +5,7 @@ #include "lix/libutil/terminal.hh" #include "lix/libutil/thread-name.hh" #include "logging.hh" +#include #include #include @@ -18,7 +19,10 @@ namespace nix { -std::atomic _isInterrupted = false; +std::atomic_unsigned_lock_free _interruptSequence{0}; +static std::atomic_unsigned_lock_free printMessageForSeq{0}, allowInterruptsAfter{0}; +thread_local std::atomic_unsigned_lock_free::value_type threadInterruptSeq{_interruptSequence.load() +}; thread_local std::function interruptCheck; @@ -27,8 +31,18 @@ Interrupted makeInterrupted() return Interrupted("interrupted by the user"); } +bool isInterrupted() +{ + const auto seq = _interruptSequence.load(std::memory_order::relaxed); + return seq > threadInterruptSeq && seq > allowInterruptsAfter.load(std::memory_order::relaxed); +} + void _interrupted() { + // don't throw for inhibited interrupt, ie those that were explicitly unset + if (_interruptSequence.load() <= allowInterruptsAfter.load()) { + return; + } /* Block user interrupts while an exception is being handled. Throwing an exception while another exception is being handled kills the program! */ @@ -39,7 +53,10 @@ void _interrupted() void unsetUserInterruptRequest() { - _isInterrupted = false; + // inhibit handling of pending interruptions in other threads + allowInterruptsAfter = _interruptSequence.load(); + // tell the signal handler thread to skip the please-try-again message + printMessageForSeq = 0; // recapture the signal as the signal handler thread will have released it AIO().unixEventPort.captureSignal(SIGINT); } @@ -96,7 +113,7 @@ static void signalHandlerThread(const std::vector set) // we only print to a terminal, and only by bypassing the logger, to // ensure that it's both a *user* who is sending us this signal, and // that the user will get a notification that isn't mixed with logs. - if (_isInterrupted && isatty(STDERR_FILENO)) { + if (_interruptSequence.load() == printMessageForSeq.load() && isatty(STDERR_FILENO)) { writeLogsToStderr( "Still shutting down. Press ^C again to abort all operations immediately.\n" ); @@ -112,6 +129,10 @@ static void signalHandlerThread(const std::vector set) pthread_sigmask(SIG_UNBLOCK, &unblock, nullptr); ::signal(SIGINT, SIG_DFL); printInterruptMessageAt = AIO().provider.getTimer().now() + 1 * kj::SECONDS; + // this is intentionally racy. triggerInterrupt increments the counter, if + // another interrupt is triggered in close proximity we do not want to see + // a message. this can happen if the repl or from the MonitorFdHup thread. + printMessageForSeq = _interruptSequence.load() + 1; triggerInterrupt(); } else if (signal == SIGTERM || signal == SIGHUP) { triggerInterrupt(); @@ -123,7 +144,7 @@ static void signalHandlerThread(const std::vector set) void triggerInterrupt() { - _isInterrupted = true; + _interruptSequence++; auto callbacks = *_interruptCallbacks.lock(); if (callbacks) { diff --git a/lix/libutil/signals.hh b/lix/libutil/signals.hh index 0778ee905..11e96bd24 100644 --- a/lix/libutil/signals.hh +++ b/lix/libutil/signals.hh @@ -41,9 +41,14 @@ static inline constexpr int KJ_RESERVED_SIGNAL = SIGUSR2; class Interrupted; -extern std::atomic _isInterrupted; +// global counter of how many interrupt requests of any type we've received. we +// count SIGINT, SIGTERM and SIGHUP equally here, but this mainly exists to let +// us keep track of which SIGINT events we have processed and which we haven't. +extern std::atomic_unsigned_lock_free _interruptSequence; extern thread_local std::function interruptCheck; +// the largest `_interruptSequence` the current thread has seen and acted upon. +extern thread_local std::atomic_unsigned_lock_free::value_type threadInterruptSeq; Interrupted makeInterrupted(); void _interrupted(); @@ -54,10 +59,21 @@ void _interrupted(); */ void unsetUserInterruptRequest(); +bool isInterrupted(); + +/** + * check whether an interrupt request is pending and throw Interrupted if so. a + * user hitting ^C is the main source of interrupts in interactive use, daemons + * are interrupted mainly by SIGHUP from clients disconnecting unexpectedly, or + * SIGTERM sent by the system service managers to tell the daemon to shut down. + */ void inline checkInterrupt() { - if (_isInterrupted || (interruptCheck && interruptCheck())) + const auto seq = _interruptSequence.load(std::memory_order::relaxed); + if (seq > threadInterruptSeq || (interruptCheck && interruptCheck())) { + threadInterruptSeq = seq; _interrupted(); + } } MakeError(Interrupted, BaseError);