-
-
Notifications
You must be signed in to change notification settings - Fork 550
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
Adds SIGTERM listener to gracefully shutdown the server #269
base: master
Are you sure you want to change the base?
Conversation
@dougwilson sorry I must have missed a notification adding the tags. When you say needs tests what kind of tests would you be looking for? Thanks |
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.
Change in 0db2765 seems to fix backward compatibility. Not very sure if @dougwilson want to implement extra test for SIGTERM case 🤔
Great idea @HarryEMartland
maybe @dougwilson can re-run the tests for |
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.
Yes, we need to add tests to test suite when new feature added or bug fixed.
If someone could give me some pointers on how to test this I would greatly appreciate it. The issue is only really noticeable if a request takes a decent amount of time. In sudo code I would want to test something like this;
I have had a play with the existing tests and can see that there is code already sending SIGTERM. I am however unsure of how to add a new endpoint with a delay in it. |
Kubernetes and other process managers send a SIGTERM to applications to give them time to shut down. This change adds a listener to this event which shuts the server down gracefully.