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
- Merge pull request #650 from mohsenomidi/master close non-closed file descriptor will resolve : #38, #43, #150, #164, #221 - shutdown will work properly — committed to pistacheio/pistache by dennisjenkins75 4 years ago
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:
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.
@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 theEINTR
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 withsigaction
, and it would be much more elegant to keep a pointer or a list of pointers toEndpoint
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.