I was trying to write a git subcommand, and I noticed that if I ctrl-c'd
it, git would return, but leave the subcommand running in the
background.
You can see the problem with this test case.
#!/usr/bin/perl
print "first sleep...\n";
$ret=system("sleep", "1m");
print "second sleep...\n";
system("sleep", "1s");
print "done with second sleep\n";
If you put it in path named git-sleep, then run "git sleep" and press ctrl-c,
it keeps running:
joey@gnu:~>git sleep
first sleep...
^Csecond sleep...
joey@gnu:~>done with second sleep
So what's going on? Well, perl's system() blocks sigint while the child
process is running. So if you run this as git-sleep, and press ctrl-c,
it will continue on to the second sleep. If the code above checked the
return status of system() it could detect that it was killed by SIGINT
and itself exit.
What I don't understand is, why does git not wait() on the subcommand it
ran? Any subcommand that forgets to check exit codes is liable to exhibit
this weird behavior sometimes.
Ie, imagine the subcommand was running something like
"git config --get core.bare" instead of sleep.
It'd be easy to forget to check the exit status of that for a SIGINT; if
the user ctrl-c'd at just the right instant, weird things would happen.
--
see shy jo
Before git 1.6.4, we used execvp to run external git dashed commands,
thus git did not return until this command is finished. With switching to
run_command (which was necessary to fix a pager issue; see d8e96fd86d4),
CTRL-C could cause that git returned before than the git dashed command is
finished.
The solution is to disable SIGINT and SIGQUIT as it is normally done by
system(). Disabling these signals is done only when silent_exec_failure
is set, which means that the current process is used as a proxy to run
another command.
Signed-off-by: Dmitry Potapov <redacted>
---
run-command.c | 19 +++++++++++++++++++
1 files changed, 19 insertions(+), 0 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:49:49
On Tue, Oct 19, 2010 at 03:59:43PM +0400, Dmitry Potapov wrote:
The solution is to disable SIGINT and SIGQUIT as it is normally done by
system(). Disabling these signals is done only when silent_exec_failure
is set, which means that the current process is used as a proxy to run
another command.
I don't understand why we would only do it for silent_exec_failure. You
claim that flag means that the current process is a proxy for another
command, but:
1. Is that really the case, or do the two things just happen to
coincide in the current codebase?
2. Why do we want to do it only for the proxy-command case? If I have
a long-running external diff or merge helper, for example, what
should happen on SIGINT? Should we exit with the child still
potentially running, or should we actually be reaping the child
properly?
How does this interact with the sigchain code? If I do:
start_command(...);
sigchain_push(...);
finish_command(...);
we will overwrite the function pushed in the sigchain_push with a stale
handler. I think you could just replace your signal() calls with:
sigchain_push(SIGINT, SIG_IGN);
...
sigchain_pop(SIGINT);
but I wonder if ignoring is necessarily the right thing. Shouldn't we
just reap the child and then run the signal handler that was there
before us? That means in general that we will continue to die via SIGINT
when we see SIGINT. With your patch, we will ignore it and (presumably)
end up dying with a return code indicated that the child had an error.
I think both of these things are not problems for executing dashed
externals. But as above, I am not sure that we should be limiting this
signal handling to those cases.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:49:49
On Tue, Oct 19, 2010 at 09:32:36AM -0400, Jeff King wrote:
How does this interact with the sigchain code? If I do:
start_command(...);
sigchain_push(...);
finish_command(...);
we will overwrite the function pushed in the sigchain_push with a stale
handler. I think you could just replace your signal() calls with:
sigchain_push(SIGINT, SIG_IGN);
...
sigchain_pop(SIGINT);
On Tue, Oct 19, 2010 at 09:32:36AM -0400, Jeff King wrote:
2. Why do we want to do it only for the proxy-command case? If I have
a long-running external diff or merge helper, for example, what
should happen on SIGINT? Should we exit with the child still
potentially running, or should we actually be reaping the child
properly?
Probably, it should be done in other cases too. However, I am not sure
if it should be done unconditionally. For instance, when we run a pager,
I don't think we should ignore the signals just because we started a
pager.
I agree that silent_exec_failure is not the best flag for that -- I was
just trying to make minimal changes to the existing behavior, and if
this flag is set, you seem always want to ignore these signals, but
there are some other cases too as you pointed above.
Now, I think we should always ignore these signals when run_command() is
used (similar to system()), but do not mask signals if start_command()
is used (or make it optional by adding a new flag).
we will overwrite the function pushed in the sigchain_push with a stale
handler. I think you could just replace your signal() calls with:
sigchain_push(SIGINT, SIG_IGN);
...
sigchain_pop(SIGINT);
Yes, it is certainly better. I was not aware about these functions.
Dmitry
From: Jeff King <hidden> Date: 2016-06-15 22:49:49
On Tue, Oct 19, 2010 at 02:16:38PM -0500, Jonathan Nieder wrote:
I think sigchain_push ought to accept a context object.
But signal() doesn't, so we would have to install a wrapper function
that gets the signal and calls the sigchain_pushed callback with the
context object. But we can't always install the wrapper. We need to
check for SIG_IGN and SIG_DFL, and literally install those.
So I think it's do-able, but I tried to keep the original sigchain as
simple as possible.
Yuck. You can get around that by pushing onto a linked list of children,
though.
Thinking about it more, though, I don't think we do necessarily want to
always wait for the child. There are really two main types of
run_command's we do:
1. The run command is basically the new process. In an ideal world, we
would exec into it, but we need the parent to hang around to do
some kind of bookkeeping (like waiting for the pager to exit).
E.g., running external dashed commands.
2. We are running the command, and if we are killed, the command
should go away too (because its point in running is to give us some
information).
E.g., running textconv filters.
And there are a few instances that don't fall into either category
(e.g., running the pager).
In case (1), we probably want to SIG_IGN, wait for the command to
finish, and then die with its exit code. If we do it right, the fact
that _it_ was killed by signal will be propagated, and the fact that we
weren't will be irrelevant.
In case (2), we probably want to keep a linked list of "expendable"
processes, and on signal death and atexit, go through the list and make
sure all are dead. This is how we handle tempfiles already in diff.c.
Given that there is only really one instance of (1), we can just code it
there. For (2), there are many such callers, but I don't know that the
mechanism necessarily needs to be included as part of run_command. A
separate module to manage the list and set up the signal handler would
be fine (though there is a race between fork() and signal death, so it
perhaps pays to get the newly created pid on the "expendable" list as
soon as possible, which may mean cooperating from run_command).
-Peff