Re: [PATCH] Remove a race condition from test cond5.test.

[Ralf Wildenhues]

Hi Stefano,

* Stefano Lattarini wrote on Mon, Jul 19, 2010 at 10:41:13PM CEST:
> And here is a follow-up patch to reduce possible race conditions 
> w.r.t. reuse of process PIDs.  The patch is against the maint branch.

I'm sorry but I have issues here, see below.

> Subject: [PATCH] Remove a race condition from test cond5.test.
> 
> * tests/cond5.test: Do not blindly try to kill the pid of the
> backgrounded Automake process after the sleep, since it might
> have terminated by itself, and its PID reused by a new and
> unrelated process.  Instead, rely on a more complex but       more
> correct combo of wrapper script(s) and temporary file(s).
> Necessity of this fix suggested by Ralf Wildenhues.

> --- a/tests/cond5.test
> +++ b/tests/cond5.test
> @@ -44,25 +44,42 @@ END
>  
>  # The bug is that automake hangs.  So we give it an appropriate grace
>  # time, then kill it if necessary.
> -$ACLOCAL
> -$AUTOMAKE 2>stderr &
> +# We use a hacky wrapper script to avoid (or reduce to a really low
> +# minimum) race conditions w.r.t. process PID.
> +
> +cat > no-race.sh <<'END'
> +#!/bin/sh
> +"$@" 2>stderr &
>  pid=$!
> +echo $pid > prog-not-finished
> +wait $pid
> +rc=$?
> +rm -f prog-not-finished
> +echo $rc > rc
> +exit $rc
> +END
> +
> +$ACLOCAL
> +: > prog-not-finished  # to be sure it will truly be there from the start
> +$SHELL -x ./no-race.sh $AUTOMAKE &
>  
>  # Make at most 30 tries, one every 10 seconds (= 300 seconds = 5 min).
>  try=1
>  while test $try -le 30; do
> -  if kill -0 $pid; then
> -    : process $pid is still alive, wait and retry
> +  if test -f prog-not-finished; then
> +    : process `cat prog-not-finished` is still alive, wait and retry
>      sleep 10
>      try=`expr $try + 1`
>    else
>      cat stderr >&2
>      # Automake must fail with a proper error message.
> +    test x"1" = x"`cat rc`"
>      grep 'variable.*OPT_SRC.*recursively defined' stderr
>      Exit 0
>    fi
>  done
>  # The automake process probably hung.  Kill it, and exit with failure.
> -echo "$me: automake process $pid hung"
> -kill $pid
> +# And yes, the repated command substitutions with `cat' are meant.

That doesn't seem ideal to me though.  If the file is removed at the
right time, cat or kill produces an unwanted error message.

> +echo "$me: automake process `cat prog-not-finished` hung"
> +(kill `cat prog-not-finished`) # some shells require this subshell
>  Exit 1

Here, we'd probably want to wait for the $SHELL -x process to finish.

In reading the original code (i.e., the one before this patch), I see
that I was mistaken last time in that the default code path does not
try to kill a process with an old, long-gone PID.  Sorry about my
misinterpretation, but upon reading again I think there is no change
warranted.  The change that you already pushed was good, though.
Thanks.

Cheers,
Ralf