diff --git a/.bazelrc.deleted_packages b/.bazelrc.deleted_packages index 5654df1266..3eec28298e 100644 --- a/.bazelrc.deleted_packages +++ b/.bazelrc.deleted_packages @@ -28,8 +28,8 @@ common --deleted_packages=gazelle/manifest/hasher common --deleted_packages=gazelle/manifest/test common --deleted_packages=gazelle/modules_mapping common --deleted_packages=gazelle/python -common --deleted_packages=gazelle/pythonconfig common --deleted_packages=gazelle/python/private +common --deleted_packages=gazelle/pythonconfig common --deleted_packages=tests/integration/bzlmod_lockfile common --deleted_packages=tests/integration/compile_pip_requirements common --deleted_packages=tests/integration/compile_pip_requirements_test_from_external_repo diff --git a/news/3809.fixed.md b/news/3809.fixed.md new file mode 100644 index 0000000000..f8c6d09df3 --- /dev/null +++ b/news/3809.fixed.md @@ -0,0 +1,3 @@ +(zipapp) Forwarded termination signals to the application, preserved inherited +ignored signals and exit status, and cleaned up temporary extraction directories. +([#3809](https://github.com/bazel-contrib/rules_python/issues/3809)) diff --git a/python/private/python_bootstrap_template.txt b/python/private/python_bootstrap_template.txt index 482918c038..895d154d3a 100644 --- a/python/private/python_bootstrap_template.txt +++ b/python/private/python_bootstrap_template.txt @@ -28,6 +28,7 @@ if ( from os.path import abspath, dirname, join, basename, normpath import os +import signal import shutil import subprocess @@ -488,6 +489,45 @@ def runfiles_envvar(runfiles_root): return (None, None) +def _run_subprocess(argv, env, cwd): + if IS_WINDOWS: + return subprocess.call(argv, env=env, cwd=cwd) + + child = None + pending_signals = [] + + def forward_signal(signum, _frame): + if child is None: + pending_signals.append(signum) + else: + try: + # Keep wait() as the sole child reaper. Popen.send_signal() may call + # poll(), which can race wait() and lose the child's exit status. + os.kill(child.pid, signum) + except ProcessLookupError: + pass + + previous_handlers = {} + for name in ("SIGHUP", "SIGINT", "SIGQUIT", "SIGTERM"): + signum = getattr(signal, name, None) + if signum is not None: + # Ignored signals must remain ignored across the child exec. + if signal.getsignal(signum) != signal.SIG_IGN: + previous_handlers[signum] = signal.signal(signum, forward_signal) + + try: + child = subprocess.Popen(argv, env=env, cwd=cwd) + for signum in pending_signals: + forward_signal(signum, None) + ret_code = child.wait() + finally: + for signum, handler in previous_handlers.items(): + signal.signal(signum, handler) + + if ret_code < 0: + return 128 - ret_code + return ret_code + def execute_file(python_program, main_filename, args, env, runfiles_root, workspace, delete_dirs): # type: (str, str, list[str], dict[str, str], str, str|None, str|None) -> ... @@ -535,8 +575,7 @@ def execute_file(python_program, main_filename, args, env, runfiles_root, print_verbose("run: subproc: environ:", mapping=os.environ) print_verbose("run: subproc: cwd:", workspace) print_verbose("run: subproc: argv:", values=argv) - ret_code = subprocess.call( - argv, env=env, cwd=workspace) + ret_code = _run_subprocess(argv, env=env, cwd=workspace) print_verbose("run: subproc: exit code:", ret_code) if delete_dirs: diff --git a/python/private/stage1_bootstrap_template.sh b/python/private/stage1_bootstrap_template.sh index 4374ff95b0..ef098127fd 100644 --- a/python/private/stage1_bootstrap_template.sh +++ b/python/private/stage1_bootstrap_template.sh @@ -60,12 +60,14 @@ declare -a INTERPRETER_ARGS_FROM_TARGET=( %interpreter_args% ) +cleanup_zip_dir="" if [[ "$IS_ZIPFILE" == "1" ]]; then # NOTE: Macs have an old version of mktemp, so we must use only the # minimal functionality of it. zip_dir=$(mktemp -d) if [[ -n "$zip_dir" && -z "${RULES_PYTHON_BOOTSTRAP_VERBOSE:-}" ]]; then + cleanup_zip_dir=1 trap 'rm -fr "$zip_dir"' EXIT fi # unzip emits a warning and exits with code 1 when there is extraneous data, @@ -153,7 +155,8 @@ python_exe=$(find_python_interpreter $RUNFILES_DIR $PYTHON_BINARY) # Zip files have to re-create the venv bin/python3 symlink because they # don't contain it already. if [[ "$IS_ZIPFILE" == "1" ]]; then - use_exec=0 + # A detached watcher below owns cleanup after exec replaces this bootstrap. + use_exec=1 # It should always be under runfiles, but double check this. We don't # want to accidentally create symlinks elsewhere. if [[ "$python_exe" != $RUNFILES_DIR/* ]]; then @@ -297,10 +300,6 @@ if [[ -z "${PYTHONSAFEPATH+x}" ]]; then interpreter_env+=("PYTHONSAFEPATH=${PYTHONSAFEPATH-1}") fi -if [[ "$IS_ZIPFILE" == "1" ]]; then - interpreter_args+=("-XRULES_PYTHON_ZIP_DIR=$zip_dir") -fi - if [[ -n "${RULES_PYTHON_ADDITIONAL_INTERPRETER_ARGS}" ]]; then read -a additional_interpreter_args <<< "${RULES_PYTHON_ADDITIONAL_INTERPRETER_ARGS}" interpreter_args+=("${additional_interpreter_args[@]}") @@ -325,6 +324,52 @@ command=( "$@" ) +# Prints an identifier for a running process that changes if the PID is reused, +# or nothing once the process has exited. A zombie counts as exited, so it +# doesn't matter whether the caller has reaped it yet. +process_identity() { + local stat fields + if [[ -r "/proc/$1/stat" ]]; then + stat=$(< "/proc/$1/stat") || return 0 + # The command name is in parentheses and may contain spaces, so split the + # fields after the last ")": field 3 (state) through field 22 (start time). + read -r -a fields <<< "${stat##*) }" + if [[ "${fields[0]}" != "Z" ]]; then + echo "${fields[19]}" + fi + elif command -v ps >/dev/null 2>&1; then + stat=$(ps -o stat=,lstart= -p "$1" 2>/dev/null) + stat="${stat#"${stat%%[![:space:]]*}"}" + if [[ -n "$stat" && "$stat" != Z* ]]; then + echo "${stat#* }" + fi + elif kill -0 "$1" 2>/dev/null; then + echo "running" + fi +} + +if [[ -n "$cleanup_zip_dir" ]]; then + # exec replaces this shell, so the EXIT trap can't remove the extracted files. + # Instead, start a watcher that removes them once this PID, which becomes the + # Python process, exits. Unlike the trap, this also works when the process is + # killed with SIGKILL. The watcher is double-forked so it isn't a child of the + # Python process, and it doesn't hold this process's stdio open. It stays in + # the caller's process group, so it ignores the signals sent to a whole group + # (e.g. Ctrl-C in a terminal) and exits on its own once the process is gone. + # Unlike the implicit SIGINT ignore for background jobs, `trap ''` also + # applies to the commands it runs. + launcher_pid=$$ + launcher_identity=$(process_identity "$launcher_pid") + ( ( + trap '' INT QUIT HUP TERM + while [[ -n "$launcher_identity" && + "$(process_identity "$launcher_pid")" == "$launcher_identity" ]]; do + sleep 1 + done + rm -fr "$zip_dir" + ) /dev/null 2>&1 & ) +fi + # We use `exec` instead of a child process so that signals sent directly (e.g. # using `kill`) to this process (the PID seen by the calling process) are # received by the Python process. Otherwise, this process receives the signal @@ -333,8 +378,8 @@ command=( # for more information. # # However, we can't use exec when there is cleanup to do afterwards. Control -# must return to this process so it can run the trap handlers. Such cases -# occur when zip mode or recreate_venv_at_runtime creates temporary files. +# must return to this process so it can run the trap handlers. This case +# occurs when recreate_venv_at_runtime creates a temporary venv. if [[ "$use_exec" == "0" ]]; then "${command[@]}" exit $? diff --git a/python/private/zipapp/zip_main_template.py b/python/private/zipapp/zip_main_template.py index 709e08815c..c93f21b4f2 100644 --- a/python/private/zipapp/zip_main_template.py +++ b/python/private/zipapp/zip_main_template.py @@ -25,6 +25,7 @@ import os # noqa: E402 import shutil # noqa: E402 +import signal # noqa: E402 import stat # noqa: E402 import subprocess # noqa: E402 import tempfile # noqa: E402 @@ -234,6 +235,56 @@ def create_runfiles_root(): return join(extract_root, "runfiles") +def run_subprocess(subprocess_argv, env, cwd): + if IS_WINDOWS: + return subprocess.call(subprocess_argv, env=env, cwd=cwd) + + child = None + pending_signals = [] + + def forward_signal(signum, _frame): + if child is None: + pending_signals.append(signum) + else: + try: + # Keep wait() as the sole child reaper. Popen.send_signal() may call + # poll(), which can race wait() and lose the child's exit status. + os.kill(child.pid, signum) + except ProcessLookupError: + pass + + previous_handlers = {} + for name in ("SIGHUP", "SIGINT", "SIGQUIT", "SIGTERM"): + signum = getattr(signal, name, None) + if signum is not None: + # Ignored signals must remain ignored across the child exec. + if signal.getsignal(signum) != signal.SIG_IGN: + previous_handlers[signum] = signal.signal(signum, forward_signal) + + try: + child = subprocess.Popen(subprocess_argv, env=env, cwd=cwd) + for signum in pending_signals: + forward_signal(signum, None) + ret_code = child.wait() + finally: + for signum, handler in previous_handlers.items(): + signal.signal(signum, handler) + + if ret_code < 0: + return 128 - ret_code + return ret_code + + +def remove_readonly(function, path, _exc_info): + # Windows won't unlink extracted files whose archived mode is read-only. + try: + os.chmod(path, stat.S_IWRITE) + function(path) + except OSError as error: + # Cleanup errors must not replace the application's exit status. + print_verbose("cleanup: failed to remove:", path, error) + + def execute_file( python_program, main_filename, @@ -269,14 +320,12 @@ def execute_file( # workspace after the process finishes so control must return here. try: subprocess_argv = [python_program] - if not EXTRACT_ROOT: - subprocess_argv.append(f"-XRULES_PYTHON_ZIP_DIR={dirname(runfiles_root)}") subprocess_argv.append(main_filename) subprocess_argv += args print_verbose("subprocess env:", mapping=env) print_verbose("subprocess cwd:", workspace) print_verbose("subprocess argv:", values=subprocess_argv) - ret_code = subprocess.call(subprocess_argv, env=env, cwd=workspace) + ret_code = run_subprocess(subprocess_argv, env=env, cwd=workspace) print_verbose("subprocess exit code:", ret_code) sys.exit(ret_code) finally: @@ -286,7 +335,11 @@ def execute_file( # whole temporary directory. extract_root = dirname(runfiles_root) print_verbose("cleanup: rmtree: ", extract_root) - shutil.rmtree(extract_root, True) + if IS_WINDOWS: + # onerror is supported by older bootstrap interpreters too. + shutil.rmtree(extract_root, onerror=remove_readonly) + else: + shutil.rmtree(extract_root, True) def finish_venv_setup(runfiles_root): diff --git a/python/private/zipapp/zip_shell_template.sh b/python/private/zipapp/zip_shell_template.sh index d79331444a..f333765622 100644 --- a/python/private/zipapp/zip_shell_template.sh +++ b/python/private/zipapp/zip_shell_template.sh @@ -35,6 +35,7 @@ if [[ -n "${RULES_PYTHON_ADDITIONAL_INTERPRETER_ARGS}" ]]; then fi +cleanup_zip_dir="" if [[ -n "$RULES_PYTHON_EXTRACT_ROOT" ]]; then zip_dir="$RULES_PYTHON_EXTRACT_ROOT/$EXTRACT_DIR/$ZIP_HASH" if [[ ! -e "$zip_dir/__main__.py" ]]; then @@ -49,6 +50,7 @@ else # Unzip emits a warning and exits 1 with the prelude ( unzip -q -d "$zip_dir" "$0" 2>/dev/null || true ) if [[ -n "$zip_dir" && -z "${RULES_PYTHON_BOOTSTRAP_VERBOSE:-}" ]]; then + cleanup_zip_dir=1 trap 'rm -fr "$zip_dir"' EXIT fi fi @@ -70,19 +72,62 @@ command=( env "${interpreter_env[@]}" "$python_exe" - "-XRULES_PYTHON_ZIP_DIR=$zip_dir" "${interpreter_args[@]}" "${INTERPRETER_ARGS_FROM_TARGET[@]}" "$RUNFILES_DIR/$STAGE2_BOOTSTRAP" "$@" ) -# NOTE: because exec isn't used, signals don't propagate to the child -# TODO: Use exec and let the program handle cleanup. Without exec, -# signals don't propagate to the child nicely. +# Prints an identifier for a running process that changes if the PID is reused, +# or nothing once the process has exited. A zombie counts as exited, so it +# doesn't matter whether the caller has reaped it yet. +process_identity() { + local stat fields + if [[ -r "/proc/$1/stat" ]]; then + stat=$(< "/proc/$1/stat") || return 0 + # The command name is in parentheses and may contain spaces, so split the + # fields after the last ")": field 3 (state) through field 22 (start time). + read -r -a fields <<< "${stat##*) }" + if [[ "${fields[0]}" != "Z" ]]; then + echo "${fields[19]}" + fi + elif command -v ps >/dev/null 2>&1; then + stat=$(ps -o stat=,lstart= -p "$1" 2>/dev/null) + stat="${stat#"${stat%%[![:space:]]*}"}" + if [[ -n "$stat" && "$stat" != Z* ]]; then + echo "${stat#* }" + fi + elif kill -0 "$1" 2>/dev/null; then + echo "running" + fi +} + +if [[ -n "$cleanup_zip_dir" ]]; then + # exec replaces this shell, so the EXIT trap can't remove the extracted files. + # Instead, start a watcher that removes them once this PID, which becomes the + # Python process, exits. Unlike the trap, this also works when the process is + # killed with SIGKILL. The watcher is double-forked so it isn't a child of the + # Python process, and it doesn't hold this process's stdio open. It stays in + # the caller's process group, so it ignores the signals sent to a whole group + # (e.g. Ctrl-C in a terminal) and exits on its own once the process is gone. + # Unlike the implicit SIGINT ignore for background jobs, `trap ''` also + # applies to the commands it runs. + launcher_pid=$$ + launcher_identity=$(process_identity "$launcher_pid") + ( ( + trap '' INT QUIT HUP TERM + while [[ -n "$launcher_identity" && + "$(process_identity "$launcher_pid")" == "$launcher_identity" ]]; do + sleep 1 + done + rm -fr "$zip_dir" + ) /dev/null 2>&1 & ) +fi + +# We use `exec` instead of a child process so that signals sent directly (e.g. +# using `kill`) to this process (the PID seen by the calling process) are +# received by the Python process. Otherwise, this process receives the signal +# and the Python process keeps running. # See https://github.com/bazel-contrib/rules_python/issues/2043#issuecomment-2215469971 # for more information. -"${command[@]}" -# Explicit exit is needed because the implicit next line the zip file this -# template is prepended to. -exit 0 +exec "${command[@]}" diff --git a/tests/bootstrap_impls/bin.py b/tests/bootstrap_impls/bin.py index 0713b5f1be..7ebe0c047e 100644 --- a/tests/bootstrap_impls/bin.py +++ b/tests/bootstrap_impls/bin.py @@ -13,8 +13,22 @@ # limitations under the License. import os +import signal import sys +if sys.argv[1:] in (["handled"], ["unhandled"]): + if sys.argv[1:] == ["handled"]: + + def handle(signum, _frame): + print(f"received:{signum}", flush=True) + + signal.signal(signal.SIGTERM, handle) + + print(f"file: {__file__}", flush=True) + print(f"ready:{os.getpid()}", flush=True) + signal.pause() + raise SystemExit(0) + print("Hello") print( "RULES_PYTHON_ZIP_DIR:{}".format(sys._xoptions.get("RULES_PYTHON_ZIP_DIR", "UNSET")) diff --git a/tests/bootstrap_impls/run_binary_zip_yes_test.sh b/tests/bootstrap_impls/run_binary_zip_yes_test.sh index 77fe4d3609..43afc5fda5 100755 --- a/tests/bootstrap_impls/run_binary_zip_yes_test.sh +++ b/tests/bootstrap_impls/run_binary_zip_yes_test.sh @@ -33,12 +33,98 @@ actual=$($bin) # How we detect if a zip file was executed from depends on which bootstrap # is used. -# bootstrap_impl=script outputs RULES_PYTHON_ZIP_DIR: +# bootstrap_impl=script outputs file:/runfiles/... # bootstrap_impl=system_python outputs file:.*Bazel.runfiles (or .exe.runfiles on Windows) -expected_pattern="RULES_PYTHON_ZIP_DIR:/\|file:.*Bazel.runfiles\|file:.*\.exe\.runfiles" +expected_pattern="file:.*/runfiles/\|file:.*Bazel.runfiles\|file:.*\.exe\.runfiles" if ! (echo "$actual" | grep "$expected_pattern" ) >/dev/null; then echo "expected output to match: $expected_pattern" echo "but got: $actual" exit 1 fi +case "$(uname -s)" in + CYGWIN*|MINGW*|MSYS*) exit 0 ;; +esac + +# Verify that temporary extracted zip directories are cleaned up when the binary exits. +extracted_file=$(echo "$actual" | sed -n 's/^file: //p') +extracted_root="${extracted_file%%/runfiles/*}" +assert_removed_eventually() { + local path="$1" + for _ in {1..150}; do + [[ ! -e "$path" ]] && return 0 + sleep 0.1 + done + echo "ERROR: temporary zip directory was not cleaned up after exit: $path" >&2 + return 1 +} +assert_removed_eventually "$extracted_root" || exit 1 + +test_dir=$(mktemp -d) +launcher_pid="" +application_pid="" +watchdog_pid="" + +cleanup() { + if [[ -n "${watchdog_pid}" ]]; then + kill "${watchdog_pid}" 2>/dev/null || true + fi + if [[ -n "${launcher_pid}" ]]; then + kill -KILL "${launcher_pid}" 2>/dev/null || true + fi + if [[ -n "${application_pid}" ]]; then + kill -KILL "${application_pid}" 2>/dev/null || true + fi + rm -rf "${test_dir}" +} +trap cleanup EXIT + +run_signal_case() { + local mode="$1" + local expected_exit="$2" + local expected_output="$3" + local log="${test_dir}/${mode}.log" + + # An inherited ignored SIGHUP must remain ignored through both bootstraps. + ( trap '' HUP; exec "$bin" "${mode}" ) >"${log}" 2>&1 & + launcher_pid=$! + for _ in {1..100}; do + if grep -F "ready:" "${log}" >/dev/null; then + break + fi + sleep 0.1 + done + grep -F "ready:" "${log}" >/dev/null || return 1 + application_pid=$(sed -n 's/^ready://p' "${log}") + + kill -HUP "${launcher_pid}" + sleep 0.2 + kill -0 "${application_pid}" || return 1 + kill -TERM "${launcher_pid}" + ( + sleep 10 + kill -KILL "${launcher_pid}" "${application_pid}" 2>/dev/null || true + ) & + watchdog_pid=$! + wait "${launcher_pid}" + exit_code=$? + kill "${watchdog_pid}" 2>/dev/null || true + watchdog_pid="" + + if [[ "${exit_code}" != "${expected_exit}" ]]; then + echo "expected exit ${expected_exit}, got ${exit_code}" >&2 + cat "${log}" >&2 + return 1 + fi + if [[ -n "${expected_output}" ]]; then + grep -F "${expected_output}" "${log}" >/dev/null || return 1 + fi + local extracted_file + extracted_file=$(sed -n 's/^file: //p' "${log}") + assert_removed_eventually "${extracted_file%%/runfiles/*}" || return 1 + launcher_pid="" + application_pid="" +} + +run_signal_case handled 0 "received:15" || exit 1 +run_signal_case unhandled 143 "" || exit 1 diff --git a/tests/py_zipapp/BUILD.bazel b/tests/py_zipapp/BUILD.bazel index e0f878cbe1..ede19baff8 100644 --- a/tests/py_zipapp/BUILD.bazel +++ b/tests/py_zipapp/BUILD.bazel @@ -4,6 +4,7 @@ load("//python:py_library.bzl", "py_library") load("//python:py_test.bzl", "py_test") load("//python/private:bzlmod_enabled.bzl", "BZLMOD_ENABLED") # buildifier: disable=bzl-visibility load("//python/zipapp:py_zipapp_binary.bzl", "py_zipapp_binary") +load("//tests/support:support.bzl", "SUPPORTS_BOOTSTRAP_SCRIPT") py_binary( name = "venv_bin", @@ -120,3 +121,29 @@ py_library( experimental_venvs_site_packages = "//python/config_settings:venvs_site_packages", imports = ["site-packages"], ) + +py_binary( + name = "signals_bin", + srcs = ["signals_main.py"], + config_settings = { + "//python/config_settings:bootstrap_impl": "script", + }, + main = "signals_main.py", + target_compatible_with = SUPPORTS_BOOTSTRAP_SCRIPT, +) + +py_zipapp_binary( + name = "signals_zipapp", + binary = ":signals_bin", + target_compatible_with = SUPPORTS_BOOTSTRAP_SCRIPT, +) + +py_test( + name = "zipapp_signals_test", + srcs = ["zipapp_signals_test.py"], + data = [":signals_zipapp"], + env = { + "TEST_ZIPAPP": "$(location :signals_zipapp)", + }, + target_compatible_with = SUPPORTS_BOOTSTRAP_SCRIPT, +) diff --git a/tests/py_zipapp/main.py b/tests/py_zipapp/main.py index 5770170d2c..595bed754e 100644 --- a/tests/py_zipapp/main.py +++ b/tests/py_zipapp/main.py @@ -1,8 +1,48 @@ -"A trivial zipapp that prints a message" +"A trivial zipapp that prints a message or waits for a signal." + +import os +import signal +import stat +import sys + + +def wait_for_signal(handle_signal): + if handle_signal: + + def handle(signum, _frame): + print(f"received:{signum}", flush=True) + + signal.signal(signal.SIGTERM, handle) + + print(f"file:{__file__}", flush=True) + print(f"ready:{os.getpid()}", flush=True) + signal.pause() + return 0 def main(): + if sys.argv[1:] == ["readonly-file"]: + path = os.path.join(os.path.dirname(__file__), "readonly-cleanup-test") + with open(path, "w") as file: + file.write("Cleanup must remove read-only extracted files.\n") + os.chmod(path, stat.S_IREAD) + if sys.argv[1:] == ["check-ignored-signals"]: + ignored = all( + signal.getsignal(sig) == signal.SIG_IGN + for sig in (signal.SIGHUP, signal.SIGINT, signal.SIGQUIT, signal.SIGTERM) + ) + print(f"ignored-signals:{ignored}") + return 0 + if sys.argv[1:] == ["wait-for-sigterm"]: + return wait_for_signal(handle_signal=True) + if sys.argv[1:] == ["wait-for-unhandled-sigterm"]: + return wait_for_signal(handle_signal=False) + if len(sys.argv) == 3 and sys.argv[1] == "exit": + return int(sys.argv[2]) + print("Hello from zipapp") + print(f"file:{__file__}") + print(f"xoption_zip_dir:{sys._xoptions.get('RULES_PYTHON_ZIP_DIR', 'UNSET')}") try: import some_dep @@ -12,13 +52,12 @@ def main(): print(f"dep: {pkgdep.pkgmod}") except ImportError as e: - import sys - e.add_note( "Failed to import a dependency.\n" + "sys.path:\n" + "\n".join(sys.path) ) raise + return 0 if __name__ == "__main__": - main() + raise SystemExit(main()) diff --git a/tests/py_zipapp/signals_main.py b/tests/py_zipapp/signals_main.py new file mode 100644 index 0000000000..2fc519a2ee --- /dev/null +++ b/tests/py_zipapp/signals_main.py @@ -0,0 +1,26 @@ +"A zipapp that reports its PID and extraction directory, then waits for SIGTERM." + +import os +import signal +import sys +import time + + +def main(): + if len(sys.argv) > 1 and sys.argv[1].startswith("--exit-code="): + sys.exit(int(sys.argv[1].split("=", 1)[1])) + + def on_sigterm(signum, frame): + # print() isn't reentrant; the signal may arrive while main() prints. + os.write(sys.stdout.fileno(), b"got SIGTERM\n") + sys.exit(0) + + signal.signal(signal.SIGTERM, on_sigterm) + print(f"pid={os.getpid()}", flush=True) + print(f"zip_dir={__file__.split(os.sep + 'runfiles' + os.sep, 1)[0]}", flush=True) + while True: + time.sleep(60) + + +if __name__ == "__main__": + main() diff --git a/tests/py_zipapp/system_python_zipapp_test.py b/tests/py_zipapp/system_python_zipapp_test.py index 7c3e2deeaf..2ea85e8fc3 100644 --- a/tests/py_zipapp/system_python_zipapp_test.py +++ b/tests/py_zipapp/system_python_zipapp_test.py @@ -1,9 +1,40 @@ import os +import signal import subprocess +import sys +import tempfile +import time import unittest +def _extract_root_from_output(output): + for line in output.splitlines(): + if line.startswith("file:"): + file_path = line.removeprefix("file:").strip() + runfiles_marker = f"{os.sep}runfiles{os.sep}" + if runfiles_marker in file_path: + return file_path.split(runfiles_marker, 1)[0] + raise AssertionError(f"Could not find file: line in output:\n{output}") + + +def _assert_removed_eventually(test, path): + # Shell launchers exec the app and leave cleanup to a detached watcher. + deadline = time.monotonic() + 15 + while os.path.exists(path): + if time.monotonic() >= deadline: + test.fail(f"Temporary zip extraction dir leaked after exit: {path}") + time.sleep(0.1) + + class SystemPythonZipAppTest(unittest.TestCase): + def test_zipapp_cleans_readonly_files(self): + output = subprocess.check_output( + [os.environ["TEST_ZIPAPP"], "readonly-file"], + stderr=subprocess.STDOUT, + text=True, + ) + _assert_removed_eventually(self, _extract_root_from_output(output)) + def test_zipapp_runnable(self): zipapp_path = os.environ["TEST_ZIPAPP"] @@ -26,6 +57,172 @@ def test_zipapp_runnable(self): ) self.assertIn("Hello from zipapp", output) self.assertIn("dep:", output) + self.assertIn( + "xoption_zip_dir:UNSET", + output, + f"RULES_PYTHON_ZIP_DIR -X option leaked into user main.py:\n{output}", + ) + + extract_root = _extract_root_from_output(output) + _assert_removed_eventually(self, extract_root) + + +@unittest.skipUnless(os.name == "posix", "POSIX signals are required") +class PosixSignalZipAppTest(unittest.TestCase): + def zipapp_command(self, *args, invoke_with_python=False): + zipapp_path = os.environ["TEST_ZIPAPP"] + command = [zipapp_path] + if invoke_with_python: + command.insert(0, sys.executable) + return [*command, *args] + + def start_signal_app(self, mode, invoke_with_python, preexec_fn=None): + process = subprocess.Popen( + self.zipapp_command(mode, invoke_with_python=invoke_with_python), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + preexec_fn=preexec_fn, + ) + self.assertIsNotNone(process.stdout) + file_line = process.stdout.readline().strip() + self.assertTrue(file_line.startswith("file:"), file_line) + extract_root = _extract_root_from_output(file_line) + ready = process.stdout.readline().strip() + self.assertTrue(ready.startswith("ready:"), ready) + return process, int(ready.removeprefix("ready:")), extract_root + + def stop_process(self, process, application_pid): + if process.poll() is None: + process.kill() + process.wait() + try: + os.kill(application_pid, signal.SIGTERM) + except ProcessLookupError: + pass + if process.stdout is not None: + process.stdout.close() + + def test_zipapp_forwards_sigterm(self): + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + process, application_pid, extract_root = self.start_signal_app( + "wait-for-sigterm", invoke_with_python + ) + try: + process.terminate() + output, _ = process.communicate(timeout=10) + self.assertEqual(0, process.returncode, output) + self.assertIn(f"received:{signal.SIGTERM}", output) + _assert_removed_eventually(self, extract_root) + finally: + self.stop_process(process, application_pid) + + def test_zipapp_preserves_signal_termination(self): + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + process, application_pid, extract_root = self.start_signal_app( + "wait-for-unhandled-sigterm", invoke_with_python + ) + try: + process.terminate() + output, _ = process.communicate(timeout=10) + expected = ( + 128 + signal.SIGTERM if invoke_with_python else -signal.SIGTERM + ) + self.assertEqual(expected, process.returncode, output) + _assert_removed_eventually(self, extract_root) + finally: + self.stop_process(process, application_pid) + + def test_zipapp_preserves_extract_root_cache(self): + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + with tempfile.TemporaryDirectory() as extract_root_env: + env = {**os.environ, "RULES_PYTHON_EXTRACT_ROOT": extract_root_env} + proc = subprocess.run( + self.zipapp_command(invoke_with_python=invoke_with_python), + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + check=True, + ) + extract_root = _extract_root_from_output(proc.stdout) + self.assertTrue( + extract_root.startswith(extract_root_env), + f"Expected {extract_root} to be under {extract_root_env}", + ) + self.assertTrue( + os.path.exists(extract_root), + f"RULES_PYTHON_EXTRACT_ROOT cache dir was unexpectedly deleted: {extract_root}", + ) + self.assertIn( + "xoption_zip_dir:UNSET", + proc.stdout, + "RULES_PYTHON_ZIP_DIR should not be exposed when " + f"RULES_PYTHON_EXTRACT_ROOT is set:\n{proc.stdout}", + ) + + def test_zipapp_preserves_sig_ign(self): + sighup = getattr(signal, "SIGHUP") + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + process, application_pid, _ = self.start_signal_app( + "wait-for-unhandled-sigterm", + invoke_with_python, + preexec_fn=lambda: signal.signal(sighup, signal.SIG_IGN), + ) + try: + process.send_signal(sighup) + time.sleep(0.2) + self.assertIsNone( + process.poll(), + f"Process died from SIGHUP (returncode={process.returncode}) instead of ignoring SIG_IGN", + ) + process.terminate() + output, _ = process.communicate(timeout=10) + expected = ( + 128 + signal.SIGTERM if invoke_with_python else -signal.SIGTERM + ) + self.assertEqual( + expected, + process.returncode, + f"Process died from SIGHUP instead of ignoring SIG_IGN:\n{output}", + ) + finally: + self.stop_process(process, application_pid) + + def test_zipapp_preserves_nonzero_exit_status(self): + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + process = subprocess.run( + self.zipapp_command( + "exit", "17", invoke_with_python=invoke_with_python + ), + check=False, + ) + self.assertEqual(17, process.returncode) + + def test_zipapp_inherits_ignored_signals(self): + def ignore_signals(): + for name in ("SIGHUP", "SIGINT", "SIGQUIT", "SIGTERM"): + signal.signal(getattr(signal, name), signal.SIG_IGN) + + for invoke_with_python in (False, True): + with self.subTest(invoke_with_python=invoke_with_python): + process = subprocess.run( + self.zipapp_command( + "check-ignored-signals", invoke_with_python=invoke_with_python + ), + preexec_fn=ignore_signals, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + timeout=10, + check=True, + ) + self.assertIn("ignored-signals:True", process.stdout) if __name__ == "__main__": diff --git a/tests/py_zipapp/zipapp_signals_test.py b/tests/py_zipapp/zipapp_signals_test.py new file mode 100644 index 0000000000..e6221f39ed --- /dev/null +++ b/tests/py_zipapp/zipapp_signals_test.py @@ -0,0 +1,108 @@ +import os +import signal +import subprocess +import time +import unittest + +# How long to wait for the extracted files to be removed after the zipapp exits. +_CLEANUP_TIMEOUT_SECONDS = 15 + + +class ZipAppSignalsTest(unittest.TestCase): + def setUp(self): + self.zipapp_path = os.environ["TEST_ZIPAPP"] + + def _start(self, new_session=False): + proc = subprocess.Popen( + [self.zipapp_path], + stdout=subprocess.PIPE, + text=True, + start_new_session=new_session, + ) + self.addCleanup(self._kill, proc) + assert proc.stdout is not None + pid_line = proc.stdout.readline().strip() + zip_dir_line = proc.stdout.readline().strip() + if not pid_line.startswith("pid=") or not zip_dir_line.startswith("zip_dir="): + self.fail(f"unexpected zipapp output: {pid_line!r} {zip_dir_line!r}") + pid = int(pid_line.split("=", 1)[1]) + if pid != proc.pid: + # The launcher didn't exec, so the Python process can outlive it and + # keep stdout open. Make sure it's killed too. + self.addCleanup(self._kill_pid, pid) + zip_dir = zip_dir_line.split("=", 1)[1] + self.assertTrue(os.path.isdir(zip_dir), f"{zip_dir} does not exist") + return proc, pid, zip_dir + + def _kill(self, proc): + if proc.poll() is None: + proc.kill() + try: + proc.communicate(timeout=10) + except subprocess.TimeoutExpired: + pass + + def _kill_pid(self, pid): + try: + os.kill(pid, signal.SIGTERM) + except ProcessLookupError: + pass + + def assertRemovedEventually(self, path): + deadline = time.monotonic() + _CLEANUP_TIMEOUT_SECONDS + while os.path.exists(path): + if time.monotonic() > deadline: + self.fail(f"{path} was not removed after the zipapp exited") + time.sleep(0.2) + + def test_signal_reaches_python_process(self): + proc, pid, zip_dir = self._start() + # The launcher must exec the interpreter, so the PID the caller started + # is the Python process and signals sent to it are delivered there. + self.assertEqual(pid, proc.pid) + + proc.send_signal(signal.SIGTERM) + output, _ = proc.communicate(timeout=30) + + self.assertIn("got SIGTERM", output) + self.assertEqual(proc.returncode, 0) + self.assertRemovedEventually(zip_dir) + + def test_extracted_files_removed_after_sigkill(self): + proc, _, zip_dir = self._start() + + proc.kill() + proc.communicate(timeout=30) + + self.assertRemovedEventually(zip_dir) + + def test_extracted_files_removed_before_process_is_reaped(self): + proc, _, zip_dir = self._start() + + # Leave the exited process as a zombie: cleanup must not wait for the + # caller to reap it. + proc.kill() + + self.assertRemovedEventually(zip_dir) + + def test_extracted_files_removed_after_process_group_signal(self): + # A terminal's Ctrl-C (SIGINT), hangup (SIGHUP) or a supervisor stopping + # the whole process group (SIGTERM) signals every process in the group, + # including the cleanup watcher, which must survive to do its job. + for sig in (signal.SIGINT, signal.SIGHUP, signal.SIGTERM): + with self.subTest(signal=sig.name): + proc, _, zip_dir = self._start(new_session=True) + + os.killpg(proc.pid, sig) + proc.communicate(timeout=30) + + self.assertRemovedEventually(zip_dir) + + def test_exit_code_is_propagated(self): + result = subprocess.run([self.zipapp_path, "--exit-code=3"], timeout=60) + + self.assertEqual(result.returncode, 3) + + +if __name__ == "__main__": + unittest.main()