[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Remove a race condition from test cond5.test.
From: |
Ralf Wildenhues |
Subject: |
Re: [PATCH] Remove a race condition from test cond5.test. |
Date: |
Wed, 21 Jul 2010 20:12:12 +0200 |
User-agent: |
Mutt/1.5.20 (2010-04-22) |
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