On 27/06/11 21:12, Alan Curry wrote:
> =?UTF-8?Q?P=C3=A1draig?= Brady writes:
>>
>> On 26/06/11 20:20, shay shimony wrote:
>>> all:
>>> timeout 12 sleep 10
>>>
>>> Note there is a tab before "timeout 12 sleep 10".
>>> Then run at same directory where the file is located "make" and try to press
>>> CTRL-C.
>>>
>>> Notes:
>>> CTRL-Z works.
>>> When executing timeout without make CTRL-C works.
>>> When executing make without timeout CTRL-C works.
>>
>> Drats,
>>
>> That because SIGINT is sent by the terminal to the foreground group.
>> The issue is that `make` and `timeout` use much the same method
>> to control their jobs. I.E. they create their own process group
>> so they can terminate all sub-processes.
>
> Are you sure? I see no evidence of that. When I run make with the above
> makefile, the processes look like this:
>
> PPID PID PGID SID TTY TPGID STAT UID TIME COMMAND
> 1 1451 1451 1451 6 16407 S 1000 0:06 -zsh
> 1451 16407 16407 1451 6 16407 S 1000 0:00 make
> 16407 16408 16408 1451 6 16407 S 1000 0:00 timeout 60 sleep 30
> 16408 16409 16408 1451 6 16407 S 1000 0:00 sleep 30
>
> The first PGID is the login shell. The second PGID is make, which was put
> into its own process group by the shell because the shell has job control
> enabled. The last PGID is timeout, which put itself into a process group.
> make never noticed any of them.
>
> In the source for GNU make 3.82 there are no calls to setpgrp or setpgid
> (unless obfuscated from grep). There is the following comment:
>
> /* A termination signal won't be sent to the entire
> process group, but it means we want to kill the children. */
>
> That's above the handling of SIGTERM, which iterates over child processes and
> passes along the SIGTERM to them.
>
> After that is the handling of SIGINT, which doesn't kill child processes
> (unless they're "remote", which is... news to me that make does remote
> things) but just waits for them.
>
> What seems to be happening is that make *doesn't* create a process group,
> therefore assumes that when it gets a SIGINT, its children have already
> gotten it too, and it just waits for them to die. A child that puts itself
> into a new process group screws this up (as would kill -2 `pidof make`).
Thanks for the analysis Alan.
Yes you're right I think.
In any case the important point is that timeout sets itself as group leader,
and is not the foreground group.
>
> I think the answer is that timeout should put itself into the foreground.
> That way it would get the SIGINT. make wouldn't get it, but wouldn't need to.
> timeout would exit quickly after SIGINT and make would proceed or abort
> according to the exit code.
I've a version locally here actually that calls tcsetpgrp() but I discounted
that as it's not timeout's place to call that I think.
timeout sets itself as group leader so that it can kill everything it starts,
but it shouldn't need to grab the foreground group as the shell (or make)
may be starting it in the background etc.
So really `make` should not assume children stay in the same group,
and propagate signals down (like it does for TERM).
It might be appropriate for `make` to call tcsetpgrp() before exec..()
but given remote & parallel jobs etc. signal propagation might be best.
I've fixed up `timeout` itself not assume children stay in
the same group, and pass signals on, in the attached patch.
cheers,
Pádraig.
>From 60f225e8806707bc899c53732d2796cdfd1d9b05 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 27 Jun 2011 19:33:16 +0100
Subject: [PATCH] timeout: support cascaded timeouts
* src/timeout.c (cleanup): Send signals directly to the child
in case it has started its own process group (like a cascaded
timeout command would for example).
* test/misc/timeout: Add a test case.
* NEWS: Mention the fix.
Reported by Shay Shimony
---
NEWS | 3 +++
src/timeout.c | 10 +++++++++-
tests/misc/timeout | 17 +++++++++++++++++
3 files changed, 29 insertions(+), 1 deletions(-)
diff --git a/NEWS b/NEWS
index 619fbdd..e18a6ec 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,9 @@ GNU coreutils NEWS -*- outline -*-
split --number l/... no longer creates extraneous files in certain cases.
[bug introduced in coreutils-8.8]
+ timeout now sends signals to commands that create their own process group.
+ [bug introduced in coreutils-7.0]
+
** Changes in behavior
chmod, chown and chgrp now output the original attributes in messages,
diff --git a/src/timeout.c b/src/timeout.c
index a686225..3877fae 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -108,9 +108,17 @@ cleanup (int sig)
alarm (kill_after);
kill_after = 0; /* Don't let later signals reset kill alarm. */
}
+ /* Send the signal directly to the monitored child,
+ in case it has itself become group leader. */
+ send_sig (monitored_pid, sig);
+ /* The normal case is the job has remained in our
+ newly created process group, so send to all processes in that. */
send_sig (0, sig);
if (sig != SIGKILL && sig != SIGCONT)
- send_sig (0, SIGCONT);
+ {
+ send_sig (monitored_pid, SIGCONT);
+ send_sig (0, SIGCONT);
+ }
}
else /* we're the child or the child is not exec'd yet. */
_exit (128 + sig);
diff --git a/tests/misc/timeout b/tests/misc/timeout
index 7506e7c..080436c 100755
--- a/tests/misc/timeout
+++ b/tests/misc/timeout
@@ -51,4 +51,21 @@ test $? = 124 && fail=1
exec timeout 10 true
) || fail=1
+cat <<\EOF > sleep_usr1
+#!/bin/sh
+trap 'echo USR1; exit' USR1
+sleep $1&
+wait
+EOF
+chmod a+x sleep_usr1
+
+# Ensure the first timeout kills the processes,
+# which was not the case before 8.13.
+# Note the first timeout must send a signal that
+# the second is handling for it to be propagated to the command.
+# SIGINT, SIGTERM, SIGALRM etc. are implicit.
+timeout -sALRM 1 timeout -sUSR1 10 ./sleep_usr1 5 > out
+echo USR1 > exp
+compare out exp || fail=1
+
Exit $fail
--
1.7.5.2
