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

gh-131913: multiprocessing: add interrupt for POSIX #132453

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

pulkin
Copy link
Contributor

@pulkin pulkin commented Apr 12, 2025

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test where we register a signal.signal handler?

@@ -125,6 +125,13 @@ def start(self):
del self._target, self._args, self._kwargs
_children.add(self)

def interrupt(self):
'''
Terminate process; sends SIGINT signal or uses TerminateProcess()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it use TerminateProcess()?

@@ -0,0 +1 @@
add :func:`multiprocessing.Process.interrupt`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a better wording. In addition, a whatsnew/3.14.rst entry must be created as well.

Comment on lines 570 to 573
def test_interrupt(self):
if os.name != 'nt':
exitcode = self._kill_process(multiprocessing.Process.interrupt)
self.assertEqual(exitcode, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_interrupt(self):
if os.name != 'nt':
exitcode = self._kill_process(multiprocessing.Process.interrupt)
self.assertEqual(exitcode, 1)
@unittest.skipIf(os.name == 'nt', "POSIX only")
def test_interrupt(self):
exitcode = self._kill_process(multiprocessing.Process.interrupt)
self.assertEqual(exitcode, 1)

Can we exit with a SIGINT value instead? or will it always be 1?

Copy link
Contributor Author

@pulkin pulkin Apr 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can exit with SIGINT if we unset the signal handler in the child process

from signal import *
signal(SIGINT, SIG_DFL)

I expect this will make it behave similarly to SIGTERM or SIGKILL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still use self.assertEqual(exitcode, signal.SIGINT) by default (at least for the test suite).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a good idea for the test suite to actually check against exit code 1 which is the default behavior.

interrupt() triggers a default_int_handler in the child process which raises KeyboardInterrupt which, in turn, will probably be processed in this chunk setting exit code to 1

https://github.com/python/cpython/blob/main/Lib/multiprocessing/process.py#L323-L327

Achieving this behavior is a major motivation for the change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh so there is an explicit value being used here. Can you actually add a comment about this as well? so that we know that Python is explicitly setting the exit code to 1. TiA.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a unit test with SIGINT handler unset, a comment in the above test about exit code 1 and some hints in the docs about how it works with exceptions and exit codes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants