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

Let subprocess launched by run handle its own sigint #246

Merged
merged 5 commits into from
Nov 17, 2024

Conversation

stefanv
Copy link
Member

@stefanv stefanv commented Nov 8, 2024

Otherwise, the subprocess is aborted by the parent process. This change makes spin run python -i ... usable.

Closes #235

/cc @adrinjalali

@adrinjalali
Copy link

This simply ignores the signal though. We'd want to delegate it to the subprocess.

With this PR, in numpy's repo, I get:

$ spin run python
$ /tmp/spin/bin/python3 vendored-meson/meson/meson.py compile -C build
$ /tmp/spin/bin/python3 vendored-meson/meson/meson.py install --only-changed -C build --destdir ../build-install
Python 3.12.7 (main, Oct  1 2024, 11:15:50) [GCC 14.2.1 20240910] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import time
>>> time.sleep(10)
^C^C^C>>> 

while with a simple python process you'd get:

$ python
Python 3.12.7 (main, Oct  1 2024, 11:15:50) [GCC 14.2.1 20240910] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import time
>>> time.sleep(10)
^CTraceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyboardInterrupt
>>> 

@stefanv
Copy link
Member Author

stefanv commented Nov 8, 2024

OK, will dig a bit further.

@stefanv stefanv added the type: Bug fix Something isn't working label Nov 9, 2024
@stefanv
Copy link
Member Author

stefanv commented Nov 9, 2024

Hopefully got it this time!

@adrinjalali
Copy link

Tests supposed to be failing?

@stefanv
Copy link
Member Author

stefanv commented Nov 12, 2024

Tests supposed to be failing?

No, it's on Windows only, will see why; Windows probably doesn't support preexec_fn.

@adrinjalali
Copy link

But I can confirm this fixes the issue nicely in linux at least.

@stefanv
Copy link
Member Author

stefanv commented Nov 17, 2024

Alternative approach to try:

try:
    process.wait()
except KeyboardInterrupt:
    print("Terminating subprocess...")
    parent = psutil.Process(process.pid)
    for child in parent.children(recursive=True):
        child.kill()
    process.wait()

@adrinjalali
Copy link

I think I like this as is now, we shouldn't decide for the process that sigint should kill it. If the process wants to do a dance on that signal, it's free to do so 😁

@stefanv
Copy link
Member Author

stefanv commented Nov 17, 2024

I just need to fire up my Windows test machine, then I can see if it works on that platform as well.

@stefanv stefanv force-pushed the avoid-capturing-sigint branch from 45d2cb6 to 5c0a79d Compare November 17, 2024 20:23
@stefanv
Copy link
Member Author

stefanv commented Nov 17, 2024

Tested on Windows, and seems like the behavior is as intended without any changes, so reverted (on that platform).

@stefanv stefanv merged commit 06dbe73 into scientific-python:main Nov 17, 2024
20 checks passed
@jarrodmillman jarrodmillman added this to the 0.13 milestone Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Bug fix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spin catches SIGINT and send SIGTERM to subprocess
3 participants