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

Add option to suppress SignalException on SIGTERM #1690

Merged
merged 7 commits into from
Mar 11, 2019

Conversation

mic-kul
Copy link
Contributor

@mic-kul mic-kul commented Dec 19, 2018

What:
This PR adds config option to disable raising SignalException when SIGTERM is received.

Why:
When running Puma inside Docker container that is orchestrated by Kubernetes/Docker swarm/ECS or other container platform SIGTERM is something expected, usually instructing app to shut down gracefully. Rolling restart is handled by infrastructure / orchestrator itself.
SignalException should not be raised in this case.

@mic-kul mic-kul changed the title Add option to suppress SignalException on SIGTERM [WIP] Add option to suppress SignalException on SIGTERM Dec 19, 2018
@mic-kul mic-kul closed this Dec 30, 2018
@mic-kul mic-kul reopened this Dec 30, 2018
@evanphx
Copy link
Member

evanphx commented Feb 20, 2019

Is this still a WIP?

@evanphx evanphx added the waiting-for-changes Waiting on changes from the requestor label Feb 20, 2019
@mic-kul mic-kul changed the title [WIP] Add option to suppress SignalException on SIGTERM Add option to suppress SignalException on SIGTERM Feb 21, 2019
@mic-kul
Copy link
Contributor Author

mic-kul commented Feb 21, 2019

Thanks for the reminder. Committed my recent changes and it's ready for another look @evanphx

@evanphx evanphx merged commit 22cc6a5 into puma:master Mar 11, 2019
@schneems
Copy link
Contributor

SignalException should not be raised in this case.

The exception is used by many libraries to clean up on SIGTERM. Firing that exception forces all the ensure blocks of the application to fire, for example telling libraries to safely close references to files etc. I don't have a problem with this being a configurable option, but I do worry about the unintended side effects of enabling this for long term system stability.

@mic-kul
Copy link
Contributor Author

mic-kul commented Mar 20, 2019

@schneems I agree it has side effects - ie. Kafka buffers might not be flushed properly.

I just couldn't understand why clustered mode raises it, while single mode does not.
In my current project we are overriding signal to SIGINT and it works without triggering on-error hooks for Airbrake/Rollbar libraries.

@schneems
Copy link
Contributor

I think in single mode the exception would be raised directly by the process since it’s what gets the sigterm versus the code in clustered is designed to propagate the sigterm. I think anyway.

shaunakpp pushed a commit to github-education-resources/classroom that referenced this pull request Aug 6, 2019
`Puma` raises a `SignalException` on receiving a `SIGTERM` to kill
the process puma was running on. While this is an expected behavior,
deployments, especially for dockerized applications, usually use
`SIGTERM` as a measure to kill processes. This results in our
exception tracker receiving a lot of `SignalException` errors on every
deploy. This small configuration change will suppress those errors.
For more information please check this link:
puma/puma#1690
shaunakpp pushed a commit to github-education-resources/classroom that referenced this pull request Aug 9, 2019
`Puma` raises a `SignalException` on receiving a `SIGTERM` to kill
the process puma was running on. While this is an expected behavior,
deployments, especially for dockerized applications, usually use
`SIGTERM` as a measure to kill processes. This results in our
exception tracker receiving a lot of `SignalException` errors on every
deploy. This small configuration change will suppress those errors.
For more information please check this link:
puma/puma#1690
floehopper added a commit to freerange/site that referenced this pull request Oct 3, 2019
This _should_ prevent the SIGTERM exceptions sent by Heroku to restart
dynos from being re-raised by Puma after it's handled them.

See puma/puma#1690 for more info.
johnnyshields added a commit to tablecheck/puma that referenced this pull request Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants