Duplicate SIGINT sent to suprocess on Unix based systems

[biasedbit]

I was putting together a simple python script to manage multiple background processes (kinda like what's being discussed in #24). This script is invoked by poe (poe → proc_manager → p1, p2, ..., pN) and while working on signal interception & forwarding, I noticed hitting ctrl+c results in 2 SIGINTs being received by my proc_manager.

This SO post has a great detailed explanation of why this happens; the tl:dr is ctrl+c → end-of-text char to tty → tty sends SIGINT to all processes in foreground process group.

Since the poe task subprocess is not detached from poe's process group (e.g. by adding a preexec_fn=os.setpgrp or process_group=0 arg to Popen call here) and there's a signal handler to explicitly forward SIGINT to subprocs here, we end up with two SIGINTs being sent to the subprocs.

Only noticed this because in my use case I'm trying to handle the first SIGINT differently from subsequent ones (call subproc.terminate() first, then subproc.kill()).

Happy to send a PR; proposed change are either A:

def handle_sigint(signum, _frame):
-     # sigint is not handled on windows
-     signum = signal.CTRL_C_EVENT if self._is_windows else signum
-     proc.send_signal(signum)
+     # Forward SIGINT as CTRL_C_EVENT on Windows. On Unix, ctrl+c causes
+     # tty to send SIGINT to all the processes in the foreground process
+     # group; forwarding would lead to a double SIGINT.
+     if self._is_windows:
+         proc.send_signal(signal.CTRL_C_EVENT)

Or B:

- proc = Popen(cmd, **popen_kwargs)
+ proc = Popen(
+     cmd,
+     **popen_kwards,
+     # Change subprocess process group on Unix since tty sends SIGINT to
+     # all processes in foreground process group on ctrl+c. Since SIGINT
+     # is already explicitly forwarded below, this would result in
+     # duplicate SIGINT being received by subprocess.
+     preexec_fn=os.setpgrp if not self._is_windows else None,
+ )

A has the downside of not forwarding an explicit SIGINT being sent to poe (e.g. via kill -INT <poe_pid>).
B I don't think there's a downside on Unix? I've opted for B in my proc_manager script.

lmk what you think and I'll send the change request.

[biasedbit]

(Tested both changes locally and they work as expected.)

[biasedbit]

In case it's helpful, here's a link to the the proc_runner script I mention above.

[nat-n]

Hi @biasedbit, thanks for the interesting issue.

Option B does seem preferable, except that it does in practice break usage of poe with some CLI tools one could want to calls via a task, some contrived examples: sl, k9s, htop.

This makes me think there should be a task option to enable it, something like separate_process_group: bool, somewhat similar to use_exec option. Would you be up for submitting a PR with that? Otherwise Ill get around to it sooner or later.

Of course for consistency adding the preexec_fn keyword argument should be done by editing the popen_kwards dictionary above, and a new option would require a test and some documentation to release.

Incidentally setting the use_exec option to you task might be a valid workaround for you?

---

Also a comment on the example task definition from your script, normally including poetry run in the task cmd is redundant (and adds a couple of seconds delay), because poe will work with poetry's venv automatically. And when calling a python script from inside the repo I think it's preferable to use a script task, so long as the target behaviour is contained in a function that is importable from the root directory (may require the scripts dir to be a package).

[biasedbit]

I'll take a stab at it over the next couple of days. Cheers!

[biasedbit]

Hey @nat-n, didn't get a chance to work on this over the holidays but picking this up today — just checking in with you to make sure we don't both end up working on it at the same time.

[nat-n]

@biasedbit all yours :)

[biasedbit]

Looking a bit more carefully into this, turns out use_exec already does what I needed 😅 Having my program become the primary process avoids the dual SIGINT issue since poe's no longer handling and forwarding signals.

I'm going to go ahead and close this; thanks for jumping in so quick and talking it through — and for building this really incredible tool 🙌