Re: [PATCH v8 00/11] Git filter protocol

From

Lars Schneider <larsxschneider@gmail.com>

Date

Oct 1, 2016, 18:59 UTC

Message-ID

<C53500E8-7352-4AAC-9F53-40CCFA7F1418@gmail.com>

In-Reply-To

<xmqq60pee6rp.fsf@gitster.mtv.corp.google.com>

> On 29 Sep 2016, at 23:27, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>> We discussed that issue in v4 and v6:
>> http://public-inbox.org/git/20160803225313.pk3tfe5ovz4y3i7l@sigill.intra.peff.net/
>> http://public-inbox.org/git/xmqqbn0a3wy3.fsf@gitster.mtv.corp.google.com/
>> 
>> 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:

What do you think about this solution? 

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;