Re: wait regression in 3800d4

[Jilles Tjoelker <jilles@stack.nl>]

On Wed, May 19, 2010 at 07:08:56PM +0000, Gerrit Pape wrote:
> Hi, since commit 3800d49 the wait builtin shows some unexpected
> behavior, see http://bugs.debian.org/581425

> To reproduce:

>  $ dash -c '
>  for i in 1 2 3; do
>    (sleep $i; echo $i) &
>  done
>  wait
>  echo all done
>  sleep 2'
>  1
>  all done
>  2
>  3
>  $ 

> Expected output:
>  1
>  2
>  3
>  all done

> I don't completely understand commitdiff 3800d49, and didn't find time
> to fix the lack of understanding yet, but this patch seems to fix this
> specific issue.

> diff --git a/src/jobs.c b/src/jobs.c
> index a4fada0..57c0854 100644
> --- a/src/jobs.c
> +++ b/src/jobs.c
> @@ -1136,8 +1136,6 @@ waitproc(int block, int *status)
>                 if (err || !block)
>                         break;
>  
> -               block = 0;
> -
>                 sigfillset(&mask);
>                 sigprocmask(SIG_SETMASK, &mask, &oldmask);
>  

This patch is not correct as it makes dash busy-wait for the second and
further processes to terminate. Also, I think aborting the wait builtin
with 128+SIGCHLD is the correct behaviour if a trap on CHLD is active
(like any other trap). This requires a more sophisticated handling of
pendingsigs so that an untrapped SIGCHLD does not overwrite a trapped
signal.

I wonder why this hasn't been noticed before, as this is the most common
use of the wait builtin. I can even reproduce the regression with the
test script in 3800d49's commit message: sometimes I see a line *** 148
meaning that the wait builtin was aborted by SIGCHLD (even though no
CHLD trap has been set); however only on an SMP system. (This is on
FreeBSD; I hacked mksignames.c to compile and work, although realtime
signals are missing.)

-- 
Jilles Tjoelker