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

[Daniel Vetter]

On Mon, Oct 26, 2015 at 11:33:22AM -0700, Dylan Baker wrote:
> 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.

Any code between fork() and exec() must be signal-safe since effects of
other threads (like holding locks) aren't synchronized at all. Which
means there's absolutely nothing you can do in there except some very
few, explicitly whitelisted functions in a general app with threads.
Session group is overkill indeed, but shouldn't be harmful either, since
already using a process group is enough to break ^Z.
-Daniel
 
> > 
> > 
> > >
> > >              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



> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch