From ef749f27d682883abf9b27b888419a048024073c Mon Sep 17 00:00:00 2001 From: Andrew Olsen Date: Wed, 14 Jun 2023 17:02:42 +1200 Subject: [PATCH 1/7] Fix sigint handling when running in helper mode --- cli_helper/kart.c | 22 +++++++++++++++++++--- kart/__init__.py | 5 ++++- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/cli_helper/kart.c b/cli_helper/kart.c index 8da656d19..777904da9 100644 --- a/cli_helper/kart.c +++ b/cli_helper/kart.c @@ -194,7 +194,7 @@ int is_helper_enabled() /** * @brief Exit signal handler for SIGALRM */ -void exit_on_alarm(int sig) +void exit_on_sigalrm(int sig) { int semval = semctl(semid, SEMNUM, GETVAL); if (semval < 0) @@ -209,6 +209,21 @@ void exit_on_alarm(int sig) exit(exit_code); } +char *socket_filename; + +/** + * @brief Exit signal handler for SIGINT + */ +void exit_on_sigint(int sig) +{ + putchar('\n'); + const char* kill_cmd_template = "lsof -t '%s' | xargs kill"; + char* kill_cmd = malloc(strlen(kill_cmd_template) + strlen(socket_filename) + 2); + sprintf(kill_cmd, kill_cmd_template, socket_filename); + system(kill_cmd); + exit(128 + sig); +} + int main(int argc, char **argv, char **environ) { char cmd_path[PATH_MAX]; @@ -267,7 +282,7 @@ int main(int argc, char **argv, char **environ) int fp = open(getcwd(NULL, 0), O_RDONLY); int fds[4] = {fileno(stdin), fileno(stdout), fileno(stderr), fp}; - char *socket_filename = malloc(strlen(getenv("HOME")) + strlen(".kart.socket") + 2); + socket_filename = malloc(strlen(getenv("HOME")) + strlen(".kart.socket") + 2); sprintf(socket_filename, "%s/%s", getenv("HOME"), ".kart.socket"); int socket_fd = socket(AF_UNIX, SOCK_STREAM, 0); @@ -370,7 +385,8 @@ int main(int argc, char **argv, char **environ) memcpy((int *)CMSG_DATA(cmsg), fds, sizeof(fds)); msg.msg_controllen = cmsg->cmsg_len; - signal(SIGALRM, exit_on_alarm); + signal(SIGALRM, exit_on_sigalrm); + signal(SIGINT, exit_on_sigint); if (sendmsg(socket_fd, &msg, 0) < 0) { diff --git a/kart/__init__.py b/kart/__init__.py index 5c3b17943..650ce4339 100644 --- a/kart/__init__.py +++ b/kart/__init__.py @@ -199,7 +199,10 @@ def _cleanup_process_group(signum, stack_frame): if _kart_process_group_killed: return _kart_process_group_killed = True - os.killpg(os.getpid(), signum) + try: + os.killpg(os.getpid(), signum) + except Exception: + pass sys.exit(128 + signum) signal.signal(signal.SIGTERM, _cleanup_process_group) From f4f92d3b4894fb8f31b198d0b6f5db27d4b28c70 Mon Sep 17 00:00:00 2001 From: Andrew Olsen Date: Fri, 16 Jun 2023 14:11:34 +1200 Subject: [PATCH 2/7] Fix SIGINT handling, don't use lsof --- cli_helper/kart.c | 16 ++++++++-------- kart/__init__.py | 2 +- kart/helper.py | 11 +++++++++++ 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/cli_helper/kart.c b/cli_helper/kart.c index 777904da9..c281e2fb2 100644 --- a/cli_helper/kart.c +++ b/cli_helper/kart.c @@ -209,18 +209,14 @@ void exit_on_sigalrm(int sig) exit(exit_code); } -char *socket_filename; - /** - * @brief Exit signal handler for SIGINT + * @brief Exit signal handler for SIGINT. + * Tries to kill the whole process group. */ void exit_on_sigint(int sig) { putchar('\n'); - const char* kill_cmd_template = "lsof -t '%s' | xargs kill"; - char* kill_cmd = malloc(strlen(kill_cmd_template) + strlen(socket_filename) + 2); - sprintf(kill_cmd, kill_cmd_template, socket_filename); - system(kill_cmd); + killpg(0, sig); exit(128 + sig); } @@ -236,6 +232,10 @@ int main(int argc, char **argv, char **environ) { debug("enabled %s, pid=%d\n", cmd_path, getpid()); + // Make this process the leader of a process group: + // The procress-group ID (pgid) will be the same as the pid. + setpgrp(); + // start or use an existing helper process char **env_ptr; @@ -282,7 +282,7 @@ int main(int argc, char **argv, char **environ) int fp = open(getcwd(NULL, 0), O_RDONLY); int fds[4] = {fileno(stdin), fileno(stdout), fileno(stderr), fp}; - socket_filename = malloc(strlen(getenv("HOME")) + strlen(".kart.socket") + 2); + char *socket_filename = malloc(strlen(getenv("HOME")) + strlen(".kart.socket") + 2); sprintf(socket_filename, "%s/%s", getenv("HOME"), ".kart.socket"); int socket_fd = socket(AF_UNIX, SOCK_STREAM, 0); diff --git a/kart/__init__.py b/kart/__init__.py index 650ce4339..2d72be54a 100644 --- a/kart/__init__.py +++ b/kart/__init__.py @@ -200,7 +200,7 @@ def _cleanup_process_group(signum, stack_frame): return _kart_process_group_killed = True try: - os.killpg(os.getpid(), signum) + os.killpg(0, signum) except Exception: pass sys.exit(128 + signum) diff --git a/kart/helper.py b/kart/helper.py index 6bce9856c..2f6601836 100644 --- a/kart/helper.py +++ b/kart/helper.py @@ -133,6 +133,7 @@ def helper(ctx, socket_filename, timeout, args): else: # child _helper_log("post-fork") + payload, fds = recv_json_and_fds(client, maxfds=4) if not payload or len(fds) != 4: click.echo( @@ -169,6 +170,16 @@ def helper(ctx, socket_filename, timeout, args): f"Payload:\n{repr(payload)}", ) else: + try: + # Join the process group of the calling process - so that if they get killed, we get killed to. + os.setpgid(0, calling_environment["pid"]) + os.environ["_KART_PGID_SET"] = "1" + except Exception as e: + # Kart will still work even if this fails: it just means SIGINT Ctrl+C might not work properly. + # We'll just log it and hope for the best. + _helper_log(f"error joining caller's process group: {e}") + pass + sys.argv[1:] = calling_environment["argv"][1:] _helper_log(f"cmd={' '.join(calling_environment['argv'])}") os.environ.clear() From c34f94adbff22334203a7c578b20d1d8713319ac Mon Sep 17 00:00:00 2001 From: Andrew Olsen Date: Mon, 19 Jun 2023 20:35:27 +1200 Subject: [PATCH 3/7] One helper per session --- cli_helper/kart.c | 6 ++---- kart/__init__.py | 4 ++-- kart/cli.py | 2 +- kart/helper.py | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/cli_helper/kart.c b/cli_helper/kart.c index c281e2fb2..c9da3cab8 100644 --- a/cli_helper/kart.c +++ b/cli_helper/kart.c @@ -282,8 +282,8 @@ int main(int argc, char **argv, char **environ) int fp = open(getcwd(NULL, 0), O_RDONLY); int fds[4] = {fileno(stdin), fileno(stdout), fileno(stderr), fp}; - char *socket_filename = malloc(strlen(getenv("HOME")) + strlen(".kart.socket") + 2); - sprintf(socket_filename, "%s/%s", getenv("HOME"), ".kart.socket"); + char *socket_filename = malloc(strlen(getenv("HOME")) + strlen(".kart.1234567890.socket") + 2); + sprintf(socket_filename, "%s/.kart.%d.socket", getenv("HOME"), getsid(0)); int socket_fd = socket(AF_UNIX, SOCK_STREAM, 0); struct sockaddr_un addr; @@ -304,8 +304,6 @@ int main(int argc, char **argv, char **environ) // process are left open in it if (fork() == 0) { - setsid(); - // start helper in background and wait char *helper_argv[] = {&cmd_path[0], "helper", "--socket", socket_filename, NULL}; diff --git a/kart/__init__.py b/kart/__init__.py index 2d72be54a..7704c7649 100644 --- a/kart/__init__.py +++ b/kart/__init__.py @@ -182,11 +182,11 @@ def _configure_process_cleanup_nonwindows(): # to attempt to give Kart it's own process group ID (PGID) if "_KART_PGID_SET" not in os.environ and os.getpid() != os.getpgrp(): try: - os.setsid() + os.setpgrp() # No need to do this again for any Kart subprocess of this Kart process. os.environ["_KART_PGID_SET"] = "1" except OSError as e: - L.warning("Error setting Kart PGID - os.setsid() failed. %s", e) + L.warning("Error setting Kart PGID - os.setpgrp() failed. %s", e) # If Kart now has its own PGID, which its children share - we want to SIGTERM that when Kart exits. if os.getpid() == os.getpgrp(): diff --git a/kart/cli.py b/kart/cli.py index ab1b71728..66de46749 100755 --- a/kart/cli.py +++ b/kart/cli.py @@ -149,7 +149,7 @@ def print_version(ctx): # report on whether this was run through helper mode helper_pid = os.environ.get("KART_HELPER_PID") if helper_pid: - click.echo(f"Executed via helper, PID: {helper_pid}") + click.echo(f"Executed via helper, SID={os.getsid(0)} PID={helper_pid}") ctx.exit() diff --git a/kart/helper.py b/kart/helper.py index 2f6601836..29ef5c3bd 100644 --- a/kart/helper.py +++ b/kart/helper.py @@ -28,7 +28,7 @@ def _helper_log(msg): @click.option( "--socket", "socket_filename", - default=Path.home() / ".kart.socket", + default=Path.home() / f".kart.{os.getsid(0)}.socket", show_default=True, help="What socket to use", ) From 19775952d11a124794a9cbfe7691df41cf8d26a4 Mon Sep 17 00:00:00 2001 From: Andrew Olsen Date: Tue, 20 Jun 2023 10:53:17 +1200 Subject: [PATCH 4/7] SIGINT handling: add test, fix windows --- kart/helper.py | 8 ++++- tests/test_cli.py | 90 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 3 deletions(-) diff --git a/kart/helper.py b/kart/helper.py index 29ef5c3bd..2a7bea441 100644 --- a/kart/helper.py +++ b/kart/helper.py @@ -23,12 +23,18 @@ def _helper_log(msg): log_file.write(f"{datetime.now()} [{os.getpid()}]: {msg}\n") +def getsid(): + if hasattr(os, "getsid"): + return os.getsid(0) + return 0 + + @click.command(context_settings=dict(ignore_unknown_options=True)) @click.pass_context @click.option( "--socket", "socket_filename", - default=Path.home() / f".kart.{os.getsid(0)}.socket", + default=Path.home() / f".kart.{getsid()}.socket", show_default=True, help="What socket to use", ) diff --git a/tests/test_cli.py b/tests/test_cli.py index 923df4f12..87a88d506 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,12 +1,15 @@ import contextlib import json +import os import re -import sys from pathlib import Path +import signal +import sys +from time import sleep import pytest -from kart import cli +from kart import cli, is_windows H = pytest.helpers.helpers() @@ -109,3 +112,86 @@ def test_ext_run(tmp_path, cli_runner, sys_path_reset): assert (val1, val2) == (False, 3) assert Path(sfile) == (tmp_path / "three.py") assert sname == "kart.ext_run.three" + + +@pytest.mark.parametrize("use_helper", [False, True]) +def test_sigint_handling_unix(use_helper, tmp_path): + if is_windows: + return + + import subprocess + + kart_bin_dir = Path(sys.executable).parent + kart_exe = kart_bin_dir / "kart" + kart_cli_exe = kart_bin_dir / "kart_cli" + + kart_with_helper_mode = kart_exe if kart_cli_exe.is_file() else kart_cli_exe + kart_without_helper = kart_cli_exe if kart_cli_exe.is_file() else kart_exe + + if use_helper and not kart_with_helper_mode.is_file(): + raise pytest.skip(f"Couldn't find kart helper mode in {kart_bin_dir}") + + kart_to_use = kart_with_helper_mode if use_helper else kart_without_helper + assert kart_to_use.is_file(), "Couldn't find kart" + + # working example + with open(tmp_path / "test.py", "wt") as fs: + fs.write( + "\n".join( + [ + "import os", + "import sys", + "from time import sleep", + "", + "def main(ctx, args):", + " print(os.getpid())", + " fork_id = os.fork()", + " if fork_id == 0:", + " sleep(100)", + " else:", + " print(fork_id)", + " sys.stdout.flush()", + " os.wait()", + "", + ] + ) + ) + + env = os.environ.copy() + env.pop("_KART_PGID_SET", None) + env.pop("NO_CONFIGURE_PROCESS_CLEANUP", None) + + p = subprocess.Popen( + [str(kart_to_use), "ext-run", str(tmp_path / "test.py")], + encoding="utf8", + env=env, + stdout=subprocess.PIPE, + ) + child_pid = p.pid + + sleep(1) + child_pid = int(p.stdout.readline()) + grandchild_pid = int(p.stdout.readline()) + + # The new kart process should be in a new process group. + assert os.getpgid(0) != os.getpgid(child_pid) + # And its subprocess should be in the same process group. + assert os.getpgid(child_pid) == os.getpgid(grandchild_pid) + orig_pgid = os.getpgid(child_pid) + + assert p.poll() == None + os.kill(child_pid, signal.SIGINT) + + sleep(1) + assert p.poll() != None + + def safe_get_pgid(pid): + try: + return os.getpgid(pid) + except Exception: + return -1 + + # The child and grandchildren should now both be dead. + # Their PIDs may now belong to other processes, but at least, they won't be in the same process group as before. + assert safe_get_pgid(child_pid) != orig_pgid + assert safe_get_pgid(grandchild_pid) != orig_pgid From 4e6acfc99a72656282a9da412269e474f515b81c Mon Sep 17 00:00:00 2001 From: Andrew Olsen Date: Tue, 20 Jun 2023 15:41:54 +1200 Subject: [PATCH 5/7] Handle SIGINT: address review comments --- cli_helper/kart.c | 11 +++++++++-- kart/helper.py | 2 +- tests/test_cli.py | 2 -- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/cli_helper/kart.c b/cli_helper/kart.c index c9da3cab8..16d1db0ad 100644 --- a/cli_helper/kart.c +++ b/cli_helper/kart.c @@ -282,8 +282,15 @@ int main(int argc, char **argv, char **environ) int fp = open(getcwd(NULL, 0), O_RDONLY); int fds[4] = {fileno(stdin), fileno(stdout), fileno(stderr), fp}; - char *socket_filename = malloc(strlen(getenv("HOME")) + strlen(".kart.1234567890.socket") + 2); - sprintf(socket_filename, "%s/.kart.%d.socket", getenv("HOME"), getsid(0)); + size_t socket_filename_sz = strlen(getenv("HOME")) + strlen("/.kart..socket") + sizeof(pid_t) * 3 + 1; + char *socket_filename = malloc(socket_filename_sz); + int r = snprintf(socket_filename, socket_filename_sz, "%s/.kart.%d.socket", getenv("HOME"), getsid(0)); + if (r < 0 || (size_t) r >= socket_filename_sz) + { + fprintf(stderr, "Error allocating socket filename\n"); + exit(1); + } + int socket_fd = socket(AF_UNIX, SOCK_STREAM, 0); struct sockaddr_un addr; diff --git a/kart/helper.py b/kart/helper.py index 2a7bea441..14a4baf84 100644 --- a/kart/helper.py +++ b/kart/helper.py @@ -180,7 +180,7 @@ def helper(ctx, socket_filename, timeout, args): # Join the process group of the calling process - so that if they get killed, we get killed to. os.setpgid(0, calling_environment["pid"]) os.environ["_KART_PGID_SET"] = "1" - except Exception as e: + except OSError as e: # Kart will still work even if this fails: it just means SIGINT Ctrl+C might not work properly. # We'll just log it and hope for the best. _helper_log(f"error joining caller's process group: {e}") diff --git a/tests/test_cli.py b/tests/test_cli.py index 87a88d506..12c98faa1 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -167,8 +167,6 @@ def test_sigint_handling_unix(use_helper, tmp_path): env=env, stdout=subprocess.PIPE, ) - child_pid = p.pid - sleep(1) child_pid = int(p.stdout.readline()) grandchild_pid = int(p.stdout.readline()) From 4b4a2a423d527ea977086a64595e2d041bdce07a Mon Sep 17 00:00:00 2001 From: Andrew Olsen Date: Tue, 20 Jun 2023 16:44:40 +1200 Subject: [PATCH 6/7] Fix SIGINT tests on linux --- tests/test_cli.py | 81 +++++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 35 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 12c98faa1..764c829c8 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -114,6 +114,29 @@ def test_ext_run(tmp_path, cli_runner, sys_path_reset): assert sname == "kart.ext_run.three" +TEST_SIGINT_PY = r""" +import datetime +import os +import sys +from time import sleep + + +def main(ctx, args): + print(os.getpid()) + fork_id = os.fork() + if fork_id == 0: + with open(args[0], 'w') as output: + while True: + output.write(datetime.datetime.now().isoformat() + '\n') + output.flush() + sleep(0.01) + else: + print(fork_id) + sys.stdout.flush() + os.wait() +""" + + @pytest.mark.parametrize("use_helper", [False, True]) def test_sigint_handling_unix(use_helper, tmp_path): if is_windows: @@ -135,34 +158,23 @@ def test_sigint_handling_unix(use_helper, tmp_path): assert kart_to_use.is_file(), "Couldn't find kart" # working example - with open(tmp_path / "test.py", "wt") as fs: - fs.write( - "\n".join( - [ - "import os", - "import sys", - "from time import sleep", - "", - "def main(ctx, args):", - " print(os.getpid())", - " fork_id = os.fork()", - " if fork_id == 0:", - " sleep(100)", - " else:", - " print(fork_id)", - " sys.stdout.flush()", - " os.wait()", - "", - ] - ) - ) + test_sigint_py_path = tmp_path / "test_sigint.py" + with open(test_sigint_py_path, "wt") as fs: + fs.write(TEST_SIGINT_PY) + + subprocess_output_path = tmp_path / "output" env = os.environ.copy() env.pop("_KART_PGID_SET", None) env.pop("NO_CONFIGURE_PROCESS_CLEANUP", None) p = subprocess.Popen( - [str(kart_to_use), "ext-run", str(tmp_path / "test.py")], + [ + str(kart_to_use), + "ext-run", + str(test_sigint_py_path), + str(subprocess_output_path), + ], encoding="utf8", env=env, stdout=subprocess.PIPE, @@ -175,21 +187,20 @@ def test_sigint_handling_unix(use_helper, tmp_path): assert os.getpgid(0) != os.getpgid(child_pid) # And its subprocess should be in the same process group. assert os.getpgid(child_pid) == os.getpgid(grandchild_pid) - orig_pgid = os.getpgid(child_pid) - assert p.poll() == None - os.kill(child_pid, signal.SIGINT) + # Time goes past and grandchild keeps writing output + output_size_1 = subprocess_output_path.stat().st_size + sleep(1) + assert p.poll() == None # Grandchild subprocess keeps running... + output_size_2 = subprocess_output_path.stat().st_size + assert output_size_2 > output_size_1 # Grandchild output keeps growing... + os.kill(child_pid, signal.SIGINT) sleep(1) assert p.poll() != None - def safe_get_pgid(pid): - try: - return os.getpgid(pid) - except Exception: - return -1 - - # The child and grandchildren should now both be dead. - # Their PIDs may now belong to other processes, but at least, they won't be in the same process group as before. - assert safe_get_pgid(child_pid) != orig_pgid - assert safe_get_pgid(grandchild_pid) != orig_pgid + # Time goes past but granchild's output has stopped. + output_size_3 = subprocess_output_path.stat().st_size + sleep(1) + output_size_4 = subprocess_output_path.stat().st_size + assert output_size_3 == output_size_4 From 9d4d6ab6d57f551e6ad3bce080b3003d5bfc25d8 Mon Sep 17 00:00:00 2001 From: Andrew Olsen Date: Tue, 20 Jun 2023 20:40:24 +1200 Subject: [PATCH 7/7] SIGINT: Address review comments --- tests/test_cli.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 764c829c8..e0cda0a98 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -137,11 +137,9 @@ def main(ctx, args): """ +@pytest.mark.skipif(is_windows, reason="No SIGINT on windows") @pytest.mark.parametrize("use_helper", [False, True]) def test_sigint_handling_unix(use_helper, tmp_path): - if is_windows: - return - import subprocess kart_bin_dir = Path(sys.executable).parent