Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better signals interface #10401

Merged
merged 1 commit into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libcmd/repl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ ProcessLineResult NixRepl::processLine(std::string line)
if (line.empty())
return ProcessLineResult::PromptAgain;

_isInterrupted = false;
setInterrupted(false);

std::string command, arg;

Expand Down
4 changes: 2 additions & 2 deletions src/libfetchers/git-utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this<GitRepoImpl>
{
auto act = (Activity *) payload;
act->result(resFetchStatus, trim(std::string_view(str, len)));
return _isInterrupted ? -1 : 0;
return getInterrupted() ? -1 : 0;
}

static int transferProgressCallback(const git_indexer_progress * stats, void * payload)
Expand All @@ -361,7 +361,7 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this<GitRepoImpl>
stats->indexed_deltas,
stats->total_deltas,
stats->received_bytes / (1024.0 * 1024.0)));
return _isInterrupted ? -1 : 0;
return getInterrupted() ? -1 : 0;
}

void fetch(
Expand Down
2 changes: 1 addition & 1 deletion src/libmain/shared.cc
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ void initNix()

initLibStore();

startSignalHandlerThread();
unix::startSignalHandlerThread();

/* Reset SIGCHLD to its default. */
struct sigaction act;
Expand Down
3 changes: 2 additions & 1 deletion src/libstore/daemon.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "daemon.hh"
#include "monitor-fd.hh"
#include "signals.hh"
#include "worker-protocol.hh"
#include "worker-protocol-impl.hh"
#include "build-result.hh"
Expand Down Expand Up @@ -1038,7 +1039,7 @@ void processConnection(
unsigned int opCount = 0;

Finally finally([&]() {
_isInterrupted = false;
setInterrupted(false);
printMsgUsing(prevLogger, lvlDebug, "%d operations", opCount);
});

Expand Down
8 changes: 4 additions & 4 deletions src/libstore/filetransfer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -258,11 +258,11 @@ struct curlFileTransfer : public FileTransfer
int progressCallback(double dltotal, double dlnow)
{
try {
act.progress(dlnow, dltotal);
act.progress(dlnow, dltotal);
} catch (nix::Interrupted &) {
assert(_isInterrupted);
assert(getInterrupted());
}
return _isInterrupted;
return getInterrupted();
}

static int progressCallbackWrapper(void * userp, double dltotal, double dlnow, double ultotal, double ulnow)
Expand Down Expand Up @@ -466,7 +466,7 @@ struct curlFileTransfer : public FileTransfer
if (errorSink)
response = std::move(errorSink->s);
auto exc =
code == CURLE_ABORTED_BY_CALLBACK && _isInterrupted
code == CURLE_ABORTED_BY_CALLBACK && getInterrupted()
? FileTransferError(Interrupted, std::move(response), "%s of '%s' was interrupted", request.verb(), request.uri)
: httpStatus != 0
? FileTransferError(err,
Expand Down
2 changes: 1 addition & 1 deletion src/libutil/current-process.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void setStackSize(rlim_t stackSize)

void restoreProcessContext(bool restoreMounts)
{
restoreSignals();
unix::restoreSignals();
if (restoreMounts) {
#if __linux__
restoreMountNamespace();
Expand Down
70 changes: 70 additions & 0 deletions src/libutil/signals.hh
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#pragma once
///@file

#include "types.hh"
#include "error.hh"
#include "logging.hh"

#include <functional>

namespace nix {

/* User interruption. */

/**
* @note Does nothing on Windows
*/
static inline void setInterrupted(bool isInterrupted);

/**
* @note Does nothing on Windows
*/
static inline bool getInterrupted();

/**
* @note Does nothing on Windows
*/
static inline void setInterruptCheck(std::function<bool()> interruptCheck);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCC says

In file included from src/nix-collect-garbage/nix-collect-garbage.cc:2:
src/libutil/signals.hh:27:20: warning: ‘void nix::setInterruptCheck(std::function<bool()>)’ declared ‘static’ but never defined [-Wunused-function]
   27 | static inline void setInterruptCheck(std::function<bool()> interruptCheck);
      |                    ^~~~~~~~~~~~~~~~~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #10411.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sorry about that!


/**
* @note Does nothing on Windows
*/
void setInterruptThrown();

/**
* @note Does nothing on Windows
*/
inline void checkInterrupt();

/**
* @note Never will happen on Windows
*/
MakeError(Interrupted, BaseError);


struct InterruptCallback
{
virtual ~InterruptCallback() { };
};

/**
* Register a function that gets called on SIGINT (in a non-signal
* context).
*
* @note Does nothing on Windows
*/
std::unique_ptr<InterruptCallback> createInterruptCallback(
std::function<void()> callback);

/**
* A RAII class that causes the current thread to receive SIGUSR1 when
* the signal handler thread receives SIGINT. That is, this allows
* SIGINT to be multiplexed to multiple threads.
*
* @note Does nothing on Windows
*/
struct ReceiveInterrupts;

}

#include "signals-impl.hh"
2 changes: 1 addition & 1 deletion src/libutil/thread-pool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void ThreadPool::doWork(bool mainThread)
ReceiveInterrupts receiveInterrupts;

if (!mainThread)
interruptCheck = [&]() { return (bool) quit; };
unix::interruptCheck = [&]() { return (bool) quit; };

bool didWork = false;
std::exception_ptr exc;
Expand Down
2 changes: 1 addition & 1 deletion src/libutil/unix/monitor-fd.hh
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public:
*/
if (count == 0) continue;
if (fds[0].revents & POLLHUP) {
triggerInterrupt();
unix::triggerInterrupt();
break;
}
/* This will only happen on macOS. We sleep a bit to
Expand Down
61 changes: 34 additions & 27 deletions src/libutil/unix/signals.hh → src/libutil/unix/signals-impl.hh
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
#pragma once
///@file
/**
* @file
*
* Implementation of some inline definitions for Unix signals, and also
* some extra Unix-only interfaces.
*
* (The only reason everything about signals isn't Unix-only is some
* no-op definitions are provided on Windows to avoid excess CPP in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not present in this PR, that's coming later?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

* downstream code.)
*/

#include "types.hh"
#include "error.hh"
Expand All @@ -24,22 +33,20 @@ namespace nix {

/* User interruption. */

namespace unix {

extern std::atomic<bool> _isInterrupted;

extern thread_local std::function<bool()> interruptCheck;

void setInterruptThrown();

void _interrupted();

void inline checkInterrupt()
{
if (_isInterrupted || (interruptCheck && interruptCheck()))
_interrupted();
}

MakeError(Interrupted, BaseError);

/**
* Sets the signal mask. Like saveSignalMask() but for a signal set that doesn't
* necessarily match the current thread's mask.
* See saveSignalMask() to set the saved mask to the current mask.
*/
void setChildSignalMask(sigset_t *sigs);

/**
* Start a thread that handles various signals. Also block those signals
Expand All @@ -63,26 +70,26 @@ void saveSignalMask();
*/
void restoreSignals();

/**
* Sets the signal mask. Like saveSignalMask() but for a signal set that doesn't
* necessarily match the current thread's mask.
* See saveSignalMask() to set the saved mask to the current mask.
*/
void setChildSignalMask(sigset_t *sigs);
void triggerInterrupt();

struct InterruptCallback
}

static inline void setInterrupted(bool isInterrupted)
{
virtual ~InterruptCallback() { };
};
unix::_isInterrupted = isInterrupted;
}

/**
* Register a function that gets called on SIGINT (in a non-signal
* context).
*/
std::unique_ptr<InterruptCallback> createInterruptCallback(
std::function<void()> callback);
static inline bool getInterrupted()
{
return unix::_isInterrupted;
}

void triggerInterrupt();
void inline checkInterrupt()
{
using namespace unix;
if (_isInterrupted || (interruptCheck && interruptCheck()))
_interrupted();
}

/**
* A RAII class that causes the current thread to receive SIGUSR1 when
Expand Down
23 changes: 14 additions & 9 deletions src/libutil/unix/signals.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,22 @@

namespace nix {

std::atomic<bool> _isInterrupted = false;
using namespace unix;

std::atomic<bool> unix::_isInterrupted = false;

namespace unix {
static thread_local bool interruptThrown = false;
thread_local std::function<bool()> interruptCheck;
}

thread_local std::function<bool()> unix::interruptCheck;

void setInterruptThrown()
{
interruptThrown = true;
unix::interruptThrown = true;
}

void _interrupted()
void unix::_interrupted()
{
/* Block user interrupts while an exception is being handled.
Throwing an exception while another exception is being handled
Expand Down Expand Up @@ -65,7 +70,7 @@ static void signalHandlerThread(sigset_t set)
}
}

void triggerInterrupt()
void unix::triggerInterrupt()
{
_isInterrupted = true;

Expand Down Expand Up @@ -96,7 +101,7 @@ void triggerInterrupt()
static sigset_t savedSignalMask;
static bool savedSignalMaskIsSet = false;

void setChildSignalMask(sigset_t * sigs)
void unix::setChildSignalMask(sigset_t * sigs)
{
assert(sigs); // C style function, but think of sigs as a reference

Expand All @@ -115,14 +120,14 @@ void setChildSignalMask(sigset_t * sigs)
savedSignalMaskIsSet = true;
}

void saveSignalMask() {
void unix::saveSignalMask() {
if (sigprocmask(SIG_BLOCK, nullptr, &savedSignalMask))
throw SysError("querying signal mask");

savedSignalMaskIsSet = true;
}

void startSignalHandlerThread()
void unix::startSignalHandlerThread()
{
updateWindowSize();

Expand All @@ -141,7 +146,7 @@ void startSignalHandlerThread()
std::thread(signalHandlerThread, set).detach();
}

void restoreSignals()
void unix::restoreSignals()
{
// If startSignalHandlerThread wasn't called, that means we're not running
// in a proper libmain process, but a process that presumably manages its
Expand Down
Loading