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

kiwix-serve should handle signals within docker #482

Closed
rgaudin opened this issue Sep 4, 2021 · 3 comments · Fixed by #488
Closed

kiwix-serve should handle signals within docker #482

rgaudin opened this issue Sep 4, 2021 · 3 comments · Fixed by #488

Comments

@rgaudin
Copy link
Member

rgaudin commented Sep 4, 2021

AFAIK, kiwix-serve has no signal management, leaving the microhttpd server started by libkiwix handling them.

When running on my computer, kiwix-serve properly exits on INT (^C), QUIT and TERM.

When running inside docker, none of those seem to have any impact. I always have to use KILL (which is handled by the system).

FYI, in docker, when using docker stop, it emmits a SIGTERM and waits for 10s until it uses SIGKILL. It is possible to send any signal using docker kill -s {signal}.

This behavior is annoying (you have to wait 10s on stop –or specify a timeout, you can't use restart, etc) and seen only with kiwix-serve. I don't know what's going on but maybe catching signals on kiwix-serve and calling Server::stop() would help ?

Note: I am using it with a full command and not a bash shortcut. PID is 1.

@mgautierfr
Copy link
Member

I would say that kiwix-serve should behave (if specified in command line) as a standard (and correctly implemented) daemon.
We already have a --daemon option but it is far from a correct daemonization of the process.

https://www.freedesktop.org/software/systemd/man/daemon.html explain pretty well what need to be implemented.
I would go to totally implement (at least) the new style daemon. Old style daemon is a bit more tricky to implement.

Implementing new style daemon implies to correctly handle SIGTERM to exit cleanly and SIGHUP to reload config (#243)

@kelson42
Copy link
Contributor

kelson42 commented Oct 3, 2021

@veloman-yunkan Would you be able to implement this ticket? Do we things which are unclear before implementation?

@veloman-yunkan
Copy link
Collaborator

@veloman-yunkan Would you be able to implement this ticket? Do we things which are unclear before implementation?

I will look at it this week.

rgaudin added a commit that referenced this issue Oct 13, 2021
Following-up on discussion in #488, now using https://github.com/Yelp/dumb-init
as entrypoint so our kiwix-* tools properly receives signals
rgaudin added a commit that referenced this issue Oct 13, 2021
Following-up on discussion in #488, now using https://github.com/Yelp/dumb-init
as entrypoint so our kiwix-* tools properly receives signals
rgaudin added a commit that referenced this issue Oct 13, 2021
Following-up on discussion in #488, now using https://github.com/Yelp/dumb-init
as entrypoint so our kiwix-* tools properly receives signals
@kelson42 kelson42 added this to the 3.2.0 milestone Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants