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

Allow to properly shutdown the PIP service #61

Closed
otbutz opened this issue Mar 12, 2018 · 6 comments
Closed

Allow to properly shutdown the PIP service #61

otbutz opened this issue Mar 12, 2018 · 6 comments

Comments

@otbutz
Copy link

otbutz commented Mar 12, 2018

I tested the following using the staging branch:

npm start &
PIP_PID=$!
sleep 10
kill -TERM $PIP_PID

The result is that the npm process exits but node and the other PIP processes(empire, country, etc) keep running. (see npm/npm#4603)

This also affects the docker image which is started with npm start aswell.

My current workaround for this behaviour is to invoke node directly:

node --max_old_space_size=4096 index.js &
PIP_PID=$!
sleep 10
kill -TERM $PIP_PID

A separate stop script might be a cleaner solution though.

orangejulius added a commit that referenced this issue Mar 12, 2018
This script uses `exec` to ensure the PID of the script is the PID of
the PIP service later.

The Dockerfile is updated to use the script, so that containers can be
shut down immediately by avoiding any intermediate processes that won't
forward signals.

See npm/npm#4603
Fixes #61
@orangejulius
Copy link
Member

Hi @otbutz,
We are all too familiar with npm/npm#4603, which is a pretty disappointing decision from NPM. I just opened #63, which should fix this. Can you test it out?

@otbutz
Copy link
Author

otbutz commented Mar 12, 2018

Had a look at your pull request. It should work for the Docker image as the node process will receive all incoming signals but it won't fix the npm start usecase.

That's why i'd suggest an approach like this one: https://stackoverflow.com/a/39128820

The stop endpoint should only work for local connections.

Edit: killing by process title should work too but requires additional commandline tools

@orangejulius
Copy link
Member

Ugh. That's clever, but a lot of extra complexity to add to all our services. We are not going to add that. I'd suggest you avoid npm start when backgrounding processes then. I now recall that we also did this in Mapzen Search production.

@otbutz
Copy link
Author

otbutz commented Mar 12, 2018

The ability to create a pid file would also solve the problem and should be pretty straightforward to implement.

https://www.npmjs.com/package/npid

@orangejulius
Copy link
Member

There are a lot of problesm with PID files. I've worked with them at scale for many years and a lot can go wrong.

As far as I know, the only really solid ways to run a service on a linux machine are to use a systemd service, which is out of scope for these Pelias repos, or to use a Docker container, which can always terminate the right processes when it shuts down.

We'll add start scripts to circumvent npm start in all our repos over time (feel free to submit PRs for ones you want right away). I'd suggest you use those for this case. We'll ensure the correct start commands are in those scripts going forward, so you should be able to count on them.

@orangejulius
Copy link
Member

I closed this issue because is merged #63, but if there are other solutions I'm missing, let me know :)

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

No branches or pull requests

2 participants