[Piglit] [PATCH 2/2] framework/test/base.py: try to use subprocess32
Thomas Wood
thomas.wood at intel.com
Thu Oct 15 03:45:57 PDT 2015
On 14 October 2015 at 22:35, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> On Wed, Oct 14, 2015 at 04:27:56PM +0100, Thomas Wood wrote:
>> On 9 October 2015 at 20:53, <baker.dylan.c at gmail.com> wrote:
>> > From: Dylan Baker <baker.dylan.c at gmail.com>
>> >
>> > This adds subprocess32 as an optional dependency to get timeouts. This
>> > module is a backport of the feature from python 3.2 that adds the
>> > timeout argument to Popen.communicate (among others).
>> >
>> > There is a caveat to this feature. Unlike the python 3.x version, this
>> > doesn't work on windows, although that isn't a change from the current
>> > state of affairs, as the current timeouts don't work on windows
>> >
>> > What this does do though, is get us timeouts that work with concurrency
>> > on posix systems (like Linux and OSX).
>> >
>> > Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
>> > ---
>> > README | 5 +-
>> > framework/test/base.py | 123 +++++++++++++-----------------------------
>> > framework/tests/base_tests.py | 2 +
>> > tox.ini | 1 +
>> > 4 files changed, 43 insertions(+), 88 deletions(-)
>> >
>> > diff --git a/README b/README
>> > index be8f4df..5813af5 100644
>> > --- a/README
>> > +++ b/README
>> > @@ -47,9 +47,12 @@ Optionally, you can install the following:
>> > - lxml. An accelerated python xml library using libxml2 (http://lxml.de/)
>> > - simplejson. A fast C based implementation of the python json library.
>> > (https://simplejson.readthedocs.org/en/latest/)
>> > - - backports.lzma. A packport of python3's lzma module to python2,
>> > + - backports.lzma. A backport of python3's lzma module to python2,
>> > this enables fast native xz (de)compression in piglit for results files
>> > (https://github.com/peterjc/backports.lzma)
>> > + - subprocess32. A backport of python 3.2's subprocess module to python 2.
>> > + This provides the timeout mechanism.
>> > + (https://pypi.python.org/pypi/subprocess32/)
>> >
>> > Now configure the build system:
>> >
>> > diff --git a/framework/test/base.py b/framework/test/base.py
>> > index bf00396..0f01ccc 100644
>> > --- a/framework/test/base.py
>> > +++ b/framework/test/base.py
>> > @@ -29,13 +29,33 @@ import subprocess
>> > import time
>> > import sys
>> > import traceback
>> > -from datetime import datetime
>> > -import threading
>> > -import signal
>> > import itertools
>> > import abc
>> > import copy
>> >
>> > +try:
>> > + from subprocess32 import Popen, TimeoutExpired
>> > +except ImportError:
>> > + from subprocess import Popen as _Popen
>> > +
>> > + class Popen(_Popen):
>> > + """Wrapper to provide timout on communicate.
>> > +
>> > + This allows us to use the same code without a plethora of if statements
>> > + to special case windows an non subprocess32 cases.
>> > +
>> > + """
>> > + def communicate(self, *args, **kwargs):
>> > + try:
>> > + del kwargs['timeout']
>> > + except KeyError:
>> > + pass
>> > +
>> > + super(Popen, self).communicate(*args, **kwargs)
>> > +
>> > + class TimeoutExpired(Exception):
>> > + pass
>> > +
>> > from framework import exceptions
>> > from framework.core import Options
>> > from framework.results import TestResult
>> > @@ -62,56 +82,6 @@ class TestRunError(exceptions.PiglitException):
>> > self.status = status
>> >
>> >
>> > -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
>> > -
>> > -
>> > def _is_crash_returncode(returncode):
>> > """Determine whether the given process return code correspond to a
>> > crash.
>> > @@ -147,8 +117,7 @@ class Test(object):
>> > """
>> > OPTS = Options()
>> > __metaclass__ = abc.ABCMeta
>> > - __slots__ = ['run_concurrent', 'env', 'result', 'cwd', '_command',
>> > - '__proc_timeout']
>> > + __slots__ = ['run_concurrent', 'env', 'result', 'cwd', '_command']
>> > timeout = 0
>> >
>> > def __init__(self, command, run_concurrent=False):
>> > @@ -159,7 +128,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
>> > @@ -205,11 +173,7 @@ class Test(object):
>> > """Convert the raw output of the test into a form piglit understands.
>> > """
>> > if _is_crash_returncode(self.result.returncode):
>> > - # check if the process was terminated by the timeout
>> > - if self.timeout > 0 and self.__proc_timeout.join() > 0:
>> > - self.result.result = 'timeout'
>> > - else:
>> > - self.result.result = 'crash'
>> > + self.result.result = 'crash'
>> > elif self.result.returncode != 0 and self.result.result == 'pass':
>> > self.result.result = 'warn'
>> >
>> > @@ -255,10 +219,6 @@ class Test(object):
>> > """
>> > pass
>> >
>> > - def __set_process_group(self):
>> > - if hasattr(os, 'setpgrp'):
>> > - os.setpgrp()
>> > -
>> > def _run_command(self):
>> > """ Run the test command and get the result
>> >
>> > @@ -285,29 +245,14 @@ class Test(object):
>> > self.env.iteritems()):
>> > fullenv[key] = str(value)
>> >
>> > - # preexec_fn is not supported on Windows platforms
>> > - if sys.platform == 'win32':
>> > - preexec_fn = None
>> > - else:
>> > - preexec_fn = self.__set_process_group
>> > -
>> > try:
>> > - proc = subprocess.Popen(self.command,
>> > - stdout=subprocess.PIPE,
>> > - stderr=subprocess.PIPE,
>> > - cwd=self.cwd,
>> > - env=fullenv,
>> > - universal_newlines=True,
>> > - preexec_fn=preexec_fn)
>> > - # 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()
>> > + proc = Popen(self.command,
>> > + stdout=subprocess.PIPE,
>> > + stderr=subprocess.PIPE,
>> > + cwd=self.cwd,
>> > + env=fullenv,
>> > + universal_newlines=True)
>> > + out, err = proc.communicate(timeout=self.timeout)
>> > returncode = proc.returncode
>> > except OSError as e:
>> > # Different sets of tests get built under different build
>> > @@ -317,6 +262,10 @@ class Test(object):
>> > raise TestRunError("Test executable not found.\n", 'skip')
>> > else:
>> > raise e
>> > + except TimeoutExpired:
>> > + raise TestRunError(
>> > + 'Test time out exceeded timeout of {}\n'.format(self.timeout),
>> > + 'timeout')
>>
>> The process is not killed when the timeout occurs, so the previous
>> code to implement that needs to be added here. This involves sending
>> SIGTERM to the child process and then sending SIGKILL to the process
>> group if SIGTERM failed to remove the process. The child processes
>> were placed in their own process group by the preexec_fn parameter
>> being set to __set_process_group.
>>
>
> If you have subprocess32 it will be killed when the timeout expires.
> This is a backport of the python3 behavior, from the python3 docs:
>
> """
> The timeout argument is passed to Popen.communicate(). If the timeout
> expires, the child process will be killed and waited for. The
> TimeoutExpired exception will be re-raised after the child process has
> terminated.
> """
>
> Am I missing something here?
I think that is the documentation for subprocess.run, which has a
slightly different behaviour to Popen.communicate:
https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate
"The child process is not killed if the timeout expires, so in order
to cleanup properly a well-behaved application should kill the child
process and finish communication:"
The additional step of sending SIGKILL to the process group also helps
to clean up misbehaving tests that have forked.
>
>>
>> >
>> > # The setter handles the bytes/unicode conversion
>> > self.result.out = out
>> > diff --git a/framework/tests/base_tests.py b/framework/tests/base_tests.py
>> > index caef9e3..95d35bd 100644
>> > --- a/framework/tests/base_tests.py
>> > +++ b/framework/tests/base_tests.py
>> > @@ -63,6 +63,7 @@ def test_run_return_early():
>> > @attr('slow')
>> > def test_timeout():
>> > """test.base.Test.run(): kills tests that exceed timeout when set"""
>> > + utils.module_check('subprocess32')
>> > utils.binary_check('sleep', 1)
>> >
>> > class _Test(Test):
>> > @@ -79,6 +80,7 @@ def test_timeout():
>> > def test_timeout_pass():
>> > """test.base.Test.run(): Result is returned when timeout is set but not exceeded
>> > """
>> > + utils.module_check('subprocess32')
>> > utils.binary_check('true')
>> >
>> > def helper():
>> > diff --git a/tox.ini b/tox.ini
>> > index ca97a51..366af9b 100644
>> > --- a/tox.ini
>> > +++ b/tox.ini
>> > @@ -27,4 +27,5 @@ deps =
>> > simplejson
>> > lxml
>> > backports.lzma
>> > + subprocess32
>> > commands = nosetests framework/tests --attr="!privliged" --with-cover --cover-package=framework --cover-tests
>> > --
>> > 2.6.1
>> >
More information about the Piglit
mailing list