bpo-36915: regrtest always remove tempdir of worker processes (GH-13312)

miss-islington · vstinner · web-flow · commit ecd668d6d99f · 2019-05-14T08:25:06.000-07:00
When using multiprocessing (-jN option), worker processes now create their temporary directory inside the temporary directory of the main process. So the main process is able to remove temporary directories of worker processes even if they crash or when they are killed by regrtest on KeyboardInterrupt (CTRL+c). Rework also how multiprocessing arguments are parsed in main.py. (cherry picked from commit 3c93153) Co-authored-by: Victor Stinner <vstinner@redhat.com>
diff --git a/Lib/test/libregrtest/main.py b/Lib/test/libregrtest/main.py
@@ -1,6 +1,5 @@
 import datetime
 import faulthandler
-import json
 import locale
 import os
 import platform
@@ -22,22 +21,6 @@
 from test import support
 
 
-# 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():
-    TEMPDIR = sysconfig.get_config_var('abs_builddir')
-    if TEMPDIR is None:
-        # bpo-30284: On Windows, only srcdir is available. Using abs_builddir
-        # mostly matters on UNIX when building Python out of the source tree,
-        # especially when the source tree is read only.
-        TEMPDIR = sysconfig.get_config_var('srcdir')
-    TEMPDIR = os.path.join(TEMPDIR, 'build')
-else:
-    TEMPDIR = tempfile.gettempdir()
-TEMPDIR = os.path.abspath(TEMPDIR)
-
-
 class Regrtest:
     """Execute a test suite.
 
@@ -98,7 +81,10 @@ def __init__(self):
         # used by --junit-xml
         self.testsuite_xml = None
 
+        # misc
         self.win_load_tracker = None
+        self.tmp_dir = None
+        self.worker_test_name = None
 
     def get_executed(self):
         return (set(self.good) | set(self.bad) | set(self.skipped)
@@ -177,6 +163,13 @@ def parse_args(self, kwargs):
         if ns.xmlpath:
             support.junit_xml_list = self.testsuite_xml = []
 
+        worker_args = ns.worker_args
+        if worker_args is not None:
+            from test.libregrtest.runtest_mp import parse_worker_args
+            ns, test_name = parse_worker_args(ns.worker_args)
+            ns.worker_args = worker_args
+            self.worker_test_name = test_name
+
         # Strip .py extensions.
         removepy(ns.args)
 
@@ -186,7 +179,7 @@ def find_tests(self, tests):
         self.tests = tests
 
         if self.ns.single:
-            self.next_single_filename = os.path.join(TEMPDIR, 'pynexttest')
+            self.next_single_filename = os.path.join(self.tmp_dir, 'pynexttest')
             try:
                 with open(self.next_single_filename, 'r') as fp:
                     next_test = fp.read().strip()
@@ -544,29 +537,54 @@ def save_xml_result(self):
             for s in ET.tostringlist(root):
                 f.write(s)
 
-    def main(self, tests=None, **kwargs):
-        global TEMPDIR
-        self.ns = self.parse_args(kwargs)
-
+    def create_temp_dir(self):
         if self.ns.tempdir:
-            TEMPDIR = self.ns.tempdir
-        elif self.ns.worker_args:
-            ns_dict, _ = json.loads(self.ns.worker_args)
-            TEMPDIR = ns_dict.get("tempdir") or TEMPDIR
+            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.
+            if sysconfig.is_python_build():
+                self.tmp_dir = sysconfig.get_config_var('abs_builddir')
+                if self.tmp_dir is None:
+                    # bpo-30284: On Windows, only srcdir is available. Using
+                    # abs_builddir mostly matters on UNIX when building Python
+                    # out of the source tree, especially when the source tree
+                    # is read only.
+                    self.tmp_dir = sysconfig.get_config_var('srcdir')
+                self.tmp_dir = os.path.join(self.tmp_dir, 'build')
+            else:
+                self.tmp_dir = tempfile.gettempdir()
 
-        os.makedirs(TEMPDIR, exist_ok=True)
+        self.tmp_dir = os.path.abspath(self.tmp_dir)
+        os.makedirs(self.tmp_dir, exist_ok=True)
 
         # Define a writable temp dir that will be used as cwd while running
         # the tests. The name of the dir includes the pid to allow parallel
         # testing (see the -j option).
-        test_cwd = 'test_python_{}'.format(os.getpid())
-        test_cwd = os.path.join(TEMPDIR, test_cwd)
+        pid = os.getpid()
+        if self.worker_test_name is not None:
+            test_cwd = 'worker_{}'.format(pid)
+        else:
+            test_cwd = 'test_python_{}'.format(pid)
+        test_cwd = os.path.join(self.tmp_dir, test_cwd)
+        return test_cwd
+
+    def main(self, tests=None, **kwargs):
+        self.ns = self.parse_args(kwargs)
+
+        test_cwd = self.create_temp_dir()
 
-        # 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 support.SAVEDCWD.
+        # 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 support.SAVEDCWD.
         with support.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
             self._main(tests, kwargs)
 
     def getloadavg(self):
@@ -588,9 +606,9 @@ def _main(self, tests, kwargs):
                 print(msg, file=sys.stderr, flush=True)
                 sys.exit(2)
 
-        if self.ns.worker_args is not None:
+        if self.worker_test_name is not None:
             from test.libregrtest.runtest_mp import run_tests_worker
-            run_tests_worker(self.ns.worker_args)
+            run_tests_worker(self.ns, self.worker_test_name)
 
         if self.ns.wait:
             input("Press any key to continue...")
@@ -611,7 +629,7 @@ def _main(self, tests, kwargs):
 
         # If we're on windows and this is the parent runner (not a worker),
         # track the load average.
-        if sys.platform == 'win32' and (self.ns.worker_args is None):
+        if sys.platform == 'win32' and self.worker_test_name is None:
             from test.libregrtest.win_utils import WindowsLoadTracker
 
             try:
diff --git a/Lib/test/libregrtest/runtest_mp.py b/Lib/test/libregrtest/runtest_mp.py
@@ -33,6 +33,12 @@ def must_stop(result, ns):
     return False
 
 
+def parse_worker_args(worker_args):
+    ns_dict, test_name = json.loads(worker_args)
+    ns = types.SimpleNamespace(**ns_dict)
+    return (ns, test_name)
+
+
 def run_test_in_subprocess(testname, ns):
     ns_dict = vars(ns)
     worker_args = (ns_dict, testname)
@@ -42,8 +48,6 @@ def run_test_in_subprocess(testname, ns):
            '-u',    # Unbuffered stdout and stderr
            '-m', 'test.regrtest',
            '--worker-args', worker_args]
-    if ns.pgo:
-        cmd += ['--pgo']
 
     # Running the child from the same working directory as regrtest's original
     # invocation ensures that TEMPDIR for the child is the same when
@@ -56,15 +60,15 @@ def run_test_in_subprocess(testname, ns):
                             cwd=support.SAVEDCWD)
 
 
-def run_tests_worker(worker_args):
-    ns_dict, testname = json.loads(worker_args)
-    ns = types.SimpleNamespace(**ns_dict)
-
+def run_tests_worker(ns, test_name):
     setup_tests(ns)
 
-    result = runtest(ns, testname)
+    result = runtest(ns, test_name)
+
     print()   # Force a newline (just in case)
-    print(json.dumps(result), flush=True)
+
+    # Serialize TestResult as list in JSON
+    print(json.dumps(list(result)), flush=True)
     sys.exit(0)
 
 
diff --git a/Misc/NEWS.d/next/Tests/2019-05-14-14-12-24.bpo-36915.58b7pH.rst b/Misc/NEWS.d/next/Tests/2019-05-14-14-12-24.bpo-36915.58b7pH.rst
@@ -0,0 +1,3 @@
+The main regrtest process now always removes all temporary directories of
+worker processes even if they crash or if they are killed on
+KeyboardInterrupt (CTRL+c).

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+The main regrtest process now always removes all temporary directories of`
	`2`	`+worker processes even if they crash or if they are killed on`
	`3`	`+KeyboardInterrupt (CTRL+c).`