-
Notifications
You must be signed in to change notification settings - Fork 1
Catch SIGTERM on default settings. #24
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good addition! Before merging this:
- change the comment line to reflect new defaults
- add test case for default option sigterm usage
For the test case, you could modify the TestListener by adding inputSig syscal.Signal
field and then use that.
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
b0725f1
to
6057ee9
Compare
Thanks for the input. Now with the tests included and comment modified. |
6057ee9
to
0205cd5
Compare
This is useful with services like AWS Fargate where SIGTERM is sent when container is being stopped [1]. Can drastically speed up the shutdown and most importantly does it gracefully. Caveats: won't work on Windows. [1] https://aws.amazon.com/blogs/containers/graceful-shutdowns-with-ecs/ Signed-off-by: Ville Valkonen <[email protected]>
fd5bd2c
to
73fdc83
Compare
signals = []os.Signal{os.Interrupt} | ||
signals = []os.Signal{ | ||
os.Interrupt, | ||
syscall.SIGTERM, // XXX Not available on Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely forgot this windows aspect. I will test this on windows and come back with comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this will continue to work on Windows even there's no mapping for it.
Finally got windows machine to implement working win support #25 and now the code uses proper build constraints to achieve win/unix compatibility. If you rebase the pr you should be able to add sigterm only for unix platfors here and here . |
This is useful with services like AWS Fargate where SIGTERM is sent when container is being stopped [1]. Can drastically speed up the shutdown and most importantly does it gracefully.
Caveats: won't work on Windows.
[1] https://aws.amazon.com/blogs/containers/graceful-shutdowns-with-ecs/