[Piglit] [PATCH v2 6/6] framework/test/base.py: use subprocess32 for timeouts.
Thomas Wood
thomas.wood at intel.com
Mon Oct 26 09:08:44 PDT 2015
On 24 October 2015 at 02:17, <baker.dylan.c at gmail.com> wrote:
> From: Dylan Baker <baker.dylan.c at gmail.com>
>
> Subprocess32 provides a backport of (ironically) python 3.3's subprocess
> module, which has a timeout parameter. When the timeout runs out then an
> exception is raised, and when that exception is caught we can kill the
> process.
>
> This is fairly similar to the way the current timeout mechanism works,
> except that the current mechanism is not thread safe. Since one of the
> major features of piglit is that it offer's processes isolated
> concurrency of tests, it makes sense to make the effort to provide a
> timeout mechanism that can be used outside of IGT, which is a
> non-concurrent test suite.
>
> This patch look pretty substantial. It's not as big as it looks, since
> it adds some fairly big tests.
> ---
> framework/test/base.py | 127 +++++++++++-------------------------------
> framework/tests/base_tests.py | 99 ++++++++++++++++++++++++++++++--
> tox.ini | 7 ++-
> 3 files changed, 131 insertions(+), 102 deletions(-)
>
> diff --git a/framework/test/base.py b/framework/test/base.py
> index 4312cbf..03cff4f 100644
> --- a/framework/test/base.py
> +++ b/framework/test/base.py
> @@ -25,16 +25,18 @@
> from __future__ import print_function, absolute_import
> import errno
> import os
> -import subprocess
> import time
> import sys
> import traceback
> -from datetime import datetime
> -import threading
> -import signal
> import itertools
> import abc
> import copy
> +import signal
> +
> +try:
> + import subprocess32 as subprocess
> +except ImportError:
> + import subprocess
>
> from framework import exceptions
> from framework.core import Options
> @@ -45,6 +47,7 @@ __all__ = [
> 'Test',
> 'TestIsSkip',
> 'TestRunError',
> + 'TimeoutMixin',
> 'ValgrindMixin',
> 'WindowResizeMixin',
> ]
> @@ -107,7 +110,6 @@ class Test(object):
> self.env = {}
> self.result = TestResult()
> self.cwd = None
> - self.__proc_timeout = None
>
> def execute(self, path, log, dmesg):
> """ Run a test
> @@ -250,6 +252,9 @@ class Test(object):
> raise TestRunError("Test executable not found.\n", 'skip')
> else:
> raise e
> + except Exception as e:
> + e.proc = proc
> + raise e
>
> # The setter handles the bytes/unicode conversion
> self.result.out = out
> @@ -332,57 +337,7 @@ class ValgrindMixin(object):
> self.result.result = 'fail'
>
>
> -if not sys.platform.startswith('win'):
> - class ProcessTimeout(threading.Thread):
> - """ Timeout class for test processes
> -
> - This class is for terminating tests that run for longer than a certain
> - amount of time. Create an instance by passing it a timeout in seconds
> - and the Popen object that is running your test. Then call the start
> - method (inherited from Thread) to start the timer. The Popen object is
> - killed if the timeout is reached and it has not completed. Wait for the
> - outcome by calling the join() method from the parent.
> -
> - """
> -
> - def __init__(self, timeout, proc):
> - threading.Thread.__init__(self)
> - self.proc = proc
> - self.timeout = timeout
> - self.status = 0
> -
> - def run(self):
> - start_time = datetime.now()
> - delta = 0
> -
> - # poll() returns the returncode attribute, which is either the return
> - # code of the child process (which could be zero), or None if the
> - # process has not yet terminated.
> -
> - while delta < self.timeout and self.proc.poll() is None:
> - time.sleep(1)
> - delta = (datetime.now() - start_time).total_seconds()
> -
> - # if the test is not finished after timeout, first try to terminate it
> - # and if that fails, send SIGKILL to all processes in the test's
> - # process group
> -
> - if self.proc.poll() is None:
> - self.status = 1
> - self.proc.terminate()
> - time.sleep(5)
> -
> - if self.proc.poll() is None:
> - self.status = 2
> - if hasattr(os, 'killpg'):
> - os.killpg(self.proc.pid, signal.SIGKILL)
> - self.proc.wait()
> -
> - def join(self):
> - threading.Thread.join(self)
> - return self.status
> -
> -
> +if not sys.platform.startswith('win') and subprocess.__name__ == 'subprocess32':
> class TimeoutMixin(object): # pylint: disable=too-few-public-methods
> """Mixin poviding timeout on *nix platforms
>
> @@ -399,12 +354,6 @@ if not sys.platform.startswith('win'):
> """
> timeout = 0
>
> - def __init__(self, *args, **kwargs):
> - super(TimeoutMixin, self).__init__(*args, **kwargs)
> - self.__proc_timeout = None
> -
> - # This level of duplication sucks, but there doesn't seem to be a good
> - # way around it without introducing even worse hacks.
> def _run_command(self, communicate_args=None):
> """Run the test command and get the result
>
> @@ -415,43 +364,29 @@ if not sys.platform.startswith('win'):
> stderr, cwd, universal newlines.
>
> """
> + args = communicate_args or {}
> + args['timeout'] = self.timeout
> +
> try:
> - proc = subprocess.Popen(self.command,
> - **self._make_popen_args())
> - # create a ProcessTimeout object to watch out for test hang if
> - # the process is still going after the timeout, then it will be
> - # killed forcing the communicate function (which is a blocking
> - # call) to return
> - if self.timeout > 0:
> - self.__proc_timeout = ProcessTimeout(self.timeout, proc)
> - self.__proc_timeout.start()
> -
> - out, err = proc.communicate(**(communicate_args or {}))
> - returncode = proc.returncode
> - except OSError as e:
> - # Different sets of tests get built under different build
> - # configurations. If a developer chooses to not build a test,
> - # Piglit should not report that test as having failed.
> - if e.errno == errno.ENOENT:
> - raise TestRunError("Test executable not found.\n", 'skip')
> - else:
> - raise e
> -
> - # The setter handles the bytes/unicode conversion
> - self.result.out = out
> - self.result.err = err
> - self.result.returncode = returncode
> + super(TimeoutMixin, self)._run_command(communicate_args=args)
> + except subprocess.TimeoutExpired as e: # pylint: disable=no-member
> + e.proc.terminate()
It may take a few seconds for a test to terminate cleanly, so it would
be best to wait before checking if the process still exists after
sending the terminate signal.
> +
> + if e.proc.poll() is None:
> + time.sleep(3)
Should be moved as above?
> + os.killpg(os.getsid(e.proc.pid), signal.SIGKILL)
Although the session id is the same as the group id for the session
leader and there isn't expected to be more that one process group in
this session, it may be safer and more obvious to use os.getpgid here
instead.
> +
> + self.result.out, self.result.err = e.proc.communicate()
> +
> + raise TestRunError(
> + 'Test run time exceeded timeout out {}\n'.format(self.timeout),
> + 'timeout')
>
> def _make_popen_args(self):
> popen_args = super(TimeoutMixin, self)._make_popen_args()
> - popen_args['preexec_fn'] = os.setpgrp
> + popen_args['start_new_session'] = True
Was creating a new session instead of just a process group intentional here?
>
> return popen_args
> -
> - def interpret_result(self):
> - if (_is_crash_returncode(self.result.returncode) and
> - self.timeout > 0 and self.__proc_timeout.join() > 0):
> - self.result.result = 'timeout'
> - else:
> - # Only call super if we don't set the result to timeout.
> - super(TimeoutMixin, self).interpret_result()
> +else:
> + class TimeoutMixin(object):
> + pass
Could the fallback class warn that the expected timeout functionality
is not available?
> diff --git a/framework/tests/base_tests.py b/framework/tests/base_tests.py
> index 6e5a734..08bb0d0 100644
> --- a/framework/tests/base_tests.py
> +++ b/framework/tests/base_tests.py
> @@ -21,10 +21,19 @@
> """ Tests for the exectest module """
>
> from __future__ import print_function, absolute_import
> +import tempfile
> +import textwrap
> +import os
>
> +import mock
> import nose.tools as nt
> from nose.plugins.attrib import attr
>
> +try:
> + import psutil
> +except ImportError:
> + pass
> +
> import framework.tests.utils as utils
> from framework.test.base import (
> Test, WindowResizeMixin, ValgrindMixin, TimeoutMixin, TestRunError
> @@ -61,10 +70,92 @@ def test_run_return_early():
>
>
> @attr('slow')
> + at nt.timed(5)
> +def test_timeout_kill_children():
> + """test.base.TimeoutMixin: kill children if terminate fails
> +
> + This creates a process that forks multiple times, and then checks that the
> + children have been killed.
> +
> + This test could leave processes running if it fails.
> +
> + """
> + utils.module_check('subprocess32')
> + utils.module_check('psutil')
> +
> + class Mixin(object):
> + def __init__(self, *args, **kwargs):
> + super(Mixin, self).__init__(*args, **kwargs)
> + self.proc = None
> +
> + def _run_command(self, *args, **kwargs):
> + try:
> + super(Mixin, self)._run_command(*args, **kwargs)
> + except Exception as e:
> + self.proc = e.proc
> +
> + # This test is aimed at testing the os.killpg functionality
> + e.proc.terminate = mock.Mock()
> +
> + # Communicating will cause the pid to be released, and this
> + # will break the test
> + e.proc.communicate = mock.Mock(return_value=('out', 'err'))
> + raise e
> +
> + class _Test(TimeoutMixin, Mixin, utils.Test):
> + timeout = 1
> +
> + with tempfile.NamedTemporaryFile() as f:
> + # Create a process with two subproccesses (not threads) that will run
> + # for a long time.
> + f.write(textwrap.dedent("""\
> + import time
> + from multiprocessing import Process
> +
> + def p():
> + for _ in range(100):
> + time.sleep(1)
> +
> + a = Process(target=p)
> + b = Process(target=p)
> + a.start()
> + b.start()
> + a.join()
> + b.join()
> + """))
> + f.seek(0) # we'll need to read the file back
> +
> + test = _Test(['python', f.name])
> + test.run()
> +
> + proc = psutil.Process(os.getsid(test.proc.pid))
> + nt.ok_(not proc.children(),
> + msg='Test process had children when it should not have')
> +
> +
> + at nt.timed(5)
> def test_timeout():
> - """test.base.Test.run(): Sets status to 'timeout' when timeout exceeded"""
> + """test.base.TimeoutMixin: Stops running test after timeout expires
> +
> + This is a little bit of extra time here, but without a sleep of 60 seconds
> + if the test runs 5 seconds it's run too long
> +
> + """
> + utils.module_check('subprocess32')
> utils.binary_check('sleep', 1)
> - class _Test(TimeoutMixin, Test):
> + class _Test(TimeoutMixin, TestTest):
> + pass
> +
> + test = _Test(['sleep', '60'])
> + test.timeout = 1
> + test.run()
> +
> +
> +def test_timeout_timeout():
> + """test.base.TimeoutMixin: Sets status to 'timeout' when timeout exceeded"""
> + utils.module_check('subprocess32')
> + utils.binary_check('sleep', 1)
> + class _Test(TimeoutMixin, TestTest):
> pass
>
> test = _Test(['sleep', '60'])
> @@ -73,10 +164,10 @@ def test_timeout():
> nt.eq_(test.result.result, 'timeout')
>
>
> - at attr('slow')
> def test_timeout_pass():
> - """test.base.Test.run(): Doesn't change status when timeout not exceeded
> + """test.base.TimeoutMixin: Doesn't change status when timeout not exceeded
> """
> + utils.module_check('subprocess32')
> utils.binary_check('true')
> class _Test(TimeoutMixin, Test):
> def interpret_result(self):
> diff --git a/tox.ini b/tox.ini
> index fe58771..dbb3ddf 100644
> --- a/tox.ini
> +++ b/tox.ini
> @@ -13,17 +13,20 @@ commands = nosetests generated_tests/test_generators.py
> [testenv:py27-noaccel]
> passenv=HOME
> deps =
> + coverage
> mako
> + mock
> nose
> - coverage
> commands = nosetests framework/tests --attr="!privileged" --with-cover --cover-package=framework --cover-tests
>
> [testenv:py27-accel]
> passenv=HOME
> deps =
> + coverage
> mako
> + mock
> nose
> - coverage
> + psutil
> simplejson
> lxml
> backports.lzma
> --
> 2.6.1
>
More information about the Piglit
mailing list