[Piglit] [PATCH v2 6/6] framework/test/base.py: use subprocess32 for timeouts.

[Dylan Baker]

On Mon, Oct 26, 2015 at 04:08:44PM +0000, Thomas Wood wrote:
> On 24 October 2015 at 02:17,  <baker.dylan.c at gmail.com> wrote:
> > From: Dylan Baker <baker.dylan.c at gmail.com>
> >
[snip]
> > -            # 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?

I can make that change.

> 
> 
> > +                    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.

I can make that change too.

> 
> 
> > +
> > +                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?

Yes. This gives us thread safety, which is the primary reason for
making this series.

From the documentation for subprocess (python 3.3, which is what
subprocess32 is based on):
"""
 The preexec_fn parameter is not safe to use in the presence of threads
 in your application. The child process could deadlock before exec is
 called. If you must use it, keep it trivial! Minimize the number of
 libraries you call into.

 If you need to modify the environment for the child use the env
 parameter rather than doing it in a preexec_fn. The start_new_session
 parameter can take the place of a previously common use of preexec_fn
 to call os.setsid() in the child.
"""

From what I read on the matter and the tests I wrote it seems like a
session will work for our purposes, but I'm by no means an expert in
unix processes.

> 
> 
> >
> >              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?
> 

Sure, we could probably also provide some sort of hook to bail out if
timeouts aren't available.

> 
> 
[snip]

Dylan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20151026/460dd57f/attachment.sig>