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.

Sign up for GitHub

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

Jump to bottom

python: Ignore SIGINT while running wrapped program from zip #20256

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+16 −5
Open

python: Ignore SIGINT while running wrapped program from zip #20256

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3613284
python: Ignore SIGINT while running wrapped program from zip
bubski Nov 11, 2023
2291951
Add comment about SIGINT forwarding
bubski Nov 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 16 additions & 5 deletions tools/python/python_bootstrap_template.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import sys
del sys.path[0]

import os
import signal
import subprocess

def IsRunningFromZip():
Expand Down Expand Up @@ -354,11 +355,21 @@ def ExecuteFile(python_program, main_filename, args, env, module_space,
ret_code = _RunForCoverage(python_program, main_filename, args, env,
coverage_entrypoint, workspace)
else:
ret_code = subprocess.call(
[python_program, main_filename] + args,
env=env,
cwd=workspace
)
# Temporarily ignoring SIGINT to not interfere with python_program's own
Copy link
Author

@bubski bubski Nov 17, 2023 •
edited

Choose a reason for hiding this comment

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

Assuming this is right, perhaps the same should be applied to internals of _RunForCoverage. I'm not sure yet.
Any feedback appreciated.

# interrupt handling. SIGINT does not need to be forwarded to python_program.
# It will receive the signal automatically as part of the same process group.
def noop(sig, frame):
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it necessary to forward the SIGINT to the wrapped process? Or does this happen automatically somehow?

Copy link
Author

@bubski bubski Nov 18, 2023 •
edited

Choose a reason for hiding this comment

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

My understanding is that SIGINT is automatically sent to all processes within a process group.
And since python_program is spawned with subprocess.call without start_new_session=True, it will get the same pgid.

Please correct me if I'm wrong, or missing a significant edge case.

This can be observed with this program:

Click to expand 

import os
import signal
import subprocess as sp
import sys
import time

pid = os.getpid()
pgid = os.getpgid(pid)

is_parent_process = len(sys.argv) == 1

print("parent" if is_parent_process else "child")
print(f"  {pid=}")
print(f"  {pgid=}")

def noop(sig, frame):
  pass

try:
  if is_parent_process:
    # signal.signal(signal.SIGINT, noop)
    sp.call(
      [sys.executable, __file__, "child"],
      # start_new_session=True,
    )
  else:
    print("press CTRL+C now")
    time.sleep(1000)
except:
  exc_type = sys.exc_info()[0]
  print("Exception caught", pid, "of type:", exc_type)

If you run it, you'll see two processes spawned:

parent
  pid=89304
  pgid=89304
child
  pid=89305
  pgid=89304
press CTRL+C now

and if you interrupt it, you will see that both of them receive KeyboardInterrupt:

…
press CTRL+C now
^CException caught 89305 of type: <class 'KeyboardInterrupt'>
Exception caught 89304 of type: <class 'KeyboardInterrupt'>

Now if you uncomment signal.signal(signal.SIGINT, noop) and repeat the steps, you will see that the parent sinks the interrupt into noop but the child still receives it as before:

parent
  pid=89584
  pgid=89584
child
  pid=89585
  pgid=89584
press CTRL+C now
^CException caught 89585 of type: <class 'KeyboardInterrupt'>

If you uncomment start_new_session=True,,
📝 and make sure to comment signal.signal(signal.SIGINT, noop) back out again,
then you'll see that the child is spawned with a different pgid this time, and no longer receives keyboard interrupts at all:

parent
  pid=89614
  pgid=89614
child
  pid=89615
  pgid=89615
press CTRL+C now
^CException caught 89614 of type: <class 'KeyboardInterrupt'>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, I didn't know that. Thanks for the detailed example! It could be worth adding a comment with a short explanation so that future readers of this part of the code don't see the same bug that isn't there.

signal.signal(signal.SIGINT, noop)

try:
ret_code = subprocess.call(
[python_program, main_filename] + args,
env=env,
cwd=workspace
)
finally:
signal.signal(signal.SIGINT, signal.SIG_DFL)

if delete_module_space:
# NOTE: dirname() is called because CreateModuleSpace() creates a
Expand Down