Re: xargs: Kill children to stop immediately

[Bernhard Voelker]

On 04/01/2016 11:56 AM, Orivej Desh wrote:
> I would like xargs to attempt to terminate its children when one of them
> returns with the code 255 to stop immediately, whereas currently xargs
> imposes unnecessary delay and, when process produce output, makes the
> output of a failing process to be buried behind output from remaining
> running processes.

Thanks for taking the time to also send a patch for your idea; by this,
we have something more detailed to talk about.

First of all, I think there really might be use cases out there requiring
an immediate stop of the parallel processing, i.e., when using -P NUM
(with 1 < NUM).
However, I think this is not the behavior everybody wants or expects.
Therefore, making this feature turned on by default is not something
we can include.  Instead, I'd rather make it an option ... and give the
user the possibility to use a different signal to send (with TERM as
default); something like "--kill-after-255[=SIGNAL]".

> ---
>  xargs/xargs.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/xargs/xargs.c b/xargs/xargs.c
> index 5cf8c13..2541f3b 100644
> --- a/xargs/xargs.c
> +++ b/xargs/xargs.c
> @@ -133,6 +133,9 @@ static bool procs_executed = false;
>  /* The number of elements in `pids'.  */
>  static unsigned long int procs_executing = 0uL;
>  
> +/* Should we terminate child processes at exit? */
> +static bool kill_children_at_exit = false;
> +
>  /* List of child processes currently executing.  */
>  static pid_t *pids = NULL;
>  
> @@ -1455,13 +1458,23 @@ wait_for_proc (bool all, unsigned int minreap)
>             wflags = WNOHANG;
>           }
>       }
> +      else if (kill_children_at_exit)
> +        {
> +          for (i = 0; i < pids_alloc; i++)
> +            {
> +              if (pids[i] != 0)
> +                {
> +                  kill(pids[i], SIGTERM);
> +                }
> +            }
> +        }
>  
>        stop_waiting = 0;
>        do
>       {
>         /* Wait for any child.   We used to use wait () here, but it's
>          * unlikely that that offers any portability advantage over
> -        * wait these days.
> +        * waitpid these days.
>          */
>         while ((pid = waitpid (-1, &status, wflags)) == (pid_t) -1)
>           {
> @@ -1518,8 +1531,11 @@ wait_for_proc (bool all, unsigned int minreap)
>        reaped++;
>  
>        if (WEXITSTATUS (status) == CHILD_EXIT_PLEASE_STOP_IMMEDIATELY)
> -     error (XARGS_EXIT_CLIENT_EXIT_255, 0,
> -            _("%s: exited with status 255; aborting"), bc_state.cmd_argv[0]);
> +        {
> +          kill_children_at_exit = true;
> +          error (XARGS_EXIT_CLIENT_EXIT_255, 0,
> +                 _("%s: exited with status 255; aborting"), 
> bc_state.cmd_argv[0]);
> +        }
>        if (WIFSTOPPED (status))
>       error (XARGS_EXIT_CLIENT_FATAL_SIG, 0,
>              _("%s: stopped by signal %d"), bc_state.cmd_argv[0], WSTOPSIG 
> (status));
> 

I'm not sure this is really the behavior you want to achieve: with that,
xargs doesn't wait for the processes to terminate after sending them
the TERM signal:

  $ cat /tmp/exit255
  #!/bin/bash
  ARG="$1"
  trap '{ echo "$ARG: got signal TERM"; exit 0; }' TERM
  [ "$ARG" = 255 ] && exit 255
  sleep 2
  exit 0

  $ seq 250 260 | xargs/xargs -t -P 20  -n 1 /tmp/exit255; \
      echo '=======DONE======='
  /tmp/exit255 250
  /tmp/exit255 251
  /tmp/exit255 252
  /tmp/exit255 253
  /tmp/exit255 254
  /tmp/exit255 255
  /tmp/exit255 256
  /tmp/exit255 257
  /tmp/exit255 258
  /tmp/exit255 259
  xargs/xargs: /tmp/exit255: exited with status 255; aborting
  xargs/xargs: /tmp/exit255: terminated by signal 15
  =======DONE=======
  $ 250: got signal TERM
  251: got signal TERM
  252: got signal TERM
  253: got signal TERM
  254: got signal TERM

Furthermore, the above "terminated by signal 15" (which doesn't show
up every time!) line looks like there's something suspicious going on.

Finally, I'm not sure whether you already have the FSF Copyright assignment
for findutils in place.  A patch of this size requires that process to be
done.

After all, I think this is a nice option worth adding ... when the
above issues are clarified/fixed - and tests and docs are in place.

Thanks & have a nice day,
Berny