guiwf/lix
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

libutil: handle sigint only once per thread

Browse Source 
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
This commit is contained in:
eldritch horrors
2025-07-06 10:45:55 +00:00
parent b1544582af
commit 29d9a88af6
4 changed files with 75 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lix/libstore/daemon.cc
+1 -4
View File
@@ -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)) {
lix/libstore/filetransfer.cc
+31 -15
View File
@@ -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));
}
lix/libutil/signals.cc
+25 -4
View File
@@ -5,6 +5,7 @@
#include "lix/libutil/terminal.hh"
#include "lix/libutil/thread-name.hh"
#include "logging.hh"
#include <atomic>
#include <csignal>
#include <kj/time.h>
@@ -18,7 +19,10 @@
namespace nix {
std::atomic<bool> _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<bool()> 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<int> 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<int> 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<int> set)
void triggerInterrupt()
{
_isInterrupted = true;
_interruptSequence++;
auto callbacks = *_interruptCallbacks.lock();
if (callbacks) {
lix/libutil/signals.hh
+18 -2
View File
@@ -41,9 +41,14 @@ static inline constexpr int KJ_RESERVED_SIGNAL = SIGUSR2;
class Interrupted;
extern std::atomic<bool> _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<bool()> 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);