threads / patch / 40589

patch, 5 partsFixes for the parallel processing

Subject: [PATCH 0/5] Fixes for the parallel processing

## tl;dr

12 messages between Oct 19, 2015 and Oct 20, 2015. Diffs are folded; open one to read it.

replies: 11people: 2as markdown or json

Stefan Beller· Oct 19, 2015, 18:24 UTC · lore

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
Stefan Beller· Oct 19, 2015, 18:24 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to run-command.c +1 −1
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
Junio C Hamano· Oct 20, 2015, 18:41 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 6 quoted lines
> 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.
Show 16 quoted lines
>  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");
Stefan Beller· Oct 19, 2015, 18:24 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to run-command.c +2 −0
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
Junio C Hamano· Oct 20, 2015, 18:49 UTC · re: Stefan Beller · lore

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

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"
Show 7 quoted lines
> 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?

Show 16 quoted lines
>  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,
Stefan Beller· Oct 20, 2015, 19:07 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 20, 2015 at 11:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> 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.

Show 17 quoted lines
>
>>  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,
Stefan Beller· Oct 19, 2015, 18:24 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to run-command.c +1 −0
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
Stefan Beller· Oct 19, 2015, 18:24 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to 2 files +28 −0

t/t0061-run-command.sh, test-run-command.c

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
Stefan Beller· Oct 19, 2015, 18:24 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 2 files +21 −7

t/t0061-run-command.sh, test-run-command.c

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
Junio C Hamano· Oct 20, 2015, 18:53 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 60 quoted lines
> 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?

Show 12 quoted lines
> +
> +	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");
Stefan Beller· Oct 20, 2015, 19:05 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 20, 2015 at 11:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
>
>         proc.argv = (const char **)argv + 3;
>
> or
>
>         proc.argv = (const char **)&argv[3];
ok, will fix
Show 6 quoted lines
>
> 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)
Show 13 quoted lines
>
>> +
>> +     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");
Stefan Beller· Oct 20, 2015, 17:51 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 3 files +32 −1

run-command.c, t/t0061-run-command.sh, test-run-command.c

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

← back to recent threads