Skip to content

Commit

Permalink
gh-109566: regrtest _add_python_opts() handles KeyboardInterrupt (#11…
Browse files Browse the repository at this point in the history
…0062)

In the subprocess code path, wait until the child process completes
with a timeout of EXIT_TIMEOUT seconds.

Fix create_worker_process() regression: use start_new_session=True if
USE_PROCESS_GROUP is true.

WorkerThread.wait_stopped() uses a timeout of 60 seconds, instead of
30 seconds.
  • Loading branch information
vstinner committed Sep 29, 2023
1 parent bd4518c commit 235aacd
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 14 deletions.
23 changes: 18 additions & 5 deletions Lib/test/libregrtest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,18 @@
from .cmdline import _parse_args, Namespace
from .findtests import findtests, split_test_packages, list_cases
from .logger import Logger
from .pgo import setup_pgo_tests
from .result import State
from .results import TestResults, EXITCODE_INTERRUPTED
from .runtests import RunTests, HuntRefleak
from .setup import setup_process, setup_test_dir
from .single import run_single_test, PROGRESS_MIN_TIME
from .pgo import setup_pgo_tests
from .results import TestResults
from .utils import (
StrPath, StrJSON, TestName, TestList, TestTuple, FilterTuple,
strip_py_suffix, count, format_duration,
printlist, get_temp_dir, get_work_dir, exit_timeout,
display_header, cleanup_temp_dir, print_warning,
MS_WINDOWS)
MS_WINDOWS, EXIT_TIMEOUT)


class Regrtest:
Expand Down Expand Up @@ -525,10 +525,23 @@ def _add_python_opts(self):
try:
if hasattr(os, 'execv') and not MS_WINDOWS:
os.execv(cmd[0], cmd)
# execv() do no return and so we don't get to this line on success
# On success, execv() do no return.
# On error, it raises an OSError.
else:
import subprocess
proc = subprocess.run(cmd)
with subprocess.Popen(cmd) as proc:
try:
proc.wait()
except KeyboardInterrupt:
# There is no need to call proc.terminate(): on CTRL+C,
# SIGTERM is also sent to the child process.
try:
proc.wait(timeout=EXIT_TIMEOUT)
except subprocess.TimeoutExpired:
proc.kill()
proc.wait()
sys.exit(EXITCODE_INTERRUPTED)

sys.exit(proc.returncode)
except Exception as exc:
print_warning(f"Failed to change Python options: {exc!r}\n"
Expand Down
13 changes: 8 additions & 5 deletions Lib/test/libregrtest/run_workers.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@
assert MAIN_PROCESS_TIMEOUT >= PROGRESS_UPDATE

# Time to wait until a worker completes: should be immediate
JOIN_TIMEOUT = 30.0 # seconds
WAIT_COMPLETED_TIMEOUT = 30.0 # seconds

# Time to wait a killed process (in seconds)
WAIT_KILLED_TIMEOUT = 60.0


# We do not use a generator so multiple threads can call next().
Expand Down Expand Up @@ -138,7 +141,7 @@ def _kill(self) -> None:
if USE_PROCESS_GROUP:
what = f"{self} process group"
else:
what = f"{self}"
what = f"{self} process"

print(f"Kill {what}", file=sys.stderr, flush=True)
try:
Expand Down Expand Up @@ -390,10 +393,10 @@ def _wait_completed(self) -> None:
popen = self._popen

try:
popen.wait(JOIN_TIMEOUT)
popen.wait(WAIT_COMPLETED_TIMEOUT)
except (subprocess.TimeoutExpired, OSError) as exc:
print_warning(f"Failed to wait for {self} completion "
f"(timeout={format_duration(JOIN_TIMEOUT)}): "
f"(timeout={format_duration(WAIT_COMPLETED_TIMEOUT)}): "
f"{exc!r}")

def wait_stopped(self, start_time: float) -> None:
Expand All @@ -414,7 +417,7 @@ def wait_stopped(self, start_time: float) -> None:
break
dt = time.monotonic() - start_time
self.log(f"Waiting for {self} thread for {format_duration(dt)}")
if dt > JOIN_TIMEOUT:
if dt > WAIT_KILLED_TIMEOUT:
print_warning(f"Failed to join {self} in {format_duration(dt)}")
break

Expand Down
2 changes: 1 addition & 1 deletion Lib/test/libregrtest/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ def display_header(use_resources: tuple[str, ...]):
print(f"== resources ({len(use_resources)}): "
f"{', '.join(sorted(use_resources))}")
else:
print(f"== resources: (all disabled, use -u option)")
print("== resources: (all disabled, use -u option)")

# This makes it easier to remember what to set in your local
# environment when trying to reproduce a sanitizer failure.
Expand Down
9 changes: 6 additions & 3 deletions Lib/test/libregrtest/worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,15 @@ def create_worker_process(runtests: RunTests, output_fd: int,
env['TEMP'] = tmp_dir
env['TMP'] = tmp_dir

# Running the child from the same working directory as regrtest's original
# invocation ensures that TEMPDIR for the child is the same when
# sysconfig.is_python_build() is true. See issue 15300.
#
# Emscripten and WASI Python must start in the Python source code directory
# to get 'python.js' or 'python.wasm' file. Then worker_process() changes
# to a temporary directory created to run tests.
work_dir = os_helper.SAVEDCWD

# Running the child from the same working directory as regrtest's original
# invocation ensures that TEMPDIR for the child is the same when
# sysconfig.is_python_build() is true. See issue 15300.
kwargs: dict[str, Any] = dict(
env=env,
stdout=output_fd,
Expand All @@ -58,6 +59,8 @@ def create_worker_process(runtests: RunTests, output_fd: int,
close_fds=True,
cwd=work_dir,
)
if USE_PROCESS_GROUP:
kwargs['start_new_session'] = True

# Pass json_file to the worker process
json_file = runtests.json_file
Expand Down

0 comments on commit 235aacd

Please sign in to comment.