[framewqork] use subprocess timeouts

Since Python 3.3, the subprocess module has its own timeout
implementation, so remove the test framework's custom one.
Required a little rejigger since the subprocess timeout is done
on the communicate() call, not set up before the test is started.
Noted that the framework intends to support two levels:
one for the testing class instance, and one for an individual
start call, which should override the one in the instance.

Signed-off-by: Mats Wichmann <mats@linux.com>

4 files changed, 95 insertions, 70 deletions

diff --git a/testing/framework/TestCmd.py b/testing/framework/TestCmd.py
index aabd1dc02..812314db9 100644
--- a/testing/framework/TestCmd.py
+++ b/testing/framework/TestCmd.py
@@ -1014,8 +1014,7 @@ def _clean():
 
 
 class TestCmd:
-    """Class TestCmd
-    """
+    """Class TestCmd"""
 
     def __init__(
         self,
@@ -1049,7 +1048,10 @@ class TestCmd:
         self.combine = combine
         self.universal_newlines = universal_newlines
         self.process = None
-        self.set_timeout(timeout)
+        # Two layers of timeout: one at the test class instance level,
+        # one set on an individual start() call (usually via a run() call)
+        self.timeout = timeout
+        self.start_timeout = None
         self.set_match_function(match, match_stdout, match_stderr)
         self.set_diff_function(diff, diff_stdout, diff_stderr)
         self._dirlist = []
@@ -1374,14 +1376,6 @@ class TestCmd:
         dir = self.canonicalize(dir)
         os.rmdir(dir)
 
-    def _timeout(self):
-        self.process.terminate()
-        self.timer.cancel()
-        self.timer = None
-
-    def set_timeout(self, timeout):
-        self.timeout = timeout
-        self.timer = None
 
     def parse_path(self, path, suppress_current=False):
         """Return a list with the single path components of path."""
@@ -1508,7 +1502,7 @@ class TestCmd:
               interpreter=None,
               arguments=None,
               universal_newlines=None,
-              timeout=_Null,
+              timeout=None,
               **kw):
         """ Starts a program or script for the test environment.
 
@@ -1536,11 +1530,8 @@ class TestCmd:
         else:
             stderr_value = PIPE
 
-        if timeout is _Null:
-            timeout = self.timeout
         if timeout:
-            self.timer = threading.Timer(float(timeout), self._timeout)
-            self.timer.start()
+            self.start_timeout = timeout
 
         if sys.platform == 'win32':
             # Set this otherwist stdout/stderr pipes default to
@@ -1592,14 +1583,32 @@ class TestCmd:
         """
         if popen is None:
             popen = self.process
-        stdout, stderr = popen.communicate()
+        if self.start_timeout:
+            timeout = self.start_timeout
+            # we're using a timeout from start, now reset it to default
+            self.start_timeout = None
+        else:
+            timeout = self.timeout
+        try:
+            stdout, stderr = popen.communicate(timeout=timeout)
+        except subprocess.TimeoutExpired:
+            popen.terminate()
+            stdout, stderr = popen.communicate()
+
+        # this is instead of using Popen as a context manager:
+        if popen.stdout:
+            popen.stdout.close()
+        if popen.stderr:
+            popen.stderr.close()
+        try:
+            if popen.stdin:
+                popen.stdin.close()
+        finally:
+            popen.wait()
 
         stdout = self.fix_binary_stream(stdout)
         stderr = self.fix_binary_stream(stderr)
 
-        if self.timer:
-            self.timer.cancel()
-            self.timer = None
         self.status = popen.returncode
         self.process = None
         self._stdout.append(stdout or '')
@@ -1611,7 +1620,7 @@ class TestCmd:
             chdir=None,
             stdin=None,
             universal_newlines=None,
-            timeout=_Null):
+            timeout=None):
         """Runs a test of the program or script for the test environment.
 
         Output and error output are saved for future retrieval via
@@ -1639,6 +1648,8 @@ class TestCmd:
             if self.verbose:
                 sys.stderr.write("chdir(" + chdir + ")\n")
             os.chdir(chdir)
+        if not timeout:
+            timeout = self.timeout
         p = self.start(program=program,
                        interpreter=interpreter,
                        arguments=arguments,
@@ -1656,17 +1667,29 @@ class TestCmd:
         # subclasses that redefine .finish().  We could abstract this
         # into Yet Another common method called both here and by .finish(),
         # but that seems ill-thought-out.
-        stdout, stderr = p.communicate(input=stdin)
-        if self.timer:
-            self.timer.cancel()
-            self.timer = None
+        try:
+            stdout, stderr = p.communicate(input=stdin, timeout=timeout)
+        except subprocess.TimeoutExpired:
+            p.terminate()
+            stdout, stderr = p.communicate()
+        
+        # this is instead of using Popen as a context manager:
+        if p.stdout:
+            p.stdout.close()
+        if p.stderr:
+            p.stderr.close()
+        try:
+            if p.stdin:
+                p.stdin.close()
+        finally:
+            p.wait()
+       
         self.status = p.returncode
         self.process = None
 
         stdout = self.fix_binary_stream(stdout)
         stderr = self.fix_binary_stream(stderr)
 
-
         self._stdout.append(stdout or '')
         self._stderr.append(stderr or '')
 
@@ -1696,12 +1719,16 @@ class TestCmd:
         time.sleep(seconds)
 
     def stderr(self, run=None) -> Optional[str]:
-        """Returns the error output from the specified run number.
+        """Returns the stored standard error output from a given run.
 
-        If there is no specified run number, then returns the error
-        output of the last run.  If the run number is less than zero,
-        then returns the error output from that many runs back from the
-        current run.
+        Args:
+            run: run number to select.  If run number is omitted,
+                return the standard error of the most recent run.
+                If negative, use as a relative offset, e.g. -2
+                means the run two prior to the most recent.
+
+        Returns:
+            selected sterr string or None if there are no stored runs.
         """
         if not run:
             run = len(self._stderr)
@@ -1718,13 +1745,12 @@ class TestCmd:
 
         Args:
             run: run number to select.  If run number is omitted,
-            return the standard output of the most recent run.
-            If negative, use as a relative offset, so that -2
-            means the run two prior to the most recent.
+                return the standard output of the most recent run.
+                If negative, use as a relative offset, e.g. -2
+                means the run two prior to the most recent.
 
         Returns:
-            selected stdout string or None if there are no
-            stored runs.
+            selected stdout string or None if there are no stored runs.
         """
         if not run:
             run = len(self._stdout)
diff --git a/testing/framework/TestCmdTests.py b/testing/framework/TestCmdTests.py
index 6f2dac5ad..6a48c981a 100644
--- a/testing/framework/TestCmdTests.py
+++ b/testing/framework/TestCmdTests.py
@@ -2848,7 +2848,7 @@ sys.exit(0)
 
 class timeout_TestCase(TestCmdTestCase):
     def test_initialization(self):
-        """Test initialization timeout"""
+        """Test initializating a TestCmd with a timeout"""
         test = TestCmd.TestCmd(workdir='', timeout=2)
         test.write('sleep.py', timeout_script)
 
@@ -2882,40 +2882,39 @@ class timeout_TestCase(TestCmdTestCase):
         test = TestCmd.TestCmd(workdir='', timeout=8)
         test.write('sleep.py', timeout_script)
 
-        test.run([sys.executable, test.workpath('sleep.py'), '2'],
-                 timeout=4)
+        test.run([sys.executable, test.workpath('sleep.py'), '2'], timeout=4)
         assert test.stderr() == '', test.stderr()
         assert test.stdout() == 'sleeping 2\nslept 2\n', test.stdout()
 
-        test.run([sys.executable, test.workpath('sleep.py'), '6'],
-                 timeout=4)
+        test.run([sys.executable, test.workpath('sleep.py'), '6'], timeout=4)
         assert test.stderr() == '', test.stderr()
         assert test.stdout() == 'sleeping 6\n', test.stdout()
 
-    def test_set_timeout(self):
-        """Test set_timeout()"""
-        test = TestCmd.TestCmd(workdir='', timeout=2)
-        test.write('sleep.py', timeout_script)
-
-        test.run([sys.executable, test.workpath('sleep.py'), '4'])
-        assert test.stderr() == '', test.stderr()
-        assert test.stdout() == 'sleeping 4\n', test.stdout()
-
-        test.set_timeout(None)
-
-        test.run([sys.executable, test.workpath('sleep.py'), '4'])
-        assert test.stderr() == '', test.stderr()
-        assert test.stdout() == 'sleeping 4\nslept 4\n', test.stdout()
-
-        test.set_timeout(6)
-
-        test.run([sys.executable, test.workpath('sleep.py'), '4'])
-        assert test.stderr() == '', test.stderr()
-        assert test.stdout() == 'sleeping 4\nslept 4\n', test.stdout()
-
-        test.run([sys.executable, test.workpath('sleep.py'), '8'])
-        assert test.stderr() == '', test.stderr()
-        assert test.stdout() == 'sleeping 8\n', test.stdout()
+    # This method has been removed
+    #def test_set_timeout(self):
+    #    """Test set_timeout()"""
+    #    test = TestCmd.TestCmd(workdir='', timeout=2)
+    #    test.write('sleep.py', timeout_script)
+    #
+    #    test.run([sys.executable, test.workpath('sleep.py'), '4'])
+    #    assert test.stderr() == '', test.stderr()
+    #    assert test.stdout() == 'sleeping 4\n', test.stdout()
+    #
+    #    test.set_timeout(None)
+    #
+    #    test.run([sys.executable, test.workpath('sleep.py'), '4'])
+    #    assert test.stderr() == '', test.stderr()
+    #    assert test.stdout() == 'sleeping 4\nslept 4\n', test.stdout()
+    #
+    #    test.set_timeout(6)
+    #
+    #    test.run([sys.executable, test.workpath('sleep.py'), '4'])
+    #    assert test.stderr() == '', test.stderr()
+    #    assert test.stdout() == 'sleeping 4\nslept 4\n', test.stdout()
+    #
+    #    test.run([sys.executable, test.workpath('sleep.py'), '8'])
+    #    assert test.stderr() == '', test.stderr()
+    #    assert test.stdout() == 'sleeping 8\n', test.stdout()
 
 
 
diff --git a/testing/framework/TestCommon.py b/testing/framework/TestCommon.py
index e8045ca96..f45b856e3 100644
--- a/testing/framework/TestCommon.py
+++ b/testing/framework/TestCommon.py
@@ -676,7 +676,7 @@ class TestCommon(TestCmd):
         """
         arguments = self.options_arguments(options, arguments)
         try:
-            return TestCmd.start(self, program, interpreter, arguments,
+            return super().start(program, interpreter, arguments,
                                  universal_newlines, **kw)
         except KeyboardInterrupt:
             raise
@@ -713,7 +713,7 @@ class TestCommon(TestCmd):
                         command.  A value of None means don't
                         test exit status.
         """
-        TestCmd.finish(self, popen, **kw)
+        super().finish(popen, **kw)
         match = kw.get('match', self.match)
         self._complete(self.stdout(), stdout,
                        self.stderr(), stderr, status, match)
@@ -750,7 +750,7 @@ class TestCommon(TestCmd):
             del kw['match']
         except KeyError:
             match = self.match
-        TestCmd.run(self, **kw)
+        super().run(**kw)
         self._complete(self.stdout(), stdout,
                        self.stderr(), stderr, status, match)
 
diff --git a/testing/framework/TestSCons.py b/testing/framework/TestSCons.py
index ca67092e3..18b91932e 100644
--- a/testing/framework/TestSCons.py
+++ b/testing/framework/TestSCons.py
@@ -461,7 +461,7 @@ class TestSCons(TestCommon):
         """
         sconsflags = initialize_sconsflags(self.ignore_python_version)
         try:
-            TestCommon.run(self, *args, **kw)
+            super().run(*args, **kw)
         finally:
             restore_sconsflags(sconsflags)
 
@@ -1636,7 +1636,7 @@ else:
             kw['stdin'] = True
         sconsflags = initialize_sconsflags(self.ignore_python_version)
         try:
-            p = TestCommon.start(self, *args, **kw)
+            p = super().start(*args, **kw)
         finally:
             restore_sconsflags(sconsflags)
         return p