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

[Daniel Vetter]

On Thu, Oct 15, 2015 at 11:22:45AM -0700, Dylan Baker wrote:
> 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.

They fork a _lot_. Some do for concurrency stress tests, others so that
you have a parent who can watch the child die (e.g. to check that we
correctly clean up all the crap left behind by a dying process using drm).

We also use fork to send signals to the main thread, which is used to
validate the ioctl restarting.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch