9
\$\begingroup\$

When you search the web on how to gracefully shut down your Qt application when receiving SIGTERM, you are invariably directed towards this informative, yet needlessly complicated, page full of code snippets: Calling Qt Functions From Unix Signal Handlers. So I gave it a shot and wrote a class which simply emits a normal Qt signal unixSignal(int signalNumber) after you call installSignalHandler(int signalNumber) once for each signal you are interested in, without the need for duplicating code for each desired signal.

Now my question is: Did I miss anything important to safely tread around the dangerous territory of signal handlers?

main.cpp

#include <QApplication>
#include "unixsignalnotifier.h"

int main() {
    // Start of application initialization
    QApplication *application = new QApplication(argc, argv);

    // Make sure that the application terminates cleanly on various unix signals.
    QObject::connect(UnixSignalNotifier::instance(), SIGNAL(unixSignal(int)), application, SLOT(quit()));
    UnixSignalNotifier::instance()->installSignalHandler(SIGINT);
    UnixSignalNotifier::instance()->installSignalHandler(SIGTERM);

    return application->exec();
}

unixsignalhandler.h

#include <QObject>
#include <QSocketNotifier>
#include <signal.h>

class UnixSignalNotifier : public QObject
{
    Q_OBJECT
public:
    static UnixSignalNotifier *instance();
    bool installSignalHandler(int signalNumber);

signals:
    void unixSignal(int signalNumber);

private slots:
    void _socketHandler(int pipeFd);

private:
    explicit UnixSignalNotifier(QObject *parent = 0);
    ~UnixSignalNotifier();
    static void _signalHandler(int signalNumber);

    static int readPipes[_NSIG];
    static int writePipes[_NSIG];
    static QSocketNotifier *notifiers[_NSIG];
};

unixsignalhandler.cpp

#include "unixsignalnotifier.h"

#include <unistd.h>
#include <sys/socket.h>

int UnixSignalNotifier::readPipes[_NSIG] = {};
int UnixSignalNotifier::writePipes[_NSIG];
QSocketNotifier *UnixSignalNotifier::notifiers[_NSIG];


UnixSignalNotifier::UnixSignalNotifier(QObject *parent) :
    QObject(parent)
{
}

UnixSignalNotifier::~UnixSignalNotifier()
{
    for (int i = 0; i < _NSIG; i++) {
        if (notifiers[i] != NULL) {
            delete notifiers[i];
            notifiers[i] = NULL;
            close(readPipes[i]);
            close(writePipes[i]);
            readPipes[i] = writePipes[i] = 0;
        }
    }
}

UnixSignalNotifier *UnixSignalNotifier::instance()
{
    static UnixSignalNotifier *inst = new UnixSignalNotifier();
    return inst;
}

bool UnixSignalNotifier::installSignalHandler(int signalNumber)
{
    Q_ASSERT(1 <= signalNumber && signalNumber < _NSIG);
    Q_ASSERT(readPipes[signalNumber] == 0);
    Q_ASSERT(writePipes[signalNumber] == 0);
    Q_ASSERT(notifiers[signalNumber] == NULL);

    struct sigaction sigact;

    sigact.sa_handler = UnixSignalNotifier::_signalHandler;
    sigemptyset(&sigact.sa_mask);
    sigact.sa_flags = 0;
    sigact.sa_flags |= SA_RESTART;

    if (sigaction(signalNumber, &sigact, 0)) {
        qFatal("%s: Couldn't register signal handler", Q_FUNC_INFO);
    }

    int sockets[2];
    if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sockets)) {
        qFatal("%s: Couldn't create socketpair", Q_FUNC_INFO);
    }
    writePipes[signalNumber] = sockets[0];
    readPipes[signalNumber] = sockets[1];

    notifiers[signalNumber] = new QSocketNotifier(readPipes[signalNumber], QSocketNotifier::Read, 0);
    connect(notifiers[signalNumber], SIGNAL(activated(int)), this, SLOT(_socketHandler(int)));

    return true;
}

void UnixSignalNotifier::_socketHandler(int pipeFd)
{
    int signalNumber = -1;
    for (int i = 1; i < _NSIG; i++) {
        if (readPipes[i] == pipeFd) signalNumber = i;
    }
    if (signalNumber >= _NSIG) {
        qWarning("%s: Unable to find signal number for socket fd %d", Q_FUNC_INFO, pipeFd);
        return;
    }
    notifiers[signalNumber]->setEnabled(false);

    char dummy;
    ::read(readPipes[signalNumber], &dummy, sizeof(dummy));
    emit unixSignal(signalNumber);

    notifiers[signalNumber]->setEnabled(true);
}

void UnixSignalNotifier::_signalHandler(int signalNumber)
{
    if (writePipes[signalNumber] != 0) {
        char dummy = 1;
        ::write(writePipes[signalNumber], &dummy, sizeof(dummy));
    }
}
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

I did something similar a while ago. Instead of one singleton that handles all signals, I found it easier (for the caller) to have one object per signal:

#include <QObject>
#include <QPointer>

#include <array>

// A UnixSignalHandler catches a particular Unix signal (e.g. SIGTERM) and emits
// a Qt signal which can be connected to a slot.  Note that a process cannot
// catch SIGKILL - a handler for SIGKILL will never emit.
class UnixSignalHandler: public QObject
{
    Q_OBJECT

public:
    UnixSignalHandler(int signal, QObject *parent = nullptr);

    static const int max_signal = 32;

signals:
    // This gives no indication of which signal has been caught; you may achieve
    // that by connecting to a QSignalMapper if required.
    void raised() const;

private slots:
    void consumeInput(int fd) const;

private:
    int fd[2];
    static std::array<QPointer<UnixSignalHandler>, max_signal> handler;
    static void handle(int signal);
};

Then the implementation is:

#include "unixsignalhandler.h"

#include <QDebug>
#include <QSocketNotifier>

#include <sys/socket.h>
#include <signal.h>
#include <unistd.h>
UnixSignalHandler::UnixSignalHandler(int signal, QObject *parent)
    : QObject(parent)
{
    if (handler[signal] != nullptr) {
        qCritical()
            << "ignoring request to register duplicate handler for signal" 
            << signal;
        return;
    }
    if (::socketpair(AF_UNIX, SOCK_STREAM, 0, fd)) {
        qCritical() << "failed to create socket pair for"
            << signal << "-" << strerror(errno);
        return;
    }
    // There's not very much that a signal handler can legally do.  One thing
    // that is permitted is to write to an open file descriptor.  When our
    // handler is called, we'll write a single byte to a socket, and this socket
    // notifier will then learn of the signal outside of the signal handler
    // context.
    auto notifier = new QSocketNotifier(fd[1], QSocketNotifier::Read, this);
    connect(notifier, &QSocketNotifier::activated,
            this, &UnixSignalHandler::consumeInput);

    struct sigaction action;
    action.sa_handler = &UnixSignalHandler::handle;
    sigemptyset(&action.sa_mask);
    action.sa_flags = SA_RESTART;
    if (::sigaction(signal, &action, 0)) {
        qCritical() << "failed to add sigaction for"
            << signal << "-" << strerror(errno);
        return;
    }

    handler[signal] = this;
}
// This slot is connected to our socket notifier.  It reads the byte that the
// signal handler wrote (to reset the notifier) and emits a Qt signal.
void UnixSignalHandler::consumeInput(int fd) const
{
    char c;
    if (::read(fd, &c, sizeof c) <= 0)
        qWarning() << "Error reading fd" << fd
            << "(ignored) -" << strerror(errno);
    emit raised();
}
// This static method is the signal handler called when the process receives a
// Unix signal.  It writes a single byte to our open file descriptor.
void UnixSignalHandler::handle(int signal)
{
    if (signal < 0 || static_cast<size_t>(signal) >= handler.size()) {
        qWarning() << "ignored out-of-range signal" << signal;
        return;
    }

    auto const h = handler[signal];
    if (!h) {
        qWarning() << "ignored unhandled signal" << signal;
        return;
    }

    char c = 0;
    if (::write(h->fd[0], &c, sizeof c) <= 0)
        qWarning() << "Error writing signal" << signal
            << "(ignored) -" << strerror(errno);
}
std::array<QPointer<UnixSignalHandler>, UnixSignalHandler::max_signal>
    UnixSignalHandler::handler;

The cost to this approach is that it consumes a file-descriptor pair for each signal we want to handle, rather than multiplexing all signals over a single channel.

Usage is slightly simpler than yours, I think (but there's not much in it). When I construct the 'main' class of my application:

connect(new UnixSignalHandler(SIGTERM, this), &UnixSignalHandler::raised, qApp, &QCoreApplication::quit);
connect(new UnixSignalHandler(SIGINT, this), &UnixSignalHandler::raised, qApp, &QCoreApplication::quit);

I could probably just make qApp be the parent, and create/connect the handlers early in main().

Other differences

Apart from the one/many signals per handler, my code differs from yours in the following:

  • I missed the destructor, meaning an fd leak (not a problem, as my handlers live the length of the program), but if I implemented one, it would be shorter - just the two close calls (QPointer is automatically reset to nullptr when the object is deleted, so the handler array looks after itself).
  • I always avoid qFatal() and Q_ASSERT (in my applications, it's better to manage without the handler than to exit). I'm playing fast and loose with qWarning() inside the signal handler (but at that point, we're in trouble anyway, if we've lost the reader). I did find it helpful to show the error indication in the warning messages when errno has been set.
  • You've found a value _NSIG for the range of signals you support. I couldn't find it documented; is that just an artefact of your particular <signal.h>?
  • I also wrote a QTest unit test:

    void raiseSignal_USR1()
    {
        UnixSignalHandler handler(SIGUSR1);
    
        QSignalSpy spy(&handler, SIGNAL(raised()));
    
        QVERIFY(spy.isValid());
        QVERIFY(spy.isEmpty());
    
        ::raise(SIGUSR1);
        QVERIFY(spy.wait());
    }
    

I think both our implementations could be improved by taking the best features from the other.

Further observations

  • QSocketNotifier can be forward-declared in the header; it needs to be a complete type only in the implementation. My implementation doesn't need to bring <signal.h> into the header, but I think you need it for the _NSIG constant.
  • Re-reading, I see that you also use a file-per-signal (but not a QObject per signal). We can both save on fds by multiplexing the signal number as the data sent across the pipe.
\$\endgroup\$
8
  • \$\begingroup\$ Ah, using one object per signal is a clever idea, and obvious in hindsight. I really, really need to get those singletons patterns out of my head. However, we still need a static (and thus singleton) data structure to hold the individual objects, like your std::array Also, my implementation doesn't do multiplexing on the pipe, meaning it also consumes two FDs per signal, just like yours. But that's also a really good idea and pretty easy to change. :) _NSIG is probably an artifact and I shouldn't use it because of the underscore. However, your limit of 32 doesn't include RT signals. \$\endgroup\$
    – Fritz
    Sep 8, 2017 at 16:21
  • \$\begingroup\$ Agreed with everything. Taking the points in sequence: The array does act as a singleton, as you say. I was re-reading your code as you wrote the comment, and updated my answer when I saw you also use one pipe per signal. My needs were only for the traditional Unix signals, so I was content to stop there. UCHAR_MAX would be another choice, if we multiplex onto a single pipe (in theory, we can't guarantee a complete write of more than one char). \$\endgroup\$ Sep 8, 2017 at 16:30
  • \$\begingroup\$ Yep, using UCHAR_MAX was also my thought when considering multiplexing. That's probably enough signals for any UNIX system in existence. If you multiplex, there's also no need to do an array lookup inside the unix signal handler, like my implementation does, it's just a single write. So we could maybe save a few bytes of memory by using a QMap<int,UnixSignalHandler*> instead of UnixSignalHandler array[UCHAR_MAX]. I'm not sure how much RAM a map actually uses, though... \$\endgroup\$
    – Fritz
    Sep 8, 2017 at 17:15
  • \$\begingroup\$ I'll try to write a combined implementation during the weekend, including multiplexing and your other remarks, and post that as a separate answer. You're welcome to do so too and see if you're faster though. :) \$\endgroup\$
    – Fritz
    Sep 8, 2017 at 17:19
  • \$\begingroup\$ @Fritz, did you ever write the combined implementation? If so, consider posting as a new question. \$\endgroup\$ Dec 4, 2020 at 11:55
2
\$\begingroup\$

Both OP's code and the accepted answer fail to handle the cleanup. There should be a mutex protecting the destructor code from UNIX signals received while the destructor is running.

+1: Hm. Neither OP's code, nor the accepted answer handle the exception when the new operator fails.

\$\endgroup\$
7
  • 1
    \$\begingroup\$ I think it would be much simpler just to swap the order of operation to close the write sides of the pipes before deleting the notifier objects. A mutex shouldn't be required if that change were made. Good observation, though - I hadn't paid much attention to that particular race. \$\endgroup\$ May 4, 2022 at 16:21
  • \$\begingroup\$ The problem with close() is that it can be interrupted, fail, and return EINTR. So close() should be called in a loop until it succeeds. \$\endgroup\$
    – Zoltan K.
    May 5, 2022 at 14:32
  • \$\begingroup\$ However, I think this is a good idea, but instead of close(), use sigaction(SIG_IGN). AFAIK sigaction() is well versed in handling signals while it is setting handlers for said signals. \$\endgroup\$
    – Zoltan K.
    May 5, 2022 at 14:34
  • \$\begingroup\$ Instead of - or possibly as well as. Not sure whether SIG_IGN is the right choice when we destruct the handler; SIG_DFL has arguments for it, too, as does remembering the pre-construction value and restoring that. \$\endgroup\$ May 5, 2022 at 16:50
  • \$\begingroup\$ Since the context is to have a dedicated class to handle the signals for a process, I think it is safe to assume that the pre-constructor value is just the SIG_DFL. But the default action is to immediately terminate the process, which contradicts the role of the destructor. So putting SIG_DFL is the same as "this destructor is sometimes just an exit()". On fork(), the child process signal handlers are reset to SIG_DFL. This leaves us with SIG_IGN, which will guarantee that the destructor can finish its job. \$\endgroup\$
    – Zoltan K.
    May 5, 2022 at 17:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.