[PATCH 0/5] Fixes for the parallel processing

[PATCH 0/5] Fixes for the parallel processing

[Stefan Beller]

I noticed a problem with the gracefully abortion in the parallel processing,
that is fixed in patch 1.

Patch 2 makes the API more maintainable/usable (the caller may forget to
call child_process_init and only fill in fields which the callback is
interested in)

Patch 3 is another fixup. It actually initializes the shutdown flag properly.
(Apparently it had the right value so far)

Patches 4 and 5 add more tests both for the gracefully aborting case as well
as the standard use case.

This applies on top of sb/submodule-parallel-fetch which is already in next.
Junio, do you want me to reroll that series with these patches squashed in
appropriately, or just put it on top of that series ?

Thanks,
Stefan

Stefan Beller (5):
  run-command: Fix early shutdown
  run-command: Call get_next_task with a clean child process.
  run-command: Initialize the shutdown flag
  test-run-command: test for gracefully aborting
  test-run-command: Increase test coverage

 run-command.c          |  5 ++++-
 t/t0061-run-command.sh | 28 ++++++++++++++++++++++++++--
 test-run-command.c     | 22 ++++++++++++++++++++--
 3 files changed, 50 insertions(+), 5 deletions(-)

-- 
2.5.0.285.g8fe9b61.dirty

[PATCH 1/5] run-command: Fix early shutdown

[Stefan Beller]

The return value of `pp_collect_finished` indicates if we want to shutdown
the parallel processing early. Both returns from that function should
return any accumulated results.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index ef3da27..8f47c6e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1077,7 +1077,7 @@ static int pp_collect_finished(struct parallel_processes *pp)
 	while (pp->nr_processes > 0) {
 		pid = waitpid(-1, &wait_status, WNOHANG);
 		if (pid == 0)
-			return 0;
+			return result;
 
 		if (pid < 0)
 			die_errno("wait");
-- 
2.5.0.285.g8fe9b61.dirty

Re: [PATCH 1/5] run-command: Fix early shutdown

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The return value of `pp_collect_finished` indicates if we want to shutdown
> the parallel processing early. Both returns from that function should
> return any accumulated results.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Makes sense.  The code could "break" out of the loop, leaving only
one return site in the function, which would be a way to ensure that
we'd consistently return the accumulated result, but this would also
do.

Thanks.

>  run-command.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index ef3da27..8f47c6e 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1077,7 +1077,7 @@ static int pp_collect_finished(struct parallel_processes *pp)
>  	while (pp->nr_processes > 0) {
>  		pid = waitpid(-1, &wait_status, WNOHANG);
>  		if (pid == 0)
> -			return 0;
> +			return result;
>  
>  		if (pid < 0)
>  			die_errno("wait");

[PATCH 2/5] run-command: Call get_next_task with a clean child process.

[Stefan Beller]

If the `get_next_task` did not explicitly called child_process_init
and only filled in some fields, there may have been some stale data
in the child process. This is hard to debug and also adds a review
burden for each new user of that API. To improve the situation, we
pass only cleanly initialized child structs to the get_next_task.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/run-command.c b/run-command.c
index 8f47c6e..b8b5747 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1010,6 +1010,8 @@ static int pp_start_one(struct parallel_processes *pp)
 	if (i == pp->max_processes)
 		die("BUG: bookkeeping is hard");
 
+	child_process_init(&pp->children[i].process);
+
 	if (!pp->get_next_task(&pp->children[i].data,
 			       &pp->children[i].process,
 			       &pp->children[i].err,
-- 
2.5.0.285.g8fe9b61.dirty

Re: [PATCH 2/5] run-command: Call get_next_task with a clean child process.

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> If the `get_next_task` did not explicitly called child_process_init

I locally did "If get_next_task did not explicitly call child_process_init"

> and only filled in some fields, there may have been some stale data
> in the child process. This is hard to debug and also adds a review
> burden for each new user of that API. To improve the situation, we
> pass only cleanly initialized child structs to the get_next_task.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Again this makes sense.

Another way, which I suspect may be conceptually cleaner, would be
to do this clean-up where pp->children[i].in_use is set to false, as
that is where the particular task is declared to be available for
reuse.  That location should be responsible to ensure that the task
indeed is reusable by calling child_process_init().

By the way, does child_process_init() leak old argv/env arrays, or
have they already been cleared by calling finish_command() when we
come to this codepath?

>  run-command.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 8f47c6e..b8b5747 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1010,6 +1010,8 @@ static int pp_start_one(struct parallel_processes *pp)
>  	if (i == pp->max_processes)
>  		die("BUG: bookkeeping is hard");
>  
> +	child_process_init(&pp->children[i].process);
> +
>  	if (!pp->get_next_task(&pp->children[i].data,
>  			       &pp->children[i].process,
>  			       &pp->children[i].err,

Re: [PATCH 2/5] run-command: Call get_next_task with a clean child process.

[Stefan Beller]

On Tue, Oct 20, 2015 at 11:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> If the `get_next_task` did not explicitly called child_process_init
>
> I locally did "If get_next_task did not explicitly call child_process_init"
>
>> and only filled in some fields, there may have been some stale data
>> in the child process. This is hard to debug and also adds a review
>> burden for each new user of that API. To improve the situation, we
>> pass only cleanly initialized child structs to the get_next_task.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> Again this makes sense.
>
> Another way, which I suspect may be conceptually cleaner, would be
> to do this clean-up where pp->children[i].in_use is set to false, as
> that is where the particular task is declared to be available for
> reuse.  That location should be responsible to ensure that the task
> indeed is reusable by calling child_process_init().
>
> By the way, does child_process_init() leak old argv/env arrays, or
> have they already been cleared by calling finish_command() when we
> come to this codepath?

It does, yes. In the reroll I move the initialization to both pp_init (to init
for the first use) and to the place where in_use is set to false.

>
>>  run-command.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 8f47c6e..b8b5747 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -1010,6 +1010,8 @@ static int pp_start_one(struct parallel_processes *pp)
>>       if (i == pp->max_processes)
>>               die("BUG: bookkeeping is hard");
>>
>> +     child_process_init(&pp->children[i].process);
>> +
>>       if (!pp->get_next_task(&pp->children[i].data,
>>                              &pp->children[i].process,
>>                              &pp->children[i].err,

[PATCH 3/5] run-command: Initialize the shutdown flag

[Stefan Beller]

It did work out without initializing the flag so far, but to make it
future proof, we want to explicitly initialize the flag.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/run-command.c b/run-command.c
index b8b5747..108b930 100644
--- a/run-command.c
+++ b/run-command.c
@@ -956,6 +956,7 @@ static struct parallel_processes *pp_init(int n,
 
 	pp->nr_processes = 0;
 	pp->output_owner = 0;
+	pp->shutdown = 0;
 	pp->children = xcalloc(n, sizeof(*pp->children));
 	pp->pfd = xcalloc(n, sizeof(*pp->pfd));
 	strbuf_init(&pp->buffered_output, 0);
-- 
2.5.0.285.g8fe9b61.dirty

[PATCH 4/5] test-run-command: test for gracefully aborting

[Stefan Beller]

We reuse the get_next_task callback which would stop after invoking the
test program 4 times. However as we have only 3 parallel processes started
(We pass in 3 as max parallel processes and 3 is smaller than the spawn
cap in run-command, so we will start the 3 processes in the first run
deterministically), then the abort logic from the new task_finished
callback kicks in and prevents the parallel processing engine to start
any more processes.

As the task_finished is unconditional, we will see its output 3 times, too.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t0061-run-command.sh | 14 ++++++++++++++
 test-run-command.c     | 14 ++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 49aa3db..0af77cd 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -67,4 +67,18 @@ test_expect_success 'run_command runs in parallel' '
 	test_cmp expect actual
 '
 
+cat >expect <<-EOF
+preloaded output of a child
+asking for a quick stop
+preloaded output of a child
+asking for a quick stop
+preloaded output of a child
+asking for a quick stop
+EOF
+
+test_expect_success 'run_command is asked to abort gracefully' '
+	test-run-command run-command-abort-3 false 2>actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/test-run-command.c b/test-run-command.c
index 699d9e9..4b59482 100644
--- a/test-run-command.c
+++ b/test-run-command.c
@@ -34,6 +34,16 @@ static int parallel_next(void** task_cb,
 	return 1;
 }
 
+static int task_finished(int result,
+			 struct child_process *cp,
+			 struct strbuf *err,
+			 void *pp_cb,
+			 void *pp_task_cb)
+{
+	strbuf_addf(err, "asking for a quick stop\n");
+	return 1;
+}
+
 int main(int argc, char **argv)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
@@ -55,6 +65,10 @@ int main(int argc, char **argv)
 		exit(run_processes_parallel(4, parallel_next,
 					    NULL, NULL, &proc));
 
+	if (!strcmp(argv[1], "run-command-abort-3"))
+		exit(run_processes_parallel(3, parallel_next,
+					    NULL, task_finished, &proc));
+
 	fprintf(stderr, "check usage\n");
 	return 1;
 }
-- 
2.5.0.285.g8fe9b61.dirty

[PATCH 5/5] test-run-command: Increase test coverage

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t0061-run-command.sh | 16 +++++++++++++---
 test-run-command.c     | 12 ++++++++----
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 0af77cd..f27ada7 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -62,8 +62,18 @@ Hello
 World
 EOF
 
-test_expect_success 'run_command runs in parallel' '
-	test-run-command run-command-parallel-4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
+test_expect_success 'run_command runs in parallel with more jobs available than tasks' '
+	test-run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
+	test-run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'run_command runs in parallel with more tasks than jobs available' '
+	test-run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
 	test_cmp expect actual
 '
 
@@ -77,7 +87,7 @@ asking for a quick stop
 EOF
 
 test_expect_success 'run_command is asked to abort gracefully' '
-	test-run-command run-command-abort-3 false 2>actual &&
+	test-run-command run-command-abort 3 false 2>actual &&
 	test_cmp expect actual
 '
 
diff --git a/test-run-command.c b/test-run-command.c
index 4b59482..44710c3 100644
--- a/test-run-command.c
+++ b/test-run-command.c
@@ -47,6 +47,7 @@ static int task_finished(int result,
 int main(int argc, char **argv)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
+	int jobs;
 
 	if (argc < 3)
 		return 1;
@@ -61,12 +62,15 @@ int main(int argc, char **argv)
 	if (!strcmp(argv[1], "run-command"))
 		exit(run_command(&proc));
 
-	if (!strcmp(argv[1], "run-command-parallel-4"))
-		exit(run_processes_parallel(4, parallel_next,
+	jobs = atoi(argv[2]);
+	proc.argv = (const char **)argv+3;
+
+	if (!strcmp(argv[1], "run-command-parallel"))
+		exit(run_processes_parallel(jobs, parallel_next,
 					    NULL, NULL, &proc));
 
-	if (!strcmp(argv[1], "run-command-abort-3"))
-		exit(run_processes_parallel(3, parallel_next,
+	if (!strcmp(argv[1], "run-command-abort"))
+		exit(run_processes_parallel(jobs, parallel_next,
 					    NULL, task_finished, &proc));
 
 	fprintf(stderr, "check usage\n");
-- 
2.5.0.285.g8fe9b61.dirty

Re: [PATCH 5/5] test-run-command: Increase test coverage

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t0061-run-command.sh | 16 +++++++++++++---
>  test-run-command.c     | 12 ++++++++----
>  2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index 0af77cd..f27ada7 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -62,8 +62,18 @@ Hello
>  World
>  EOF
>  
> -test_expect_success 'run_command runs in parallel' '
> -	test-run-command run-command-parallel-4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
> +test_expect_success 'run_command runs in parallel with more jobs available than tasks' '
> +	test-run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'run_command runs in parallel with as many jobs as tasks' '
> +	test-run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'run_command runs in parallel with more tasks than jobs available' '
> +	test-run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -77,7 +87,7 @@ asking for a quick stop
>  EOF
>  
>  test_expect_success 'run_command is asked to abort gracefully' '
> -	test-run-command run-command-abort-3 false 2>actual &&
> +	test-run-command run-command-abort 3 false 2>actual &&
>  	test_cmp expect actual
>  '
>  
> diff --git a/test-run-command.c b/test-run-command.c
> index 4b59482..44710c3 100644
> --- a/test-run-command.c
> +++ b/test-run-command.c
> @@ -47,6 +47,7 @@ static int task_finished(int result,
>  int main(int argc, char **argv)
>  {
>  	struct child_process proc = CHILD_PROCESS_INIT;
> +	int jobs;
>  
>  	if (argc < 3)
>  		return 1;
> @@ -61,12 +62,15 @@ int main(int argc, char **argv)
>  	if (!strcmp(argv[1], "run-command"))
>  		exit(run_command(&proc));
>  
> -	if (!strcmp(argv[1], "run-command-parallel-4"))
> -		exit(run_processes_parallel(4, parallel_next,
> +	jobs = atoi(argv[2]);
> +	proc.argv = (const char **)argv+3;

	proc.argv = (const char **)argv + 3;

or

	proc.argv = (const char **)&argv[3];

Given the line immediately before refers to argv[2], the latter
might be easier on the eyes to follow.

In what way does this "Increase" test coverage?  By allowing the
caller to specify arbitrarily higher parallelism?

> +
> +	if (!strcmp(argv[1], "run-command-parallel"))
> +		exit(run_processes_parallel(jobs, parallel_next,
>  					    NULL, NULL, &proc));
>  
> -	if (!strcmp(argv[1], "run-command-abort-3"))
> -		exit(run_processes_parallel(3, parallel_next,
> +	if (!strcmp(argv[1], "run-command-abort"))
> +		exit(run_processes_parallel(jobs, parallel_next,
>  					    NULL, task_finished, &proc));
>  
>  	fprintf(stderr, "check usage\n");

Re: [PATCH 5/5] test-run-command: Increase test coverage

[Stefan Beller]

On Tue, Oct 20, 2015 at 11:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>         proc.argv = (const char **)argv + 3;
>
> or
>
>         proc.argv = (const char **)&argv[3];

ok, will fix

>
> Given the line immediately before refers to argv[2], the latter
> might be easier on the eyes to follow.
>
> In what way does this "Increase" test coverage?  By allowing the
> caller to specify arbitrarily higher parallelism?

Currently we have exact 4 jobs to be run with at most 4 parallel
processes. This sounds as if we're testing only one special case,
but in reality we want to have any number of tasks be processed
successfully, so the test I came up with is 3 cases
* more tasks than max jobs (implying reused slots)
* equal number of jobs and max jobs
* less tasks than max jobs (implying unused slots)

>
>> +
>> +     if (!strcmp(argv[1], "run-command-parallel"))
>> +             exit(run_processes_parallel(jobs, parallel_next,
>>                                           NULL, NULL, &proc));
>>
>> -     if (!strcmp(argv[1], "run-command-abort-3"))
>> -             exit(run_processes_parallel(3, parallel_next,
>> +     if (!strcmp(argv[1], "run-command-abort"))
>> +             exit(run_processes_parallel(jobs, parallel_next,
>>                                           NULL, task_finished, &proc));
>>
>>       fprintf(stderr, "check usage\n");

[PATCH 6/5] run-command: Fix missing output from late callbacks

[Stefan Beller]

The callbacks in the parallel processing API were given the contract, that
they are not allowed to print anything to stdout/err, but the API will take
care of their output needs instead.

In case a child process is started, the callback can first add its messages
to the buffer and then the child process output is buffered just in the
same buffer, and so the output will be taken care of eventually once the
child process is done.

When no child process is run, we also need to fulfill our promise to
output the buffer eventually. So when no child process is started, we need
to amend the contents of the buffer passed to the child to our buffer for
finished processes. We cannot output directly at that point in time as
another process may be in the middle of its output.

The buffer for finished child processes then also needs to be flushed in
the cleanup phase as it may contain data.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Another issue I just found. This goes on top of the series
    [PATCH 0/5] Fixes for the parallel processing
    
Thanks,
Stefan

 run-command.c          | 11 ++++++++++-
 t/t0061-run-command.sh |  9 +++++++++
 test-run-command.c     | 13 +++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 108b930..707e0a6 100644
--- a/run-command.c
+++ b/run-command.c
@@ -979,6 +979,12 @@ static void pp_cleanup(struct parallel_processes *pp)
 
 	free(pp->children);
 	free(pp->pfd);
+
+	/*
+	 * When get_next_task added messages to the buffer in its last
+	 * iteration, the buffered output is non empty.
+	 */
+	fputs(pp->buffered_output.buf, stderr);
 	strbuf_release(&pp->buffered_output);
 
 	sigchain_pop_common();
@@ -1016,8 +1022,11 @@ static int pp_start_one(struct parallel_processes *pp)
 	if (!pp->get_next_task(&pp->children[i].data,
 			       &pp->children[i].process,
 			       &pp->children[i].err,
-			       pp->data))
+			       pp->data)) {
+		strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
+		strbuf_reset(&pp->children[i].err);
 		return 1;
+	}
 
 	if (start_command(&pp->children[i].process)) {
 		int code = pp->start_failure(&pp->children[i].process,
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index f27ada7..12228b4 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -91,4 +91,13 @@ test_expect_success 'run_command is asked to abort gracefully' '
 	test_cmp expect actual
 '
 
+cat >expect <<-EOF
+no further jobs available
+EOF
+
+test_expect_success 'run_command outputs ' '
+	test-run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/test-run-command.c b/test-run-command.c
index 44710c3..91e94e8 100644
--- a/test-run-command.c
+++ b/test-run-command.c
@@ -34,6 +34,15 @@ static int parallel_next(void** task_cb,
 	return 1;
 }
 
+static int no_job(void** task_cb,
+		  struct child_process *cp,
+		  struct strbuf *err,
+		  void *cb)
+{
+	strbuf_addf(err, "no further jobs available\n");
+	return 0;
+}
+
 static int task_finished(int result,
 			 struct child_process *cp,
 			 struct strbuf *err,
@@ -73,6 +82,10 @@ int main(int argc, char **argv)
 		exit(run_processes_parallel(jobs, parallel_next,
 					    NULL, task_finished, &proc));
 
+	if (!strcmp(argv[1], "run-command-no-jobs"))
+		exit(run_processes_parallel(jobs, no_job,
+					    NULL, task_finished, &proc));
+
 	fprintf(stderr, "check usage\n");
 	return 1;
 }
-- 
2.5.0.273.gfd9d19f.dirty