At Wednesday 21 July 2010, Ralf Wildenhues wrote: > 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 > > [CUT] > > > > -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. Yes, but that's not a problem IMO, since the test is going to fail anyway if this code path has been taken.
> > +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. Definitely right. Not waiting for its termination is a more serious error which could cause intermittent weird behaviours. > 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. OK, let's drop this patch then. It was quite ugly anyway. > The change that you already pushed was > good, though. Good! Regards, Stefano
