[Piglit] [PATCH 2/2] framework/test/base.py: try to use subprocess32

[Dylan Baker]

On Thu, Oct 15, 2015 at 11:45:57AM +0100, Thomas Wood wrote:
> 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>
> >>
> >> 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 docs recommend using proc.kill() for this purpose.

I'm really hesitant to use preexec_fn, because there is a big warning
that preexec_fn is not safe to use with threads, and for the Open{G,C}L
tests having concurrency is a requirement, so if we require preexec_fn
we make timeouts and concurency mutually exclusive.

Looking at the docs there may be a way to handle process groups without
the use of a preexec_fn. I'll look into it further.

> 
> The additional step of sending SIGKILL to the process group also helps
> to clean up misbehaving tests that have forked.
> 

Do IGT tests regularly fork? I don't think any of the native piglit
tests fork.

[snip]
-------------- 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/20151015/00a7f2b0/attachment-0001.sig>