On 14 October 2015 at 22:35, Dylan Baker <[email protected]> wrote: > On Wed, Oct 14, 2015 at 04:27:56PM +0100, Thomas Wood wrote: >> On 9 October 2015 at 20:53, <[email protected]> wrote: >> > From: Dylan Baker <[email protected]> >> > >> > 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 <[email protected]> >> > --- >> > 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 >> > _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
