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

Handle properly SINGINT and SIGTERM signals #43

Merged
merged 1 commit into from
Feb 5, 2017

Conversation

nhuray
Copy link
Contributor

@nhuray nhuray commented Feb 2, 2017

Hi @mhart,

This is a pull request to manage properly the SINGINT and SIGTERM signals when Kinesalite is bundle in a Docker container.

Context

The current behavior is due to a change introduced by c61b0e9 in Node.

With this commit, when a user sends SIGINT by pressing CTRL-C in a terminal where node is the foreground process, the node process doesn't exit explicitly and instead forward the signal.

Please read this issue for more information: nodejs/node-v0.x-archive#9131

Impact

When Kinesalite run in a Docker container (as PID 1) and we send a SIGINT or SIGTERM signal with CTRL+C the container does not exit properly:

CONTAINER ID        IMAGE                        STATUS               NAMES
51b1f016686b        saikocat/kinesalite:1.11.5   Exited (137)   kinesalite

@mhart
Copy link
Owner

mhart commented Feb 5, 2017

This is really an issue with Docker – it runs processes with PID 1 and provides no way to have normal (non-PID-1) SIGINT behaviour. Node.js acts correctly here – other containers like mysql and redis suffer from the same problem.

However, that said, I'm happy to add this with some checks for process.pid – I'll merge this in a sec

@mhart mhart merged commit b6e41b5 into mhart:master Feb 5, 2017
@mhart
Copy link
Owner

mhart commented Feb 5, 2017

Merged and released as 1.11.6 – thanks for the contribution!

I limited it to just SIGINT and only if the pid is 1, as it is in Docker – all other behaviour should act as normal

Let me know if you have any further issues

@nhuray
Copy link
Contributor Author

nhuray commented Feb 6, 2017

It's just perfect:

~/D/P/B/W/docker-kinesalite ❯❯❯ docker run -ti  bandsintown/kinesalite                                                                                                                                                ⏎ 1.11.6 ✚ ✱
Kinesalite started on port 4567
^C
caught SIGINT, exiting
~/D/P/B/W/docker-kinesalite ❯❯❯ docker ps                                                                                                                                                                               
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
~/D/P/B/W/docker-kinesalite ❯❯❯ docker ps -a                                                                                                                                                                            
CONTAINER ID        IMAGE                      COMMAND                  CREATED             STATUS                      PORTS               NAMES
880eaaea34b0        bandsintown/kinesalite     "node kinesalite.j..."   21 seconds ago      Exited (0) 17 seconds ago                       epic_visvesvaraya

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants