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 5 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 (7c39078).

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 3 files 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.

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