pistache: InstallSignalHandler does not work as expected

Pistache::Address addr(Pistache::Ipv4::any(), Pistache::Port(options.get_port()));
auto opts = Pistache::Http::Endpoint::options().threads(2).flags(Pistache::Tcp::Options::InstallSignalHandler);;

Pistache::Http::Endpoint server(addr);
server.init(opts);
server.setHandler(Pistache::Http::make_handler<HttpHandler>());

log("Serving the API on %s:%d.", addr.host().c_str(), static_cast<uint16_t>(addr.port()) );
server.serve();
server.shutdown();

Sending CTRL+C to the terminal does nothing although I would expect server.serve() to return and process to properly shut down. What purpose does the InstallSignalHandler serve? Also, isn’t it a bad design choice that a multi-threaded lib attempts to handle signals? Signals in a multi-threaded process are already tricky enough, so having a lib to have its own signal handler only adds to the complexity with questionable benefits.

About this issue

  • Original URL
  • State: open
  • Created 6 years ago
  • Reactions: 3
  • Comments: 17 (13 by maintainers)

Commits related to this issue

Most upvoted comments

@kiplingw Yes, You are right, but this library provides http server and server should be daemonized. But of course, everybody can daemonize themselves if it want. Pistache provides .flags(Pistache::Tcp::Options::InstallSignalHandler). This suggest that this can be daemonized but there is no docs about it 😕

I very much disagree that pistache “should be daemonized”. Pistache is a library that does one thing (well): Respond to HTTP requests. Pistache should not attempt to be all things to all users.

There are many use-cases where one would use pistache to create an HTTP control plane into existing software, that may or may not already run as a daemon. If pistache forced daemonization, this would break these and other use-cases.

If you want your own software to daemonize, then implement that in your software. Turning a process into a proper daemon is ~10 lines of code.

imho, we should remove the signal handler from pistache, as that seems to be a proper function of the app, not a library. The only benefit to having a signal handler in pistache is that one can call “Endpoint::serve()” and that should exit on SIGINT (but that is broken) without creating any extra threads. However, one could install their own signal handler and call Endpoint::shutdown() (which now works) from that handler.

Anyway, this is how I solved it:

First before any threads are created I have this code:

    if (sigemptyset(&signals)        != 0
    ||  sigaddset(&signals, SIGTERM) != 0
    ||  sigaddset(&signals, SIGINT)  != 0
    ||  sigaddset(&signals, SIGQUIT) != 0
    ||  sigaddset(&signals, SIGPIPE) != 0
    ||  sigaddset(&signals, SIGALRM) != 0
    ||  pthread_sigmask(SIG_BLOCK, &signals, nullptr) != 0) return false;

It should ensure that no thread will be able to get any signals. Since Pistache is running in threaded mode I can just keep polling for any signals from the main thread.

Pistache::Address addr(Pistache::Ipv4::any(), Pistache::Port(options.get_port()));
auto opts = Pistache::Http::Endpoint::options().threads(2);
Pistache::Http::Endpoint server(addr);
server.init(opts);
server.setHandler(Pistache::Http::make_handler<HttpHandler>());

log("Serving the API on %s:%d.", addr.host().c_str(), static_cast<uint16_t>(addr.port()) );
server.serveThreaded();

bool terminate = false;
while (!terminate) {
    int number = 0;
    int status = sigwait(&signals, &number);
    if (status != 0) {
        log("sigwait() returned %d, shutting down.", status);
        break;
    }

    switch (number) {
        case SIGINT : log("\rCaught signal %d (SIGINT).",  number); terminate = true; break;
        case SIGTERM: log("\rCaught signal %d (SIGTERM).", number); terminate = true; break;
        case SIGQUIT: log("\rCaught signal %d (SIGQUIT).", number); terminate = true; break;
        case SIGPIPE: log("\rCaught signal %d (SIGPIPE).", number);                   break;
        case SIGALRM: log("\rCaught signal %d (SIGALRM).", number);                   break;
        default     : log("\rCaught signal %d.",           number);                   break;
    }
}

log("Shutting down the HTTP server.");
server.shutdown();

@nicraMarcin, yes indeed! Time to leave the four corner prison of a web browser behind!

I stayed away from this discussion until @dennisjenkins75 wrote his comment. I couldn’t agree more.

Ideally, I would see pistache throw away even networking and threading so it would operate solely on stdin and stdout. But if you’re not doing that then at least get rid of signals. The host program would implement threading, networking and signals the way it sees fit.

I would love to be able to set up a HTTP server written in C++, using pistache and a wonderful linux tool named tcpserver which will handle the networking for me.

I have not fixed the signal handler. If someone puts forth a working PR, I’ll gladly review and merge it.

Work-around: I recently changed my own app from using “Endpoint::serve()” to “Endpoint::serveThreaded()”. I instlll my own signal handlers in my main thread, and when I get SIGINT, SIGTERM, SIGHUP, the main thread calls “Endpoint::shutdown()”. pistache shuts down just fine, allowing my main thread to do cleanup work.

The issue is produced in the epoll_wait loop, that does not check whether the EINTR result is produced by a termination signal from the user, and just repeats the call. I’ve created a patch that removes this loop (there is another similar loop in the caller function) and adds the option of choosing which signal(s) you want the endpoint to react with a shutdown (usually daemons react to SIGTERM and SIGHUP too). It still needs some work because it does not work if the Tcp::Options object goes out of scope when the endpoint is initialized, but I thought it may be useful to share.

On the other hand, I agree with @kiplingw that signal should be replaced with sigaction, and it would be much more elegant to keep a pointer or a list of pointers to Endpoint instead of a file descriptor, which I think it would allow us to easily support multiple simultaneous endpoints in a single program.

0001-Better-signal-handling.patch.txt

I’d also like to point out that signal(2) is deprecated and we should be using sigaction(2) instead - assuming we want Pistache to trap any signals at all.