> On 29 Sep 2016, at 23:27, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Lars Schneider <larsxschneider@xxxxxxxxx> writes: > >> We discussed that issue in v4 and v6: >> http://public-inbox.org/git/20160803225313.pk3tfe5ovz4y3i7l@xxxxxxxxxxxxxxxxxxxxx/ >> http://public-inbox.org/git/xmqqbn0a3wy3.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxx/ >> >> My impression was that you don't want Git to wait for the filter process. >> If Git waits for the filter process - how long should Git wait? > > I am not sure where you got that impression. I did say that I do > not want Git to _KILL_ my filter process. That does not mean I want > Git to go away without waiting for me. > > If the filter process refuses to die forever when Git told it to > shutdown (by closing the pipe to it, for example), that filter > process is simply buggy. I think we want users to become aware of > that, instead of Git leaving it behind, which essentially is to > sweep the problem under the rug. > > I agree with what Peff said elsewhere in the thread; if a filter > process wants to take time to clean things up while letting Git > proceed, it can do its own process management, but I think it is > sensible for Git to wait the filter process it directly spawned. To realize the approach above I prototyped the run-command patch below: I added an "exit_timeout" variable to the "child_process" struct. On exit, Git will close the pipe to the process and wait "exit_timeout" seconds until it kills the child process. If "exit_timeout" is negative then Git will wait until the process is done. If we use that in the long running filter process, then we could make the timeout even configurable. E.g. with "filter.<driver>.process-timeout". What do you think about this solution? Thanks, Lars diff --git a/run-command.c b/run-command.c index 3269362..a933066 100644 --- a/run-command.c +++ b/run-command.c @@ -21,6 +21,8 @@ void child_process_clear(struct child_process *child) struct child_to_clean { pid_t pid; + int stdin; + int timeout; struct child_to_clean *next; }; static struct child_to_clean *children_to_clean; @@ -28,9 +30,30 @@ static int installed_child_cleanup_handler; static void cleanup_children(int sig, int in_signal) { + int status; + struct timeval tv; + time_t secs; + while (children_to_clean) { struct child_to_clean *p = children_to_clean; children_to_clean = p->next; + + if (p->timeout != 0 && p->stdin > 0) + close(p->stdin); + + if (p->timeout < 0) { + // Wait until the process finishes + while ((waitpid(p->pid, &status, 0)) < 0 && errno == EINTR) + ; /* nothing */ + } else if (p->timeout != 0) { + // Wait until the process finishes or timeout + gettimeofday(&tv, NULL); + secs = tv.tv_sec; + while (getpgid(p->pid) >= 0 && tv.tv_sec - secs < p->timeout) { + gettimeofday(&tv, NULL); + } + } + kill(p->pid, sig); if (!in_signal) free(p); @@ -49,10 +72,12 @@ static void cleanup_children_on_exit(void) cleanup_children(SIGTERM, 0); } -static void mark_child_for_cleanup(pid_t pid) +static void mark_child_for_cleanup(pid_t pid, int timeout, int stdin) { struct child_to_clean *p = xmalloc(sizeof(*p)); p->pid = pid; + p->stdin = stdin; + p->timeout = timeout; p->next = children_to_clean; children_to_clean = p; @@ -422,7 +447,7 @@ int start_command(struct child_process *cmd) if (cmd->pid < 0) error_errno("cannot fork() for %s", cmd->argv[0]); else if (cmd->clean_on_exit) - mark_child_for_cleanup(cmd->pid); + mark_child_for_cleanup(cmd->pid, cmd->exit_timeout, cmd->in); /* * Wait for child's execvp. If the execvp succeeds (or if fork() @@ -483,7 +508,7 @@ int start_command(struct child_process *cmd) if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT)) error_errno("cannot spawn %s", cmd->argv[0]); if (cmd->clean_on_exit && cmd->pid >= 0) - mark_child_for_cleanup(cmd->pid); + mark_child_for_cleanup(cmd->pid, cmd->exit_timeout, cmd->in); argv_array_clear(&nargv); cmd->argv = sargv; @@ -765,7 +790,7 @@ int start_async(struct async *async) exit(!!async->proc(proc_in, proc_out, async->data)); } - mark_child_for_cleanup(async->pid); + mark_child_for_cleanup(async->pid, 0, -1); if (need_in) close(fdin[0]); diff --git a/run-command.h b/run-command.h index cf29a31..f2eca33 100644 --- a/run-command.h +++ b/run-command.h @@ -33,6 +33,7 @@ struct child_process { int in; int out; int err; + int exit_timeout; const char *dir; const char *const *env; unsigned no_stdin:1;