Handling of SIGTERM in kiwix-serve

[veloman-yunkan]

Fixes #482

[kelson42]

@rgaudin Would you be able to check please that the handling in Docker works now please?

[rgaudin]

@rgaudin Would you be able to check please that the handling in Docker works now please?

I'd like to but I am having difficulties buidling a static kiwix-serve. @veloman-yunkan can you share a static linux build that I could try?

[veloman-yunkan]

@rgaudin Would you be able to check please that the handling in Docker works now please?

I'd like to but I am having difficulties buidling a static kiwix-serve. @veloman-yunkan can you share a static linux build that I could try?

@rgaudin Here you are

[rgaudin]

@veloman-yunkan, thank you. kiwix-serve now properly shuts down on docker stop and docker restart works as expected.

Unfortunately, it doesn't react anymore to ^C so when using it with docker run -it, it can't be stopped in the same terminal.

FROM alpine

RUN mkdir -p /data && wget -O /data/archlinux_en_all_maxi_2021-05.zim https://download.kiwix.org/zim/other/archlinux_en_all_maxi_2021-05.zim

VOLUME /data

RUN wget https://github.com/kiwix/kiwix-tools/files/7324533/kiwix-serve.gz && \
    gunzip kiwix-serve.gz && \
    mv kiwix-serve /usr/local/bin/ && \
    chmod +x /usr/local/bin/kiwix-* && \
    rm -rf /kiwix-serve.gz

CMD ["/usr/local/bin/kiwix-serve", "/data/archlinux_en_all_maxi_2021-05.zim"]

docker build . -t kst && docker run -p 9999:80 -it kst

[mgautierfr]

LGTM.
But we must fix the last comment raised by @rgaudin (I don't know why it doesn't react anymore to SIGINT)

[veloman-yunkan]

Unfortunately, it doesn't react anymore to ^C so when using it with docker run -it, it can't be stopped in the same terminal.

@rgaudin The fix is easy - SIGINT must be handled in exactly the same way as SIGTERM.

For some reason I cannot upload the new build to github. Please let me know if you want to test it too.

[veloman-yunkan]

LGTM. But we must fix the last comment raised by @rgaudin (I don't know why it doesn't react anymore to SIGINT)

@mgautierfr Since the full change is small, I amended my fix and force pushed it without using a temporary fix-up commit.

[kelson42]

What about all the other standard singals which were working. I think in particular to SiGSTP which is really useful?

[veloman-yunkan]

This fix addresses handling of signals in kiwix-serve when it is executed with PID=1 (see https://petermalmgren.com/signal-handling-docker/). Otherwise, all other signals work as before. I don't think that suspending a process with PID=1 on SIGSTP makes any sense.

[mgautierfr]

For what I understand, the root cause of all this is that we are running the process with PID=1. So no default signal handler are used by the kernel. (@veloman-yunkan do you confirm ?)

Instead of re implement them wouldn't be better to use tools like https://github.com/Yelp/dumb-init or https://github.com/krallin/tini ?

[rgaudin]

Agrees with @mgautierfr. I've tested dumb-init quickly and it seems to work fine in all of my scenarios. Preparing a PR

[rgaudin]

See #489, for an alternative using dumb-init

[veloman-yunkan]

For what I understand, the root cause of all this is that we are running the process with PID=1. So no default signal handler are used by the kernel. (@veloman-yunkan do you confirm ?)

I confirm that.

Instead of re implement them wouldn't be better to use tools like https://github.com/Yelp/dumb-init or https://github.com/krallin/tini ?

For kiwix docker images that solution will work too. However it won't render this fix meaningless, since the latter provides a clean way of shutting down the server.

[mgautierfr]

Nice catch !

[mgautierfr]

For kiwix docker images that solution will work too. However it won't render this fix meaningless, since the latter provides a clean way of shutting down the server.

I agree.

And I wonder if we should not reset the signal handling to the default one after we handle it (or correctly handle a second signal). If something goes wrong during the server.stop() user would be able to kill the process with another Ctrl-C.
This is something I see few times on process taking times to clean up. A first ``Ctrl-C` is handled by the process with a message kind of
"Stopping the server... A new Ctrl-C will quit the process right now."

This is less important for SIGTERM as it is used by system before shutdown and a SIGKILL will be used just after if process doesn't quit.
We may also want to handle the signal raised before the server is started.
The easier is probably to init the wainting boolean to false, set it to true just before starting the server. And if wainting is false during the signal handling exit the process directly.

[veloman-yunkan]

For kiwix docker images that solution will work too. However it won't render this fix meaningless, since the latter provides a clean way of shutting down the server.

I agree.

And I wonder if we should not reset the signal handling to the default one after we handle it (or correctly handle a second signal). If something goes wrong during the server.stop() user would be able to kill the process with another Ctrl-C. This is something I see few times on process taking times to clean up. A first ``Ctrl-C` is handled by the process with a message kind of "Stopping the server... A new Ctrl-C will quit the process right now."

This is less important for SIGTERM as it is used by system before shutdown and a SIGKILL will be used just after if process doesn't quit. We may also want to handle the signal raised before the server is started. The easier is probably to init the wainting boolean to false, set it to true just before starting the server. And if wainting is false during the signal handling exit the process directly.

@mgautierfr In kiwix-serve SIGINT and SIGTERM should be handled identically. SIGINT has a different semantics for interactive applications, where it is intended to interrupt the current action invoked by the user (for example, in interactive python interpreter Ctrl-C interrupts the evaluation of the current expression entered in the REPL, but doesn't terminate the interpreter itself). In our case, the current action is the execution of the server in its entirety, that's why the effect of Ctrl-C should be indistinguishable from SIGTERM.

I implemented the handling of a second SIGINT or SIGTERM signal (or the first signal before the server loop is entered) without resorting to the default method. Then this fix keeps working for PID=1 processes too without relying on dumb-init or a similar workaround.

[mgautierfr]

Yes, we agree.
What I was saying is that we don't need to be prepare for a second SIGTERM as it should be followed by a SIGKILL (if something goes wrong when we quit our process). On the contrary, if something goes wrong when we quit after a first SIGINT, the user will send a second one.
But both (first) SIGTERM and SIGINT must be handled the same. And it is not a problem if we are handling the second SIGTERM the same way that SIGINT.