python: Ignore SIGINT while running wrapped program from zip

[bubski]

This is a proposal to resolve #20255

python_bootstrap_template has been updated to ignore SIGINT while the wrapped program is running, to not interfere with its potential custom handling.

[bubski]

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

[fmeum]

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

[bubski]

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'>

[fmeum]

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.

[bubski]

👋 @iancha1992 I'm not sure who to reach out to.
Could you help me find someone who could take a look at this?
Thank you!

[iancha1992]

cc: @fmeum @rickeylev

[bubski]

👋 @fmeum @rickeylev would either of you have the time to see if what I'm suggesting is reasonable?

[fmeum]

@aignas Could you take over here?

[aignas]

This looks like a reasonable change, could you create a PR against the bootstrap template that is in the rules_python please? The latest versions of rules_python use the copy that is there.

It would be also great if we could add a test to check the behaviour. If I understand correctly, we could create a special py_binary that could be invoked it py_test via subprocess.