Re: [PATCH 1/2] run-command: Remove set_nonblocking
Am 05.11.2015 um 19:17 schrieb Stefan Beller:
Show 10 quoted lines
> strbuf_read_once can also operate on blocking file descriptors if we are
> sure they are ready. The poll (2) command however makes sure this is the
> case.
>
> Reading the manual for poll (2), there may be spurious returns indicating
> readiness but that is for network sockets only. Pipes should be unaffected.
> By having this patch, we rely on the correctness of poll to return
> only pipes ready to read.
>
> This fixes compilation in Windows.
It certainly does (but I haven't tested, yet). But parallel processes will not work because we do not have a sufficiently complete waitpid emulation, yet. (waitpid(-1, ...) is not implemented.)
However, I think that the infrastructure can be simplified even further to a level that we do not need additional emulation on Windows.
First let me say that I find it very questionable that the callbacks receive a struct child_process. This is an implementation detail. It is also an implementation detail that stderr of the children is read and buffered, and that the child's stdout is redirected to stderr. It should not be the task of the get_next_task callback to set the members no_stdin, stdout_to_stderr, and err of struct child_process.
If you move that initialization to pp_start_one, you will notice rather sooner than later that the readable end of the file descriptor is never closed!
Which makes me think: Other users of start_command/finish_command work such that they
1. request a pipe by setting .out = -1
2. start_command
3. read from .out until EOF
4. close .out
5. wait for the process with finish_command
But the parallel_process infrastructure does not follow this pattern. It
1. requests a pipe by setting .err = -1
2. start_command
3. read from .err
4. wait for the process with waitpid
(and forgets to close .err). EOF is not in the picture (but that is not essential).
I suggest to change this such that we read from the children until EOF, mark them to be at their end of life, and then wait for them using finish_command (assuming that a process that closes stdout and stderr will die very soon if it is not already dead).
Here is a prototype patch. Feel free to pick it up. It marks a process whose EOF we have found by setting .err to -1. It's probably better to extend the meaning of the in_use indicator for this purpose. This seems to work on Linux with test-run-command with sub-processes that produce 100k output each:
./test-run-command run-command-parallel 5 sh -c "printf \"%0100000d\n\" 999"
although error handling would require some polishing according to t0061-run-command.
Show changes to run-command.c +16 −50
diff --git a/run-command.c b/run-command.c
index 51d078c..3e42299 100644
--- a/run-command.c
+++ b/run-command.c
@@ -977,7 +977,7 @@ static struct parallel_processes *pp_init(int n,
for (i = 0; i < n; i++) {
strbuf_init(&pp->children[i].err, 0);
child_process_init(&pp->children[i].process);
- pp->pfd[i].events = POLLIN;
+ pp->pfd[i].events = POLLIN|POLLHUP;
pp->pfd[i].fd = -1;
}
sigchain_push_common(handle_children_on_signal);
@@ -1061,11 +1061,17 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
/* Buffer output from all pipes. */
for (i = 0; i < pp->max_processes; i++) {
if (pp->children[i].in_use &&
- pp->pfd[i].revents & POLLIN)
- if (strbuf_read_once(&pp->children[i].err,
- pp->children[i].process.err, 0) < 0)
+ pp->pfd[i].revents & (POLLIN|POLLHUP)) {
+ int n = strbuf_read_once(&pp->children[i].err,
+ pp->children[i].process.err, 0);
+ if (n == 0) {
+ close(pp->children[i].process.err);
+ pp->children[i].process.err = -1;
+ } else if (n < 0) {
if (errno != EAGAIN)
die_errno("read");
+ }
+ }
}
}
@@ -1082,59 +1088,20 @@ static void pp_output(struct parallel_processes *pp)
static int pp_collect_finished(struct parallel_processes *pp)
{
int i = 0;
- pid_t pid;
- int wait_status, code;
+ int code;
int n = pp->max_processes;
int result = 0;
while (pp->nr_processes > 0) {
- pid = waitpid(-1, &wait_status, WNOHANG);
- if (pid == 0)
- break;
-
- if (pid < 0)
- die_errno("wait");
-
for (i = 0; i < pp->max_processes; i++)
if (pp->children[i].in_use &&
- pid == pp->children[i].process.pid)
+ pp->children[i].process.err == -1)
break;
+
if (i == pp->max_processes)
- die("BUG: found a child process we were not aware of");
-
- if (strbuf_read(&pp->children[i].err,
- pp->children[i].process.err, 0) < 0)
- die_errno("strbuf_read");
-
- if (WIFSIGNALED(wait_status)) {
- code = WTERMSIG(wait_status);
- if (!pp->shutdown &&
- code != SIGINT && code != SIGQUIT)
- strbuf_addf(&pp->children[i].err,
- "%s died of signal %d",
- pp->children[i].process.argv[0],
- code);
- /*
- * This return value is chosen so that code & 0xff
- * mimics the exit code that a POSIX shell would report for
- * a program that died from this signal.
- */
- code += 128;
- } else if (WIFEXITED(wait_status)) {
- code = WEXITSTATUS(wait_status);
- /*
- * Convert special exit code when execvp failed.
- */
- if (code == 127) {
- code = -1;
- errno = ENOENT;
- }
- } else {
- strbuf_addf(&pp->children[i].err,
- "waitpid is confused (%s)",
- pp->children[i].process.argv[0]);
- code = -1;
- }
+ break;
+
+ code = finish_command(&pp->children[i].process);
if (pp->task_finished(code, &pp->children[i].process,
&pp->children[i].err, pp->data,
@@ -1144,7 +1111,6 @@ static int pp_collect_finished(struct parallel_processes *pp)
pp->nr_processes--;
pp->children[i].in_use = 0;
pp->pfd[i].fd = -1;
- child_process_clear(&pp->children[i].process);
child_process_init(&pp->children[i].process);
if (i != pp->output_owner) {
Stefan Beller
