From 198027a8fa4c053a06935d7e2110186b9b655f26 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Sat, 25 Jun 2022 09:58:22 +0200 Subject: [PATCH 1/3] gh-94026: Buffer regrtest worker stdout in temporary file Co-authored-by: Victor Stinner --- Lib/test/libregrtest/runtest_mp.py | 74 +++++++++++++++--------------- 1 file changed, 36 insertions(+), 38 deletions(-) diff --git a/Lib/test/libregrtest/runtest_mp.py b/Lib/test/libregrtest/runtest_mp.py index 6ebabb86873bcb..b7113ca89388b6 100644 --- a/Lib/test/libregrtest/runtest_mp.py +++ b/Lib/test/libregrtest/runtest_mp.py @@ -9,7 +9,7 @@ import threading import time import traceback -from typing import NamedTuple, NoReturn, Literal, Any +from typing import NamedTuple, NoReturn, Literal, Any, TextIO from test import support from test.support import os_helper @@ -53,7 +53,7 @@ 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, tmp_dir: str, stdout_fh: TextIO) -> subprocess.Popen: ns_dict = vars(ns) worker_args = (ns_dict, testname) worker_args = json.dumps(worker_args) @@ -75,18 +75,18 @@ def run_test_in_subprocess(testname: str, ns: Namespace, tmp_dir: str) -> subpro # 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. - kw = {'env': env} + kw = dict( + env=env, + stdout=stdout_fh, + # bpo-45410: Write stderr into stdout to keep messages order + stderr=stdout_fh, + text=True, + close_fds=(os.name != 'nt'), + cwd=os_helper.SAVEDCWD, + ) if USE_PROCESS_GROUP: kw['start_new_session'] = True - return subprocess.Popen(cmd, - stdout=subprocess.PIPE, - # bpo-45410: Write stderr into stdout to keep - # messages order - stderr=subprocess.STDOUT, - universal_newlines=True, - close_fds=(os.name != 'nt'), - cwd=os_helper.SAVEDCWD, - **kw) + return subprocess.Popen(cmd, **kw) def run_tests_worker(ns: Namespace, test_name: str) -> NoReturn: @@ -212,12 +212,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, tmp_dir: str, stdout_fh: TextIO) -> int: 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, tmp_dir, stdout_fh) self._killed = False self._popen = popen @@ -234,10 +234,11 @@ def _run_process(self, test_name: str, tmp_dir: str) -> tuple[int, str, str]: raise ExitThread try: - # bpo-45410: stderr is written into stdout - stdout, _ = popen.communicate(timeout=self.timeout) + # gh-94026: stdout+stderr are written to tempfile + popen.communicate(timeout=self.timeout) retcode = popen.returncode assert retcode is not None + return retcode except subprocess.TimeoutExpired: if self._stopped: # kill() has been called: communicate() fails on reading @@ -252,17 +253,12 @@ def _run_process(self, test_name: str, tmp_dir: str) -> tuple[int, str, str]: # bpo-38207: Don't attempt to call communicate() again: on it # can hang until all child processes using stdout # pipes completes. - stdout = '' except OSError: if self._stopped: # kill() has been called: communicate() fails # on reading closed stdout raise ExitThread raise - else: - stdout = stdout.strip() - - return (retcode, stdout) except: self._kill() raise @@ -272,23 +268,28 @@ def _run_process(self, test_name: str, tmp_dir: str) -> tuple[int, str, str]: self.current_test_name = None def _runtest(self, test_name: str) -> MultiprocessResult: - # Don't check for leaked temporary files and directories if Python is - # run on WASI. WASI don't pass environment variables like TMPDIR to - # worker processes. - if not support.is_wasi: + # gh-94026: Write stdout+stderr to a tempfile as workaround for + # non-blocking pipes on Emscripten with NodeJS. + with tempfile.TemporaryFile('w+') as stdout_fh: # 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 = tempfile.mkdtemp(prefix="test_python_") - tmp_dir = os.path.abspath(tmp_dir) - try: - retcode, stdout = self._run_process(test_name, tmp_dir) - finally: - tmp_files = os.listdir(tmp_dir) - os_helper.rmtree(tmp_dir) - else: - retcode, stdout = self._run_process(test_name, None) - tmp_files = () + if not support.is_wasi: + # Don't check for leaked temporary files and directories if Python is + # run on WASI. WASI don't pass environment variables like TMPDIR to + # worker processes. + tmp_dir = tempfile.mkdtemp(prefix="test_python_") + tmp_dir = os.path.abspath(tmp_dir) + try: + retcode = self._run_process(test_name, tmp_dir, stdout_fh) + finally: + tmp_files = os.listdir(tmp_dir) + os_helper.rmtree(tmp_dir) + else: + retcode = self._run_process(test_name, None, stdout_fh) + tmp_files = () + stdout_fh.seek(0) + stdout = stdout_fh.read().strip() if retcode is None: return self.mp_result_error(Timeout(test_name), stdout) @@ -343,9 +344,6 @@ def run(self) -> None: def _wait_completed(self) -> None: popen = self._popen - # stdout must be closed to ensure that communicate() does not hang - popen.stdout.close() - try: popen.wait(JOIN_TIMEOUT) except (subprocess.TimeoutExpired, OSError) as exc: From 141d491edd5def7866fba448fd6c29d17cad8c12 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Wed, 29 Jun 2022 09:04:57 +0200 Subject: [PATCH 2/3] Address review --- Lib/test/libregrtest/runtest_mp.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Lib/test/libregrtest/runtest_mp.py b/Lib/test/libregrtest/runtest_mp.py index b7113ca89388b6..b139a64cc0587d 100644 --- a/Lib/test/libregrtest/runtest_mp.py +++ b/Lib/test/libregrtest/runtest_mp.py @@ -235,7 +235,7 @@ def _run_process(self, test_name: str, tmp_dir: str, stdout_fh: TextIO) -> int: try: # gh-94026: stdout+stderr are written to tempfile - popen.communicate(timeout=self.timeout) + popen.wait(timeout=self.timeout) retcode = popen.returncode assert retcode is not None return retcode @@ -270,7 +270,9 @@ def _run_process(self, test_name: str, tmp_dir: str, stdout_fh: TextIO) -> int: def _runtest(self, test_name: str) -> MultiprocessResult: # gh-94026: Write stdout+stderr to a tempfile as workaround for # non-blocking pipes on Emscripten with NodeJS. - with tempfile.TemporaryFile('w+') as stdout_fh: + with tempfile.TemporaryFile( + 'w+', encoding=sys.stdout.encoding + ) as stdout_fh: # 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. From bd760602ce534d95374a2bd2ef6fb57a4913866b Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Wed, 29 Jun 2022 09:40:43 +0200 Subject: [PATCH 3/3] get retcode directly --- Lib/test/libregrtest/runtest_mp.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/libregrtest/runtest_mp.py b/Lib/test/libregrtest/runtest_mp.py index b139a64cc0587d..a4d3e5c3146a8c 100644 --- a/Lib/test/libregrtest/runtest_mp.py +++ b/Lib/test/libregrtest/runtest_mp.py @@ -235,8 +235,7 @@ def _run_process(self, test_name: str, tmp_dir: str, stdout_fh: TextIO) -> int: try: # gh-94026: stdout+stderr are written to tempfile - popen.wait(timeout=self.timeout) - retcode = popen.returncode + retcode = popen.wait(timeout=self.timeout) assert retcode is not None return retcode except subprocess.TimeoutExpired: