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

[Dylan Baker]

On Mon, Oct 19, 2015 at 02:09:01PM +0200, Daniel Vetter wrote:
> 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

Okay. I have a solution that I think will work for igt, I just need to
figure out how to de-tangle stuff enough to not break windows when we
want to turn this on for wider piglit.

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/20151019/331d37c7/attachment.sig>