{"thread":{"id":"40746","subject":"[PATCH] run-command: detect finished children by closed pipe rather than waitpid","startedAt":"2015-11-06T23:48:57Z","lastAt":"2015-11-11T22:18:39Z","messageCount":7,"participants":["Stefan Beller","Torsten Bögershausen","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"273039","messageId":"1446853737-19047-1-git-send-email-sbeller@google.com","threadId":"40746","inReplyTo":null,"subject":"[PATCH] run-command: detect finished children by closed pipe rather than waitpid","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-06T23:48:57Z","receivedAt":"2015-11-06T23:48:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Detect if a child stopped working by checking if their stderr pipe\nwas closed instead of checking their state with waitpid.\nAs waitpid is not fully working in Windows, this is an approach which\nallows for better cross platform operation. (It's less code, too)\n\nPreviously we did not close the read pipe of finished children, which we\ndo now.\n\nThe old way missed some messages on an early abort. We just killed the\nchildren and did not bother to look what was left over. With this approach\nwe'd send a signal to the children and wait for them to close the pipe to\nhave all the messages (including possible \"killed by signal 15\" messages).\n\nTo have the test suite passing as before, we allow for real graceful\nabortion now. In case the user wishes to abort parallel execution\nthe user needs to provide either the signal used to kill all children\nor the children are let run until they finish normally.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Hi,\n \n this applis on top of origin/sb/submodule-parallel-fetch,\n making Windows folks possibly even more happy. It makes the code easier\n to read and has less races on cleaning up a terminated child.\n \n It follows the idea of Johannes patch, instead of encoding information in .err\n I removed the in_use flag and added a state, currently having 3 states.\n \n Thanks,\n Stefan\n\n Johannes schrieb:\n > First let me say that I find it very questionable that the callbacks\n > receive a struct child_process.\n \n I tried to get rid of the child_process struct in the callbacks, but that's\n not as easy as one may think. The submodule fetch code looks like this:\n \n get_next_submodule(..) {\n        ...\n        child_process_init(cp);\n        cp->dir = strbuf_detach(&submodule_path, NULL);\n        cp->env = local_repo_env;\n        cp->git_cmd = 1;\n        if (!spf->quiet)\n                strbuf_addf(err, \"Fetching submodule %s%s\\n\",\n                            spf->prefix, ce->name);\n        argv_array_init(&cp->args);\n        argv_array_pushv(&cp->args, spf->args.argv);\n        argv_array_push(&cp->args, default_argv);\n        argv_array_push(&cp->args, \"--submodule-prefix\");\n        argv_array_push(&cp->args, submodule_prefix.buf);\n }\n \n So we need to have access to the args, and git_cmd, environment variables\n and the directory where to run the command in. (So quite a lot of the\n internals already)\n \n\n run-command.c      | 140 +++++++++++++++++++++++------------------------------\n run-command.h      |  12 +++--\n submodule.c        |   3 --\n test-run-command.c |   3 --\n 4 files changed, 69 insertions(+), 89 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1fbd286..cf17baf 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -858,6 +858,12 @@ int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)\n \treturn finish_command(cmd);\n }\n \n+enum child_state {\n+\tFREE,\n+\tWORKING,\n+\tWAIT_CLEANUP,\n+};\n+\n static struct parallel_processes {\n \tvoid *data;\n \n@@ -869,7 +875,7 @@ static struct parallel_processes {\n \ttask_finished_fn task_finished;\n \n \tstruct {\n-\t\tunsigned in_use : 1;\n+\t\tenum child_state state;\n \t\tstruct child_process process;\n \t\tstruct strbuf err;\n \t\tvoid *data;\n@@ -923,7 +929,7 @@ static void kill_children(struct parallel_processes *pp, int signo)\n \tint i, n = pp->max_processes;\n \n \tfor (i = 0; i < n; i++)\n-\t\tif (pp->children[i].in_use)\n+\t\tif (pp->children[i].state == WORKING)\n \t\t\tkill(pp->children[i].process.pid, signo);\n }\n \n@@ -967,7 +973,7 @@ static struct parallel_processes *pp_init(int n,\n \tfor (i = 0; i < n; i++) {\n \t\tstrbuf_init(&pp->children[i].err, 0);\n \t\tchild_process_init(&pp->children[i].process);\n-\t\tpp->pfd[i].events = POLLIN;\n+\t\tpp->pfd[i].events = POLLIN | POLLHUP;\n \t\tpp->pfd[i].fd = -1;\n \t}\n \tsigchain_push_common(handle_children_on_signal);\n@@ -1011,41 +1017,48 @@ static void set_nonblocking(int fd)\n  *  0 if a new task was started.\n  *  1 if no new jobs was started (get_next_task ran out of work, non critical\n  *    problem with starting a new command)\n- * -1 no new job was started, user wishes to shutdown early.\n+ * <0 no new job was started, user wishes to shutdown early. Use negative code\n+ *    to signal the children.\n  */\n static int pp_start_one(struct parallel_processes *pp)\n {\n-\tint i;\n+\tint i, code;\n \n \tfor (i = 0; i < pp->max_processes; i++)\n-\t\tif (!pp->children[i].in_use)\n+\t\tif (pp->children[i].state == FREE)\n \t\t\tbreak;\n \tif (i == pp->max_processes)\n \t\tdie(\"BUG: bookkeeping is hard\");\n \n-\tif (!pp->get_next_task(&pp->children[i].data,\n-\t\t\t       &pp->children[i].process,\n-\t\t\t       &pp->children[i].err,\n-\t\t\t       pp->data)) {\n+\tcode = pp->get_next_task(&pp->children[i].data,\n+\t\t\t\t &pp->children[i].process,\n+\t\t\t\t &pp->children[i].err,\n+\t\t\t\t pp->data);\n+\tif (!code) {\n \t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n \t\tstrbuf_reset(&pp->children[i].err);\n \t\treturn 1;\n \t}\n+\tpp->children[i].process.err = -1;\n+\tpp->children[i].process.stdout_to_stderr = 1;\n+\tpp->children[i].process.no_stdin = 1;\n \n \tif (start_command(&pp->children[i].process)) {\n-\t\tint code = pp->start_failure(&pp->children[i].process,\n-\t\t\t\t\t     &pp->children[i].err,\n-\t\t\t\t\t     pp->data,\n-\t\t\t\t\t     &pp->children[i].data);\n+\t\tcode = pp->start_failure(&pp->children[i].process,\n+\t\t\t\t\t &pp->children[i].err,\n+\t\t\t\t\t pp->data,\n+\t\t\t\t\t &pp->children[i].data);\n \t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n \t\tstrbuf_reset(&pp->children[i].err);\n-\t\treturn code ? -1 : 1;\n+\t\tif (code)\n+\t\t\tpp->shutdown = 1;\n+\t\treturn code;\n \t}\n \n \tset_nonblocking(pp->children[i].process.err);\n \n \tpp->nr_processes++;\n-\tpp->children[i].in_use = 1;\n+\tpp->children[i].state = WORKING;\n \tpp->pfd[i].fd = pp->children[i].process.err;\n \treturn 0;\n }\n@@ -1063,19 +1076,24 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n \n \t/* Buffer output from all pipes. */\n \tfor (i = 0; i < pp->max_processes; i++) {\n-\t\tif (pp->children[i].in_use &&\n-\t\t    pp->pfd[i].revents & POLLIN)\n-\t\t\tif (strbuf_read_once(&pp->children[i].err,\n-\t\t\t\t\t     pp->children[i].process.err, 0) < 0)\n+\t\tif (pp->children[i].state == WORKING &&\n+\t\t    pp->pfd[i].revents & (POLLIN | POLLHUP)) {\n+\t\t\tint n = strbuf_read_once(&pp->children[i].err,\n+\t\t\t\t\t\t pp->children[i].process.err, 0);\n+\t\t\tif (n == 0) {\n+\t\t\t\tclose(pp->children[i].process.err);\n+\t\t\t\tpp->children[i].state = WAIT_CLEANUP;\n+\t\t\t} else if (n < 0)\n \t\t\t\tif (errno != EAGAIN)\n \t\t\t\t\tdie_errno(\"read\");\n+\t\t}\n \t}\n }\n \n static void pp_output(struct parallel_processes *pp)\n {\n \tint i = pp->output_owner;\n-\tif (pp->children[i].in_use &&\n+\tif (pp->children[i].state == WORKING &&\n \t    pp->children[i].err.len) {\n \t\tfputs(pp->children[i].err.buf, stderr);\n \t\tstrbuf_reset(&pp->children[i].err);\n@@ -1084,68 +1102,30 @@ static void pp_output(struct parallel_processes *pp)\n \n static int pp_collect_finished(struct parallel_processes *pp)\n {\n-\tint i = 0;\n-\tpid_t pid;\n-\tint wait_status, code;\n+\tint i, code;\n \tint n = pp->max_processes;\n \tint result = 0;\n \n \twhile (pp->nr_processes > 0) {\n-\t\tpid = waitpid(-1, &wait_status, WNOHANG);\n-\t\tif (pid == 0)\n-\t\t\tbreak;\n-\n-\t\tif (pid < 0)\n-\t\t\tdie_errno(\"wait\");\n-\n \t\tfor (i = 0; i < pp->max_processes; i++)\n-\t\t\tif (pp->children[i].in_use &&\n-\t\t\t    pid == pp->children[i].process.pid)\n+\t\t\tif (pp->children[i].state == WAIT_CLEANUP)\n \t\t\t\tbreak;\n \t\tif (i == pp->max_processes)\n-\t\t\tdie(\"BUG: found a child process we were not aware of\");\n-\n-\t\tif (strbuf_read(&pp->children[i].err,\n-\t\t\t\tpp->children[i].process.err, 0) < 0)\n-\t\t\tdie_errno(\"strbuf_read\");\n-\n-\t\tif (WIFSIGNALED(wait_status)) {\n-\t\t\tcode = WTERMSIG(wait_status);\n-\t\t\tif (!pp->shutdown &&\n-\t\t\t    code != SIGINT && code != SIGQUIT)\n-\t\t\t\tstrbuf_addf(&pp->children[i].err,\n-\t\t\t\t\t    \"%s died of signal %d\",\n-\t\t\t\t\t    pp->children[i].process.argv[0],\n-\t\t\t\t\t    code);\n-\t\t\t/*\n-\t\t\t * This return value is chosen so that code & 0xff\n-\t\t\t * mimics the exit code that a POSIX shell would report for\n-\t\t\t * a program that died from this signal.\n-\t\t\t */\n-\t\t\tcode += 128;\n-\t\t} else if (WIFEXITED(wait_status)) {\n-\t\t\tcode = WEXITSTATUS(wait_status);\n-\t\t\t/*\n-\t\t\t * Convert special exit code when execvp failed.\n-\t\t\t */\n-\t\t\tif (code == 127) {\n-\t\t\t\tcode = -1;\n-\t\t\t\terrno = ENOENT;\n-\t\t\t}\n-\t\t} else {\n-\t\t\tstrbuf_addf(&pp->children[i].err,\n-\t\t\t\t    \"waitpid is confused (%s)\",\n-\t\t\t\t    pp->children[i].process.argv[0]);\n-\t\t\tcode = -1;\n-\t\t}\n+\t\t\tbreak;\n+\n+\t\tcode = finish_command(&pp->children[i].process);\n \n-\t\tif (pp->task_finished(code, &pp->children[i].process,\n-\t\t\t\t      &pp->children[i].err, pp->data,\n-\t\t\t\t      &pp->children[i].data))\n-\t\t\tresult = 1;\n+\t\tcode = pp->task_finished(code, &pp->children[i].process,\n+\t\t\t\t\t &pp->children[i].err, pp->data,\n+\t\t\t\t\t &pp->children[i].data);\n+\n+\t\tif (code)\n+\t\t\tresult = code;\n+\t\tif (code < 0)\n+\t\t\tbreak;\n \n \t\tpp->nr_processes--;\n-\t\tpp->children[i].in_use = 0;\n+\t\tpp->children[i].state = FREE;\n \t\tpp->pfd[i].fd = -1;\n \t\tchild_process_deinit(&pp->children[i].process);\n \t\tchild_process_init(&pp->children[i].process);\n@@ -1170,7 +1150,7 @@ static int pp_collect_finished(struct parallel_processes *pp)\n \t\t\t * running process time.\n \t\t\t */\n \t\t\tfor (i = 0; i < n; i++)\n-\t\t\t\tif (pp->children[(pp->output_owner + i) % n].in_use)\n+\t\t\t\tif (pp->children[(pp->output_owner + i) % n].state == WORKING)\n \t\t\t\t\tbreak;\n \t\t\tpp->output_owner = (pp->output_owner + i) % n;\n \t\t}\n@@ -1184,7 +1164,7 @@ int run_processes_parallel(int n,\n \t\t\t   task_finished_fn task_finished,\n \t\t\t   void *pp_cb)\n {\n-\tint i;\n+\tint i, code;\n \tint output_timeout = 100;\n \tint spawn_cap = 4;\n \tstruct parallel_processes *pp;\n@@ -1195,12 +1175,12 @@ int run_processes_parallel(int n,\n \t\t    i < spawn_cap && !pp->shutdown &&\n \t\t    pp->nr_processes < pp->max_processes;\n \t\t    i++) {\n-\t\t\tint code = pp_start_one(pp);\n+\t\t\tcode = pp_start_one(pp);\n \t\t\tif (!code)\n \t\t\t\tcontinue;\n \t\t\tif (code < 0) {\n \t\t\t\tpp->shutdown = 1;\n-\t\t\t\tkill_children(pp, SIGTERM);\n+\t\t\t\tkill_children(pp, -code);\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n@@ -1208,9 +1188,11 @@ int run_processes_parallel(int n,\n \t\t\tbreak;\n \t\tpp_buffer_stderr(pp, output_timeout);\n \t\tpp_output(pp);\n-\t\tif (pp_collect_finished(pp)) {\n-\t\t\tkill_children(pp, SIGTERM);\n+\t\tcode = pp_collect_finished(pp);\n+\t\tif (code) {\n \t\t\tpp->shutdown = 1;\n+\t\t\tif (code < 0)\n+\t\t\t\tkill_children(pp, -code);\n \t\t}\n \t}\n \ndiff --git a/run-command.h b/run-command.h\nindex c24aa54..414cc81 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -134,6 +134,8 @@ int finish_async(struct async *async);\n  *\n  * Return 1 if the next child is ready to run.\n  * Return 0 if there are currently no more tasks to be processed.\n+ * To send a signal to other child processes for abortion,\n+ * return negative signal code.\n  */\n typedef int (*get_next_task_fn)(void **pp_task_cb,\n \t\t\t\tstruct child_process *cp,\n@@ -151,8 +153,9 @@ typedef int (*get_next_task_fn)(void **pp_task_cb,\n  * pp_cb is the callback cookie as passed into run_processes_parallel,\n  * pp_task_cb is the callback cookie as passed into get_next_task_fn.\n  *\n- * Return 0 to continue the parallel processing. To abort gracefully,\n- * return non zero.\n+ * Return 0 to continue the parallel processing. To abort return non zero.\n+ * To send a signal to other child processes for abortion, return\n+ * negative signal code.\n  */\n typedef int (*start_failure_fn)(struct child_process *cp,\n \t\t\t\tstruct strbuf *err,\n@@ -169,8 +172,9 @@ typedef int (*start_failure_fn)(struct child_process *cp,\n  * pp_cb is the callback cookie as passed into run_processes_parallel,\n  * pp_task_cb is the callback cookie as passed into get_next_task_fn.\n  *\n- * Return 0 to continue the parallel processing. To abort gracefully,\n- * return non zero.\n+ * Return 0 to continue the parallel processing.  To abort return non zero.\n+ * To send a signal to other child processes for abortion, return\n+ * negative signal code.\n  */\n typedef int (*task_finished_fn)(int result,\n \t\t\t\tstruct child_process *cp,\ndiff --git a/submodule.c b/submodule.c\nindex c21b265..281bccd 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -689,9 +689,6 @@ static int get_next_submodule(void **task_cb, struct child_process *cp,\n \t\t\tcp->dir = strbuf_detach(&submodule_path, NULL);\n \t\t\tcp->env = local_repo_env;\n \t\t\tcp->git_cmd = 1;\n-\t\t\tcp->no_stdin = 1;\n-\t\t\tcp->stdout_to_stderr = 1;\n-\t\t\tcp->err = -1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, \"Fetching submodule %s%s\\n\",\n \t\t\t\t\t    spf->prefix, ce->name);\ndiff --git a/test-run-command.c b/test-run-command.c\nindex 13e5d44..b1f04d1 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -26,9 +26,6 @@ static int parallel_next(void** task_cb,\n \t\treturn 0;\n \n \targv_array_pushv(&cp->args, d->argv);\n-\tcp->stdout_to_stderr = 1;\n-\tcp->no_stdin = 1;\n-\tcp->err = -1;\n \tstrbuf_addf(err, \"preloaded output of a child\\n\");\n \tnumber_callbacks++;\n \treturn 1;\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"273045","messageId":"563D9A0E.8050405@web.de","threadId":"40746","inReplyTo":"1446853737-19047-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] run-command: detect finished children by closed pipe rather than waitpid","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-11-07T06:28:30Z","receivedAt":"2015-11-07T06:28:30Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 07.11.15 00:48, Stefan Beller wrote:\n>  \n> +enum child_state {\n> +\tFREE,\n> +\tWORKING,\n> +\tWAIT_CLEANUP,\n> +};\n\nThis kind of  \"generic names\" feels are begging for \nname clash, some day, may be in the future.\nHow about something like this:\n> +\tGIT_CP_FREE,\n> +\tGIT_CP_WORKING,\n> +\tGIT_CP_WAIT_CLEANUP,\n"},{"id":"273046","messageId":"563DBDDA.2000106@kdbg.org","threadId":"40746","inReplyTo":"1446853737-19047-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] run-command: detect finished children by closed pipe rather than waitpid","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-11-07T09:01:14Z","receivedAt":"2015-11-07T09:01:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.11.2015 um 00:48 schrieb Stefan Beller:\n> Detect if a child stopped working by checking if their stderr pipe\n> was closed instead of checking their state with waitpid.\n> As waitpid is not fully working in Windows, this is an approach which\n> allows for better cross platform operation. (It's less code, too)\n>\n> Previously we did not close the read pipe of finished children, which we\n> do now.\n>\n> The old way missed some messages on an early abort. We just killed the\n> children and did not bother to look what was left over. With this approach\n> we'd send a signal to the children and wait for them to close the pipe to\n> have all the messages (including possible \"killed by signal 15\" messages).\n>\n> To have the test suite passing as before, we allow for real graceful\n> abortion now. In case the user wishes to abort parallel execution\n> the user needs to provide either the signal used to kill all children\n> or the children are let run until they finish normally.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>   Hi,\n>\n>   this applis on top of origin/sb/submodule-parallel-fetch,\n>   making Windows folks possibly even more happy. It makes the code easier\n>   to read and has less races on cleaning up a terminated child.\n>\n>   It follows the idea of Johannes patch, instead of encoding information in .err\n>   I removed the in_use flag and added a state, currently having 3 states.\n>\n>   Thanks,\n>   Stefan\n>\n>   Johannes schrieb:\n>   > First let me say that I find it very questionable that the callbacks\n>   > receive a struct child_process.\n>\n>   I tried to get rid of the child_process struct in the callbacks, but that's\n>   not as easy as one may think.\n\nFair enough. I see you removed .err, .no_stdin and .stdout_to_stderr \nfrom the callback. Good.\n\n>   \t\tpp->nr_processes--;\n> -\t\tpp->children[i].in_use = 0;\n> +\t\tpp->children[i].state = FREE;\n>   \t\tpp->pfd[i].fd = -1;\n>   \t\tchild_process_deinit(&pp->children[i].process);\n\nThis cleanup is implied by finish_command and can be removed.\n\n>   \t\tchild_process_init(&pp->children[i].process);\n\n> @@ -1195,12 +1175,12 @@ int run_processes_parallel(int n,\n>   \t\t    i < spawn_cap && !pp->shutdown &&\n>   \t\t    pp->nr_processes < pp->max_processes;\n>   \t\t    i++) {\n> -\t\t\tint code = pp_start_one(pp);\n> +\t\t\tcode = pp_start_one(pp);\n>   \t\t\tif (!code)\n>   \t\t\t\tcontinue;\n>   \t\t\tif (code < 0) {\n>   \t\t\t\tpp->shutdown = 1;\n> -\t\t\t\tkill_children(pp, SIGTERM);\n> +\t\t\t\tkill_children(pp, -code);\n\nI'll see what this means for our kill emulation on Windows. Currently, \nwe handle only SIGTERM.\n\n>   \t\t\t}\n>   \t\t\tbreak;\n>   \t\t}\n\nThanks you very much!\n\n-- Hannes\n"},{"id":"273224","messageId":"CAGZ79kaZ0W5q8=iowbSTJY_mDNNYc6qdTszcWDDDCtYfZyeK_Q@mail.gmail.com","threadId":"40746","inReplyTo":"563DBDDA.2000106@kdbg.org","subject":"Re: [PATCH] run-command: detect finished children by closed pipe rather than waitpid","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-11T20:37:16Z","receivedAt":"2015-11-11T20:37:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"including the list and all others this time.\n\n>>                         if (code < 0) {\n>>                                 pp->shutdown = 1;\n>> -                               kill_children(pp, SIGTERM);\n>> +                               kill_children(pp, -code);\n>\n>\n> I'll see what this means for our kill emulation on Windows. Currently, we\n> handle only SIGTERM.\n\nSo currently we only pass in SIGTERM from the callers, and I certainly\nonly intend\nto use that signal. I just thought special casing the SIGTERM signal\nwould do no good\nin terms of design here.\n\nSo maybe that was not the right thought and we do have to special case\nSIGTERM here?\n\nI worked with another large proprietary parallel program (spawning\nchild processes) before.\nIf you pressed CTRL-C, it would omit a message like:\n\n    Sending no further input to the children\n\nIf you press Ctrl-C again, it would print:\n\n    Sending SIGTERM to children, to shut them down gracefully\n\nIf you pressed once again, you'll get a\n\n    User seems to be angry, shutting down hard, sending SIGKILL\n    $ # that was fast now.\n\nI guess we don't want to play around like that, but if the parent process gets\na SIGTERM, we relay that and other signals we ignore for now?\n"},{"id":"273226","messageId":"5643A9A0.50105@kdbg.org","threadId":"40746","inReplyTo":"CAGZ79kaZ0W5q8=iowbSTJY_mDNNYc6qdTszcWDDDCtYfZyeK_Q@mail.gmail.com","subject":"Re: [PATCH] run-command: detect finished children by closed pipe rather than waitpid","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-11-11T20:48:32Z","receivedAt":"2015-11-11T20:48:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.11.2015 um 21:37 schrieb Stefan Beller:\n> including the list and all others this time.\n>\n>>>                          if (code < 0) {\n>>>                                  pp->shutdown = 1;\n>>> -                               kill_children(pp, SIGTERM);\n>>> +                               kill_children(pp, -code);\n>>\n>>\n>> I'll see what this means for our kill emulation on Windows. Currently, we\n>> handle only SIGTERM.\n>\n> So currently we only pass in SIGTERM from the callers, and I certainly\n> only intend\n> to use that signal. I just thought special casing the SIGTERM signal\n> would do no good\n> in terms of design here.\n>\n> So maybe that was not the right thought and we do have to special case\n> SIGTERM here?\n\nI wonder why task_finish() callback gets to choose a signal. The point \nhere is, IIUC, when one child dies, the others must be halted, too. \nSIGTERM seems to be the only sensible choice.\n\n-- Hannes\n"},{"id":"273227","messageId":"CAGZ79kZTiGGL0t-CkeuWDfzX-pK_Lu8PZzE1sutq2jR_23B5xg@mail.gmail.com","threadId":"40746","inReplyTo":"5643A9A0.50105@kdbg.org","subject":"Re: [PATCH] run-command: detect finished children by closed pipe rather than waitpid","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-11T20:53:05Z","receivedAt":"2015-11-11T20:53:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 11, 2015 at 12:48 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>>\n>> So maybe that was not the right thought and we do have to special case\n>> SIGTERM here?\n>\n>\n> I wonder why task_finish() callback gets to choose a signal. The point here\n> is, IIUC, when one child dies, the others must be halted, too. SIGTERM seems\n> to be the only sensible choice.\n\nSIGKILL would also do?\n\nIn case you know your children, you can also send a SIGUSR1 or SIGUSR2.\n\nOr if you want to quit the top level program, but want to keep going with some\nof the children (repacking, garbage collection, networking stuff), you\nway want to\ndecouple them using SIGHUP ?\n\nSo I am not convinced SIGTERM is the only true choice here. And because I\nhave no idea which of the signals may be useful in the future, I decided to\ngo with all of them.\n"},{"id":"273232","messageId":"5643BEBF.6000804@kdbg.org","threadId":"40746","inReplyTo":"CAGZ79kZTiGGL0t-CkeuWDfzX-pK_Lu8PZzE1sutq2jR_23B5xg@mail.gmail.com","subject":"Re: [PATCH] run-command: detect finished children by closed pipe rather than waitpid","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-11-11T22:18:39Z","receivedAt":"2015-11-11T22:18:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.11.2015 um 21:53 schrieb Stefan Beller:\n> On Wed, Nov 11, 2015 at 12:48 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>> I wonder why task_finish() callback gets to choose a signal. The point here\n>> is, IIUC, when one child dies, the others must be halted, too. SIGTERM seems\n>> to be the only sensible choice.\n>\n> SIGKILL would also do?\n>\n> In case you know your children, you can also send a SIGUSR1 or SIGUSR2.\n>...\n> So I am not convinced SIGTERM is the only true choice here. And because I\n> have no idea which of the signals may be useful in the future, I decided to\n> go with all of them.\n\nFair enough.\n\n-- Hannes\n"}]}