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

["Jakub Narębski" <jnareb@gmail.com>]

W dniu 01.10.2016 o 20:59, Lars Schneider pisze: 
> 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.

Well, it would be good to tell users _why_ Git is hanging, see below.

>>
>> 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.

That might be good approach.  Probably the default would be to wait.

> 
> 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".

Sidenote: we prefer camelCase rather than kebab-case for config
variables, that is, "filter.<driver>.processTimeout".

Also, how would one set default value of timeout for all process
based filters?

> 
> What do you think about this solution?

I think this addition be done after, assuming that we come up
with good default behavior (e.g. wait for filter processes
to finish).

Also, we would probably want to add some progress information
in a similar way to progress info for checkout, that is display
it after a few seconds waiting.

This could be, for example:

  Waiting for filter '<driver>' to finish... done

With timeout it could look like this (where underlined part
is interactive, that is changes every second):

  Waiting 10s for '<driver>' filter process to finish.
          ^^^

And then either

  Filter '<driver>' killed

or

  Filter '<driver>' finished


> 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);

Why are you not closing stdin of filter driver process if timeout
is zero?  Is it maybe some kind of special value?  If it is, this
is not documented.

> +
> +		if (p->timeout < 0) {
> +			// Wait until the process finishes
> +			while ((waitpid(p->pid, &status, 0)) < 0 && errno == EINTR)
> +				;	/* nothing */

Ah, this loop is here because waiting on waitpid() can be interrupted
by the delivery of a signal to the calling process; though the result
is -1, not just any < 0.

Looks good (but we would want some progress information, probably).

> +		} 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);
> +			}

WTF?  Busy wait loop???

Also, if we want to wait for child without blocking, then instead
of cryptic getpgid(p->pid) maybe use waitpid(p->pid, &status, WNOHANG);
it is more explicit.

There is also another complication: there can be more than one
long-running filter driver used.  With this implementation we
wait for each of one in sequence, e.g. 10s + 10s + 10s.

> +		}
> +
>  		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)

Hmmmm... three parameters is not too much, but we might want to
pass "struct child_process *" directly if this number grows.

>  {
>  	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);

Eh?  A bit magic.

>  
>  	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;

I guess it is no problem adding new field to child_process struct;
it is not constrained for memory...

>  	const char *dir;
>  	const char *const *env;
>  	unsigned no_stdin:1;
> 

Where we read the value of the configuration variable?