Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-93353: regrtest creates worker directory in the parent #93817

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 17 additions & 20 deletions Lib/test/libregrtest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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

Expand All @@ -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
Expand Down
51 changes: 35 additions & 16 deletions Lib/test/libregrtest/runtest_mp.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import json
import os.path
import queue
import random
import shlex
import signal
import subprocess
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)


Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
20 changes: 12 additions & 8 deletions Lib/test/test_regrtest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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):
Expand Down