From 615f31f33c7736cf93f6dffd336469eb4ef80d51 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 14 Jun 2022 18:09:54 +0200 Subject: [PATCH] gh-93353: regrtest creates worker directory in the parent regrtest now creates the working directory of the worker process in the parent process. Don't check for temporary files if Python is run on WASI: WASI doesn't pass environment variables to the worker process. --- Lib/test/libregrtest/main.py | 37 ++++++++++------------ Lib/test/libregrtest/runtest_mp.py | 51 ++++++++++++++++++++---------- Lib/test/test_regrtest.py | 20 +++++++----- 3 files changed, 64 insertions(+), 44 deletions(-) diff --git a/Lib/test/libregrtest/main.py b/Lib/test/libregrtest/main.py index 85bf6e1d48e3a7..f61f3bbbdb77eb 100644 --- a/Lib/test/libregrtest/main.py +++ b/Lib/test/libregrtest/main.py @@ -605,9 +605,9 @@ def set_temp_dir(self): self.tmp_dir = self.ns.tempdir if not self.tmp_dir: - # When tests are run from the Python build directory, it is best practice - # to keep the test files in a subfolder. This eases the cleanup of leftover - # files using the "make distclean" command. + # When tests are run from the Python build directory, it is best + # practice to keep the test files in a subfolder. This eases the + # cleanup of leftover files using the "make distclean" command. if sysconfig.is_python_build(): self.tmp_dir = sysconfig.get_config_var('abs_builddir') if self.tmp_dir is None: @@ -634,11 +634,7 @@ def create_temp_dir(self): nounce = random.randint(0, 1_000_000) else: nounce = os.getpid() - if self.worker_test_name is not None: - test_cwd = 'test_python_worker_{}'.format(nounce) - else: - test_cwd = 'test_python_{}'.format(nounce) - test_cwd += os_helper.FS_NONASCII + test_cwd = f'test_python_{nounce}_{os_helper.FS_NONASCII}' test_cwd = os.path.join(self.tmp_dir, test_cwd) return test_cwd @@ -658,25 +654,26 @@ def cleanup(self): def main(self, tests=None, **kwargs): self.parse_args(kwargs) - self.set_temp_dir() + if self.worker_test_name is None: + self.set_temp_dir() + else: + self.tmp_dir = os.getcwd() if self.ns.cleanup: self.cleanup() sys.exit(0) - test_cwd = self.create_temp_dir() - try: - # Run the tests in a context manager that temporarily changes the CWD - # to a temporary and writable directory. If it's not possible to - # create or change the CWD, the original CWD will be used. + # Run the tests in a context manager that temporarily changes the + # CWD to a temporary and writable directory. If it's not possible + # to create or change the CWD, the original CWD will be used. # The original CWD is available from os_helper.SAVEDCWD. - with os_helper.temp_cwd(test_cwd, quiet=True): - # When using multiprocessing, worker processes will use test_cwd - # as their parent temporary directory. So when the main process - # exit, it removes also subdirectories of worker processes. - self.ns.tempdir = test_cwd - + if self.worker_test_name is None: + test_cwd = self.create_temp_dir() + with os_helper.temp_cwd(test_cwd, quiet=True): + self._main(tests, kwargs) + else: + # Worker process self._main(tests, kwargs) except SystemExit as exc: # bpo-38203: Python can hang at exit in Py_Finalize(), especially diff --git a/Lib/test/libregrtest/runtest_mp.py b/Lib/test/libregrtest/runtest_mp.py index a901b582f27dac..6c744a0f97795a 100644 --- a/Lib/test/libregrtest/runtest_mp.py +++ b/Lib/test/libregrtest/runtest_mp.py @@ -2,6 +2,7 @@ import json import os.path import queue +import random import shlex import signal import subprocess @@ -53,7 +54,8 @@ def parse_worker_args(worker_args) -> tuple[Namespace, str]: return (ns, test_name) -def run_test_in_subprocess(testname: str, ns: Namespace, tmp_dir: str) -> subprocess.Popen: +def run_test_in_subprocess(testname: str, ns: Namespace, + work_dir: str, tmp_dir: str) -> subprocess.Popen: ns_dict = vars(ns) worker_args = (ns_dict, testname) worker_args = json.dumps(worker_args) @@ -86,7 +88,7 @@ def run_test_in_subprocess(testname: str, ns: Namespace, tmp_dir: str) -> subpro stderr=subprocess.STDOUT, universal_newlines=True, close_fds=(os.name != 'nt'), - cwd=os_helper.SAVEDCWD, + cwd=work_dir, **kw) @@ -213,12 +215,12 @@ def mp_result_error( test_result.duration_sec = time.monotonic() - self.start_time return MultiprocessResult(test_result, stdout, err_msg) - def _run_process(self, test_name: str, tmp_dir: str) -> tuple[int, str, str]: + def _run_process(self, test_name: str, work_dir: str, tmp_dir: str) -> tuple[int, str, str]: self.start_time = time.monotonic() self.current_test_name = test_name try: - popen = run_test_in_subprocess(test_name, self.ns, tmp_dir) + popen = run_test_in_subprocess(test_name, self.ns, work_dir, tmp_dir) self._killed = False self._popen = popen @@ -272,22 +274,39 @@ def _run_process(self, test_name: str, tmp_dir: str) -> tuple[int, str, str]: self._popen = None self.current_test_name = None + def create_work_dir(self): + # Generate an unique directory name: 6 random decimal digits gives + # almost 20 bits of entropy. It's rare to run more than 100 processes + # in parallel. + nounce = random.randint(0, 1_000_000) + # create a non-ASCII name to detect issues with non-ASCII characters + path = f'test_python_worker_{nounce}_{os_helper.FS_NONASCII}' + return os.path.join(os.getcwd(), path) + def _runtest(self, test_name: str) -> MultiprocessResult: - if self.ns.use_mp == 1: - # gh-93353: Check for leaked temporary files in the parent process, - # since the deletion of temporary files can happen late during - # Python finalization: too late for libregrtest. - tmp_dir = os.getcwd() + '_tmpdir' - tmp_dir = os.path.abspath(tmp_dir) - try: + # The working directory of the worker process is a sub-directory of the + # main process. So when the main process exits, it removes also + # sub-directories of worker processes. + work_dir = self.create_work_dir() + tmp_dir = None + tmp_files = None + try: + os.mkdir(work_dir) + if not support.is_wasi: + tmp_dir = work_dir + '_tmpdir' os.mkdir(tmp_dir) - retcode, stdout = self._run_process(test_name, tmp_dir) - finally: + + retcode, stdout = self._run_process(test_name, work_dir, tmp_dir) + + if tmp_dir is not None: + # gh-93353: Check for leaked temporary files in the parent + # process, since the deletion of temporary files can happen + # late during Python finalization: too late for libregrtest. tmp_files = os.listdir(tmp_dir) + finally: + os_helper.rmtree(work_dir) + if tmp_dir is not None: os_helper.rmtree(tmp_dir) - else: - retcode, stdout = self._run_process(test_name, None) - tmp_files = () if retcode is None: return self.mp_result_error(Timeout(test_name), stdout) diff --git a/Lib/test/test_regrtest.py b/Lib/test/test_regrtest.py index 93c0cae1473a0c..a36d18488a5ef7 100644 --- a/Lib/test/test_regrtest.py +++ b/Lib/test/test_regrtest.py @@ -1357,6 +1357,8 @@ def test_cleanup(self): for name in names: self.assertFalse(os.path.exists(name), name) + @unittest.skipIf(support.is_wasi, + 'checking temp files is not implemented on WASI') def test_leak_tmp_file(self): code = textwrap.dedent(r""" import os.path @@ -1369,15 +1371,17 @@ def test_leak_tmp_file(self): with open(filename, "wb") as fp: fp.write(b'content') """) - testname = self.create_test(code=code) + testnames = [self.create_test(code=code) for _ in range(3)] - output = self.run_tests("--fail-env-changed", "-v", "-j1", testname, exitcode=3) - self.check_executed_tests(output, [testname], - env_changed=[testname], - fail_env_changed=True) - self.assertIn(f"Warning -- {testname} leaked temporary " - f"files (1): mytmpfile", - output) + output = self.run_tests("--fail-env-changed", "-v", "-j2", *testnames, exitcode=3) + self.check_executed_tests(output, testnames, + env_changed=testnames, + fail_env_changed=True, + randomize=True) + for testname in testnames: + self.assertIn(f"Warning -- {testname} leaked temporary " + f"files (1): mytmpfile", + output) class TestUtils(unittest.TestCase):