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 signal handling for SIGTERM in addition to SIGINT #1259

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dfangl
Copy link

@dfangl dfangl commented Jan 30, 2025

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

  • Add parameter of the signal value to the interruptPy method, to be able to pass the right signal to python
  • Install SIGTERM handler

Discussion

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.

@Thrameos
Copy link
Contributor

Looking at the PR I don't think a different flag is needed. But will review further later.

@Thrameos
Copy link
Contributor

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.

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.91%. Comparing base (fa54bca) to head (3a69715).

Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
native/common/jp_context.cpp 75.55% <ø> (+1.66%) ⬆️

... and 1 file with indirect coverage changes

@Thrameos
Copy link
Contributor

Looks like there is still a windows issue. Once that is complete I can review and merge.

@dfangl
Copy link
Author

dfangl commented Feb 3, 2025

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.

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.

2 participants