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

fix: sigterm handling so NGINX gracefully exits #1

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

sbdchd
Copy link

@sbdchd sbdchd commented Oct 7, 2020

After compiling nginx with the modifications:

make build-heroku-18

I then tested that the changes actually worked:

apt-get update
apt-get install -y python3-venv
python3 -m venv venv
./venv/bin/pip install gunicorn
FORCE=1 bin/start-nginx
./venv/bin/gunicorn -b unix:/tmp/nginx.socket app:app

# in another terminal (should hang for 15s assuming app is setup correctly)
curl localhost:5000

# in another terminal
# get the PID
ps aux | grep master

# should be graceful, that is nginx should shutdown after it finishes serving the request
kill -TERM $PID

FORCE=1 bin/start-nginx
# should kill nginx without waiting
# curl returns an error:
#   curl: (52) Empty reply from server
kill -QUIT $PID

The app used by gunicorn was the hello world with a sleep thrown in
so we mimic a long running request.

import time

def app(environ, start_response):
        time.sleep(15)
        data = b"Hello, World!\n"
        start_response("200 OK", [
            ("Content-Type", "text/plain"),
            ("Content-Length", str(len(data)))
        ])
        return iter([data])

Based on heroku#56

After compiling nginx with the modifications:

```shell
make build-heroku-18
```

I tested that the changes actually worked:

```shell
apt-get update
apt-get install -y python3-venv
python3 -m venv venv
./venv/bin/pip install gunicorn
FORCE=1 bin/start-nginx
./venv/bin/gunicorn -b unix:/tmp/nginx.socket app:app

# in another terminal (should hang for 15s assuming app is setup correctly)
curl localhost:5000

# in another terminal
# get the PID
ps aux | grep master

# should be graceful, that is nginx should shutdown after it finishes serving the request
kill -TERM $PID

FORCE=1 bin/start-nginx
# should kill nginx without waiting
# curl returns an error:
#   curl: (52) Empty reply from server
kill -QUIT $PID
```

The `app` used by `gunicorn` was the hello world with a sleep thrown in
so we mimic a long running request.

```python
import time

def app(environ, start_response):
        time.sleep(15)
        data = b"Hello, World!\n"
        start_response("200 OK", [
            ("Content-Type", "text/plain"),
            ("Content-Length", str(len(data)))
        ])
        return iter([data])
```

Based on heroku#56
@sbdchd sbdchd requested review from chdsbd and edanaher October 7, 2020 22:06
@sbdchd sbdchd changed the title fix: sigterm handling so NGINX gracefully exist fix: sigterm handling so NGINX gracefully exits Oct 7, 2020
@chdsbd
Copy link

chdsbd commented Oct 7, 2020

I think we can specify the buildpack to use this branch if we want to test on staging before merging.

@sbdchd
Copy link
Author

sbdchd commented Oct 8, 2020

I set this up on staging and it seems to run alright.

Also checked the SHA of the binaries to make sure it's actually using the new binary since it's hard to replicate the shutdown behavior.

Prod (old Buildpack):

~ $ sha1sum /app/bin/nginx
9ea1b169d767d7c72f741dc21ed934dc32f65d73  /app/bin/nginx

Staging:

/ $ sha1sum /app/bin/nginx
2f80245b376990e825d885eccbd223b1f8ee4425  /app/bin/nginx

Local:

root@e5049f172b89:/buildpack# sha1sum /buildpack/nginx/nginx
2f80245b376990e825d885eccbd223b1f8ee4425  /buildpack/nginx/nginx

@sbdchd sbdchd merged commit bd85c36 into master Oct 8, 2020
Copy link

@edanaher edanaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this before merge, but this LGTM aside from a couple unnecessary variables.

Also, if you wanted to test this, I suspect rapidly hitting staging (with either a benchmarking tool or a couple simultaneous curls (e.g., parallel -j4 curl marshall-staging/some-endpoint) while you restart marshall would suffice to reproduce this. Ideally, though, you'd test before the update to confirm that this does consistently produce 503's/H13's, and then see after that they're gone.

Comment on lines +47 to +48
NGX_SHUTDOWN_SIGNAL=TERM
NGX_TERMINATE_SIGNAL=QUIT

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure these two lines aren't doing anything, since they're just setting shell variables that are never used. I suspect a previous version of this script used them, but they're no longer necessary.

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.

3 participants