-
Notifications
You must be signed in to change notification settings - Fork 184
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 signal handling for SIGTERM in addition to SIGINT #1259
base: master
Are you sure you want to change the base?
Conversation
Looking at the PR I don't think a different flag is needed. But will review further later. |
PyErr_SetInterruptEx did not appear until 3.10. You will need to add version controls around the statement to get it to pass all checks. Also a unit test would be a good idea, or the code may get broken in some uncontrolled way in the future. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1259 +/- ##
==========================================
+ Coverage 86.87% 86.91% +0.04%
==========================================
Files 113 113
Lines 10336 10336
Branches 4065 4065
==========================================
+ Hits 8979 8984 +5
+ Misses 761 757 -4
+ Partials 596 595 -1
|
Looks like there is still a windows issue. Once that is complete I can review and merge. |
I skipped the tests on windows for now. SIGTERM is not a thing on Windows, and it seems like the SIGINT handling does not work either, the process just seems to crash. "Signal" handling on Windows does not make a lot of sense anyway, but it might make sense to fix the CTRL-C behavior in windows at some point (assuming it actually does not work as the tests suggest, and not just the test setup is wrong). I don't have a Windows setup right now, but should skipping not be acceptable, I will set one up. |
Motivation
This PR addresses #1258 by adding a SIGTERM handler in addition to a SIGINT handler to pass the control back to the python process, should the signal be cought.
Changes
interruptPy
method, to be able to pass the right signal to pythonDiscussion
We might want to have the sigterm handling behind a separate flag, as being discussed in #1258. Happy to do this, should it be preferred.