{"thread":{"id":"41011","subject":"[PATCHv2 0/7] Rerolling sb/submodule-parallel-fetch for the time after 2.7","startedAt":"2015-12-16T00:04:05Z","lastAt":"2015-12-18T18:12:33Z","messageCount":19,"participants":["Stefan Beller","Junio C Hamano","Torsten Bögershausen","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"274529","messageId":"1450224252-16833-1-git-send-email-sbeller@google.com","threadId":"41011","inReplyTo":null,"subject":"[PATCHv2 0/7] Rerolling sb/submodule-parallel-fetch for the time after 2.7","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-16T00:04:05Z","receivedAt":"2015-12-16T00:04:05Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I am sending out a new version for replacing sb/submodule-parallel-fetch for\nthe time after the 2.7 release.\n\n* Dropped the patch, which introduces xread_nonblock\n* strbuf_read_once uses xread now. This is safe as we poll before using\n  strbuf_read_once, so we know we won't stall.\n* have the commit message reworded for \"run-command: add an asynchronous parallel child processor\"\n  with Johannes' suggestion.\n  \n  Thanks,\n  Stefan\n\nJonathan Nieder (1):\n  submodule.c: write \"Fetching submodule <foo>\" to stderr\n\nStefan Beller (6):\n  xread: poll on non blocking fds\n  strbuf: add strbuf_read_once to read without blocking\n  sigchain: add command to pop all common signals\n  run-command: add an asynchronous parallel child processor\n  fetch_populated_submodules: use new parallel job processing\n  submodules: allow parallel fetching, add tests and documentation\n\n Documentation/fetch-options.txt |   7 +\n builtin/fetch.c                 |   6 +-\n builtin/pull.c                  |   6 +\n run-command.c                   | 335 ++++++++++++++++++++++++++++++++++++++++\n run-command.h                   |  80 ++++++++++\n sigchain.c                      |   9 ++\n sigchain.h                      |   1 +\n strbuf.c                        |  11 ++\n strbuf.h                        |   8 +\n submodule.c                     | 141 +++++++++++------\n submodule.h                     |   2 +-\n t/t0061-run-command.sh          |  53 +++++++\n t/t5526-fetch-submodules.sh     |  71 ++++++---\n test-run-command.c              |  55 ++++++-\n wrapper.c                       |  20 ++-\n 15 files changed, 731 insertions(+), 74 deletions(-)\n\n-- \n2.6.4.443.ge094245.dirty\n"},{"id":"274532","messageId":"1450224252-16833-2-git-send-email-sbeller@google.com","threadId":"41011","inReplyTo":"1450224252-16833-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 1/7] submodule.c: write \"Fetching submodule <foo>\" to stderr","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-16T00:04:06Z","receivedAt":"2015-12-16T00:04:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Jonathan Nieder <jrnieder@gmail.com>\n\nThe \"Pushing submodule <foo>\" progress output correctly goes to\nstderr, but \"Fetching submodule <foo>\" is going to stdout by\nmistake.  Fix it to write to stderr.\n\nNoticed while trying to implement a parallel submodule fetch.  When\nthis particular output line went to a different file descriptor, it\nwas buffered separately, resulting in wrongly interleaved output if\nwe copied it to the terminal naively.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n submodule.c                 |  2 +-\n t/t5526-fetch-submodules.sh | 51 +++++++++++++++++++++++----------------------\n 2 files changed, 27 insertions(+), 26 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 14e7624..8386477 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -689,7 +689,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\tgit_dir = submodule_git_dir.buf;\n \t\tif (is_directory(git_dir)) {\n \t\t\tif (!quiet)\n-\t\t\t\tprintf(\"Fetching submodule %s%s\\n\", prefix, ce->name);\n+\t\t\t\tfprintf(stderr, \"Fetching submodule %s%s\\n\", prefix, ce->name);\n \t\t\tcp.dir = submodule_path.buf;\n \t\t\targv_array_push(&argv, default_argv);\n \t\t\targv_array_push(&argv, \"--submodule-prefix\");\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex a4532b0..17759b14 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -16,7 +16,8 @@ add_upstream_commit() {\n \t\tgit add subfile &&\n \t\tgit commit -m new subfile &&\n \t\thead2=$(git rev-parse --short HEAD) &&\n-\t\techo \"From $pwd/submodule\" > ../expect.err &&\n+\t\techo \"Fetching submodule submodule\" > ../expect.err &&\n+\t\techo \"From $pwd/submodule\" >> ../expect.err &&\n \t\techo \"   $head1..$head2  master     -> origin/master\" >> ../expect.err\n \t) &&\n \t(\n@@ -27,6 +28,7 @@ add_upstream_commit() {\n \t\tgit add deepsubfile &&\n \t\tgit commit -m new deepsubfile &&\n \t\thead2=$(git rev-parse --short HEAD) &&\n+\t\techo \"Fetching submodule submodule/subdir/deepsubmodule\" >> ../expect.err\n \t\techo \"From $pwd/deepsubmodule\" >> ../expect.err &&\n \t\techo \"   $head1..$head2  master     -> origin/master\" >> ../expect.err\n \t)\n@@ -56,9 +58,7 @@ test_expect_success setup '\n \t(\n \t\tcd downstream &&\n \t\tgit submodule update --init --recursive\n-\t) &&\n-\techo \"Fetching submodule submodule\" > expect.out &&\n-\techo \"Fetching submodule submodule/subdir/deepsubmodule\" >> expect.out\n+\t)\n '\n \n test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n@@ -67,7 +67,7 @@ test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n \t\tcd downstream &&\n \t\tgit fetch --recurse-submodules >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -96,7 +96,7 @@ test_expect_success \"using fetchRecurseSubmodules=true in .gitmodules recurses i\n \t\tgit config -f .gitmodules submodule.submodule.fetchRecurseSubmodules true &&\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -127,7 +127,7 @@ test_expect_success \"--recurse-submodules overrides fetchRecurseSubmodules setti\n \t\tgit config --unset -f .gitmodules submodule.submodule.fetchRecurseSubmodules &&\n \t\tgit config --unset submodule.submodule.fetchRecurseSubmodules\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -146,7 +146,7 @@ test_expect_success \"--dry-run propagates to submodules\" '\n \t\tcd downstream &&\n \t\tgit fetch --recurse-submodules --dry-run >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -155,7 +155,7 @@ test_expect_success \"Without --dry-run propagates to submodules\" '\n \t\tcd downstream &&\n \t\tgit fetch --recurse-submodules >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -166,7 +166,7 @@ test_expect_success \"recurseSubmodules=true propagates into submodules\" '\n \t\tgit config fetch.recurseSubmodules true\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -180,7 +180,7 @@ test_expect_success \"--recurse-submodules overrides config in submodule\" '\n \t\t) &&\n \t\tgit fetch --recurse-submodules >../actual.out 2>../actual.err\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -214,16 +214,15 @@ test_expect_success \"Recursion stops when no new submodule commits are fetched\"\n \tgit add submodule &&\n \tgit commit -m \"new submodule\" &&\n \thead2=$(git rev-parse --short HEAD) &&\n-\techo \"Fetching submodule submodule\" > expect.out.sub &&\n \techo \"From $pwd/.\" > expect.err.sub &&\n \techo \"   $head1..$head2  master     -> origin/master\" >>expect.err.sub &&\n-\thead -2 expect.err >> expect.err.sub &&\n+\thead -3 expect.err >> expect.err.sub &&\n \t(\n \t\tcd downstream &&\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n \ttest_i18ncmp expect.err.sub actual.err &&\n-\ttest_i18ncmp expect.out.sub actual.out\n+\ttest_must_be_empty actual.out\n '\n \n test_expect_success \"Recursion doesn't happen when new superproject commits don't change any submodules\" '\n@@ -269,7 +268,7 @@ test_expect_success \"Recursion picks up config in submodule\" '\n \t\t)\n \t) &&\n \ttest_i18ncmp expect.err.sub actual.err &&\n-\ttest_i18ncmp expect.out actual.out\n+\ttest_must_be_empty actual.out\n '\n \n test_expect_success \"Recursion picks up all submodules when necessary\" '\n@@ -285,7 +284,8 @@ test_expect_success \"Recursion picks up all submodules when necessary\" '\n \t\tgit add subdir/deepsubmodule &&\n \t\tgit commit -m \"new deepsubmodule\"\n \t\thead2=$(git rev-parse --short HEAD) &&\n-\t\techo \"From $pwd/submodule\" > ../expect.err.sub &&\n+\t\techo \"Fetching submodule submodule\" > ../expect.err.sub &&\n+\t\techo \"From $pwd/submodule\" >> ../expect.err.sub &&\n \t\techo \"   $head1..$head2  master     -> origin/master\" >> ../expect.err.sub\n \t) &&\n \thead1=$(git rev-parse --short HEAD) &&\n@@ -295,13 +295,13 @@ test_expect_success \"Recursion picks up all submodules when necessary\" '\n \techo \"From $pwd/.\" > expect.err.2 &&\n \techo \"   $head1..$head2  master     -> origin/master\" >> expect.err.2 &&\n \tcat expect.err.sub >> expect.err.2 &&\n-\ttail -2 expect.err >> expect.err.2 &&\n+\ttail -3 expect.err >> expect.err.2 &&\n \t(\n \t\tcd downstream &&\n \t\tgit fetch >../actual.out 2>../actual.err\n \t) &&\n \ttest_i18ncmp expect.err.2 actual.err &&\n-\ttest_i18ncmp expect.out actual.out\n+\ttest_must_be_empty actual.out\n '\n \n test_expect_success \"'--recurse-submodules=on-demand' doesn't recurse when no new commits are fetched in the superproject (and ignores config)\" '\n@@ -317,7 +317,8 @@ test_expect_success \"'--recurse-submodules=on-demand' doesn't recurse when no ne\n \t\tgit add subdir/deepsubmodule &&\n \t\tgit commit -m \"new deepsubmodule\" &&\n \t\thead2=$(git rev-parse --short HEAD) &&\n-\t\techo \"From $pwd/submodule\" > ../expect.err.sub &&\n+\t\techo Fetching submodule submodule > ../expect.err.sub &&\n+\t\techo \"From $pwd/submodule\" >> ../expect.err.sub &&\n \t\techo \"   $head1..$head2  master     -> origin/master\" >> ../expect.err.sub\n \t) &&\n \t(\n@@ -335,7 +336,7 @@ test_expect_success \"'--recurse-submodules=on-demand' recurses as deep as necess\n \tgit add submodule &&\n \tgit commit -m \"new submodule\" &&\n \thead2=$(git rev-parse --short HEAD) &&\n-\ttail -2 expect.err > expect.err.deepsub &&\n+\ttail -3 expect.err > expect.err.deepsub &&\n \techo \"From $pwd/.\" > expect.err &&\n \techo \"   $head1..$head2  master     -> origin/master\" >>expect.err &&\n \tcat expect.err.sub >> expect.err &&\n@@ -354,7 +355,7 @@ test_expect_success \"'--recurse-submodules=on-demand' recurses as deep as necess\n \t\t\tgit config --unset -f .gitmodules submodule.subdir/deepsubmodule.fetchRecursive\n \t\t)\n \t) &&\n-\ttest_i18ncmp expect.out actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err\n '\n \n@@ -388,7 +389,7 @@ test_expect_success \"'fetch.recurseSubmodules=on-demand' overrides global config\n \thead2=$(git rev-parse --short HEAD) &&\n \techo \"From $pwd/.\" > expect.err.2 &&\n \techo \"   $head1..$head2  master     -> origin/master\" >>expect.err.2 &&\n-\thead -2 expect.err >> expect.err.2 &&\n+\thead -3 expect.err >> expect.err.2 &&\n \t(\n \t\tcd downstream &&\n \t\tgit config fetch.recurseSubmodules on-demand &&\n@@ -399,7 +400,7 @@ test_expect_success \"'fetch.recurseSubmodules=on-demand' overrides global config\n \t\tcd downstream &&\n \t\tgit config --unset fetch.recurseSubmodules\n \t) &&\n-\ttest_i18ncmp expect.out.sub actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err.2 actual.err\n '\n \n@@ -416,7 +417,7 @@ test_expect_success \"'submodule.<sub>.fetchRecurseSubmodules=on-demand' override\n \thead2=$(git rev-parse --short HEAD) &&\n \techo \"From $pwd/.\" > expect.err.2 &&\n \techo \"   $head1..$head2  master     -> origin/master\" >>expect.err.2 &&\n-\thead -2 expect.err >> expect.err.2 &&\n+\thead -3 expect.err >> expect.err.2 &&\n \t(\n \t\tcd downstream &&\n \t\tgit config submodule.submodule.fetchRecurseSubmodules on-demand &&\n@@ -427,7 +428,7 @@ test_expect_success \"'submodule.<sub>.fetchRecurseSubmodules=on-demand' override\n \t\tcd downstream &&\n \t\tgit config --unset submodule.submodule.fetchRecurseSubmodules\n \t) &&\n-\ttest_i18ncmp expect.out.sub actual.out &&\n+\ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err.2 actual.err\n '\n \n-- \n2.6.4.443.ge094245.dirty\n"},{"id":"274534","messageId":"1450224252-16833-3-git-send-email-sbeller@google.com","threadId":"41011","inReplyTo":"1450224252-16833-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 2/7] xread: poll on non blocking fds","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-16T00:04:07Z","receivedAt":"2015-12-16T00:04:07Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The man page of read(2) says:\n\n  EAGAIN The file descriptor fd refers to a file other than a socket\n\t and has been marked nonblocking (O_NONBLOCK), and the read\n\t would block.\n\n  EAGAIN or EWOULDBLOCK\n\t The file descriptor fd refers to a socket and has been marked\n\t nonblocking (O_NONBLOCK), and the read would block.  POSIX.1-2001\n\t allows either error to be returned for this case, and does not\n\t require these constants to have the same value, so a portable\n\t application should check for both possibilities.\n\nIf we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.\nAs the intent of xread is to read as much as possible either until the\nfd is EOF or an actual error occurs, we can ease the feeder of the fd\nby not spinning the whole time, but rather wait for it politely by not\nbusy waiting.\n\nWe should not care if the call to poll failed, as we're in an infinite\nloop and can only get out with the correct read().\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nAcked-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n wrapper.c | 20 ++++++++++++++++++--\n 1 file changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 6fcaa4d..1770efa 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -236,8 +236,24 @@ ssize_t xread(int fd, void *buf, size_t len)\n \t    len = MAX_IO_SIZE;\n \twhile (1) {\n \t\tnr = read(fd, buf, len);\n-\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n-\t\t\tcontinue;\n+\t\tif (nr < 0) {\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tif (errno == EAGAIN || errno == EWOULDBLOCK) {\n+\t\t\t\tstruct pollfd pfd;\n+\t\t\t\tpfd.events = POLLIN;\n+\t\t\t\tpfd.fd = fd;\n+\t\t\t\t/*\n+\t\t\t\t * it is OK if this poll() failed; we\n+\t\t\t\t * want to leave this infinite loop\n+\t\t\t\t * only when read() returns with\n+\t\t\t\t * success, or an expected failure,\n+\t\t\t\t * which would be checked by the next\n+\t\t\t\t * call to read(2).\n+\t\t\t\t */\n+\t\t\t\tpoll(&pfd, 1, -1);\n+\t\t\t}\n+\t\t}\n \t\treturn nr;\n \t}\n }\n-- \n2.6.4.443.ge094245.dirty\n"},{"id":"274530","messageId":"1450224252-16833-4-git-send-email-sbeller@google.com","threadId":"41011","inReplyTo":"1450224252-16833-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 3/7] strbuf: add strbuf_read_once to read without blocking","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-16T00:04:08Z","receivedAt":"2015-12-16T00:04:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The new call will read from a file descriptor into a strbuf once. The\nunderlying call xread is just run once. xread only reattempts\nreading in case of EINTR, which makes it suitable to use for a\nnonblocking read.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n strbuf.c | 11 +++++++++++\n strbuf.h |  8 ++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex d76f0ae..38686ff 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -384,6 +384,17 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \treturn sb->len - oldlen;\n }\n \n+ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n+{\n+\tssize_t cnt;\n+\n+\tstrbuf_grow(sb, hint ? hint : 8192);\n+\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n+\tif (cnt > 0)\n+\t\tstrbuf_setlen(sb, sb->len + cnt);\n+\treturn cnt;\n+}\n+\n #define STRBUF_MAXLINK (2*PATH_MAX)\n \n int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\ndiff --git a/strbuf.h b/strbuf.h\nindex 7123fca..2bf90e7 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -367,6 +367,14 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n \n /**\n+ * Read the contents of a given file descriptor partially by using only one\n+ * attempt of xread. The third argument can be used to give a hint about the\n+ * file size, to avoid reallocs. Returns the number of new bytes appended to\n+ * the sb.\n+ */\n+extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);\n+\n+/**\n  * Read the contents of a file, specified by its path. The third argument\n  * can be used to give a hint about the file size, to avoid reallocs.\n  */\n-- \n2.6.4.443.ge094245.dirty\n"},{"id":"274531","messageId":"1450224252-16833-5-git-send-email-sbeller@google.com","threadId":"41011","inReplyTo":"1450224252-16833-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 4/7] sigchain: add command to pop all common signals","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-16T00:04:09Z","receivedAt":"2015-12-16T00:04:09Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The new method removes all common signal handlers that were installed\nby sigchain_push.\n\nCC: Jeff King <peff@peff.net>\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n sigchain.c | 9 +++++++++\n sigchain.h | 1 +\n 2 files changed, 10 insertions(+)\n\ndiff --git a/sigchain.c b/sigchain.c\nindex faa375d..2ac43bb 100644\n--- a/sigchain.c\n+++ b/sigchain.c\n@@ -50,3 +50,12 @@ void sigchain_push_common(sigchain_fun f)\n \tsigchain_push(SIGQUIT, f);\n \tsigchain_push(SIGPIPE, f);\n }\n+\n+void sigchain_pop_common(void)\n+{\n+\tsigchain_pop(SIGPIPE);\n+\tsigchain_pop(SIGQUIT);\n+\tsigchain_pop(SIGTERM);\n+\tsigchain_pop(SIGHUP);\n+\tsigchain_pop(SIGINT);\n+}\ndiff --git a/sigchain.h b/sigchain.h\nindex 618083b..138b20f 100644\n--- a/sigchain.h\n+++ b/sigchain.h\n@@ -7,5 +7,6 @@ int sigchain_push(int sig, sigchain_fun f);\n int sigchain_pop(int sig);\n \n void sigchain_push_common(sigchain_fun f);\n+void sigchain_pop_common(void);\n \n #endif /* SIGCHAIN_H */\n-- \n2.6.4.443.ge094245.dirty\n"},{"id":"274536","messageId":"1450224252-16833-6-git-send-email-sbeller@google.com","threadId":"41011","inReplyTo":"1450224252-16833-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 5/7] run-command: add an asynchronous parallel child processor","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-16T00:04:10Z","receivedAt":"2015-12-16T00:04:10Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This allows to run external commands in parallel with ordered output\non stderr.\n\nIf we run external commands in parallel we cannot pipe the output directly\nto the our stdout/err as it would mix up. So each process's output will\nflow through a pipe, which we buffer. One subprocess can be directly\npiped to out stdout/err for a low latency feedback to the user.\n\nExample:\nLet's assume we have 5 submodules A,B,C,D,E and each fetch takes a\ndifferent amount of time as the different submodules vary in size, then\nthe output of fetches in sequential order might look like this:\n\n time -->\n output: |---A---| |-B-| |-------C-------| |-D-| |-E-|\n\nWhen we schedule these submodules into maximal two parallel processes,\na schedule and sample output over time may look like this:\n\nprocess 1: |---A---| |-D-| |-E-|\n\nprocess 2: |-B-| |-------C-------|\n\noutput:    |---A---|B|---C-------|DE\n\nSo A will be perceived as it would run normally in the single child\nversion. As B has finished by the time A is done, we can dump its whole\nprogress buffer on stderr, such that it looks like it finished in no\ntime. Once that is done, C is determined to be the visible child and\nits progress will be reported in real time.\n\nSo this way of output is really good for human consumption, as it only\nchanges the timing, not the actual output.\n\nFor machine consumption the output needs to be prepared in the tasks,\nby either having a prefix per line or per block to indicate whose tasks\noutput is displayed, because the output order may not follow the\noriginal sequential ordering:\n\n |----A----| |--B--| |-C-|\n\nwill be scheduled to be all parallel:\n\nprocess 1: |----A----|\nprocess 2: |--B--|\nprocess 3: |-C-|\noutput:    |----A----|CB\n\nThis happens because C finished before B did, so it will be queued for\noutput before B.\n\nTo detect when a child has finished executing, we check interleaved\nwith other actions (such as checking the liveliness of children or\nstarting new processes) whether the stderr pipe still exists. Once a\nchild closed its stderr stream, we assume it is terminating very soon,\nand use `finish_command()` from the single external process execution\ninterface to collect the exit status.\n\nBy maintaining the strong assumption of stderr being open until the\nvery end of a child process, we can avoid other hassle such as an\nimplementation using `waitpid(-1)`, which is not implemented in Windows.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n run-command.c          | 335 +++++++++++++++++++++++++++++++++++++++++++++++++\n run-command.h          |  80 ++++++++++++\n t/t0061-run-command.sh |  53 ++++++++\n test-run-command.c     |  55 +++++++-\n 4 files changed, 522 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 13fa452..51fd72c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -3,6 +3,8 @@\n #include \"exec_cmd.h\"\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n+#include \"thread-utils.h\"\n+#include \"strbuf.h\"\n \n void child_process_init(struct child_process *child)\n {\n@@ -865,3 +867,336 @@ int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)\n \tclose(cmd->out);\n \treturn finish_command(cmd);\n }\n+\n+enum child_state {\n+\tGIT_CP_FREE,\n+\tGIT_CP_WORKING,\n+\tGIT_CP_WAIT_CLEANUP,\n+};\n+\n+struct parallel_processes {\n+\tvoid *data;\n+\n+\tint max_processes;\n+\tint nr_processes;\n+\n+\tget_next_task_fn get_next_task;\n+\tstart_failure_fn start_failure;\n+\ttask_finished_fn task_finished;\n+\n+\tstruct {\n+\t\tenum child_state state;\n+\t\tstruct child_process process;\n+\t\tstruct strbuf err;\n+\t\tvoid *data;\n+\t} *children;\n+\t/*\n+\t * The struct pollfd is logically part of *children,\n+\t * but the system call expects it as its own array.\n+\t */\n+\tstruct pollfd *pfd;\n+\n+\tunsigned shutdown : 1;\n+\n+\tint output_owner;\n+\tstruct strbuf buffered_output; /* of finished children */\n+};\n+\n+static int default_start_failure(struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb,\n+\t\t\t\t void *pp_task_cb)\n+{\n+\tint i;\n+\n+\tstrbuf_addstr(err, \"Starting a child failed:\");\n+\tfor (i = 0; cp->argv[i]; i++)\n+\t\tstrbuf_addf(err, \" %s\", cp->argv[i]);\n+\n+\treturn 0;\n+}\n+\n+static int default_task_finished(int result,\n+\t\t\t\t struct child_process *cp,\n+\t\t\t\t struct strbuf *err,\n+\t\t\t\t void *pp_cb,\n+\t\t\t\t void *pp_task_cb)\n+{\n+\tint i;\n+\n+\tif (!result)\n+\t\treturn 0;\n+\n+\tstrbuf_addf(err, \"A child failed with return code %d:\", result);\n+\tfor (i = 0; cp->argv[i]; i++)\n+\t\tstrbuf_addf(err, \" %s\", cp->argv[i]);\n+\n+\treturn 0;\n+}\n+\n+static void kill_children(struct parallel_processes *pp, int signo)\n+{\n+\tint i, n = pp->max_processes;\n+\n+\tfor (i = 0; i < n; i++)\n+\t\tif (pp->children[i].state == GIT_CP_WORKING)\n+\t\t\tkill(pp->children[i].process.pid, signo);\n+}\n+\n+static struct parallel_processes *pp_for_signal;\n+\n+static void handle_children_on_signal(int signo)\n+{\n+\tkill_children(pp_for_signal, signo);\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n+static void pp_init(struct parallel_processes *pp,\n+\t\t    int n,\n+\t\t    get_next_task_fn get_next_task,\n+\t\t    start_failure_fn start_failure,\n+\t\t    task_finished_fn task_finished,\n+\t\t    void *data)\n+{\n+\tint i;\n+\n+\tif (n < 1)\n+\t\tn = online_cpus();\n+\n+\tpp->max_processes = n;\n+\n+\ttrace_printf(\"run_processes_parallel: preparing to run up to %d tasks\", n);\n+\n+\tpp->data = data;\n+\tif (!get_next_task)\n+\t\tdie(\"BUG: you need to specify a get_next_task function\");\n+\tpp->get_next_task = get_next_task;\n+\n+\tpp->start_failure = start_failure ? start_failure : default_start_failure;\n+\tpp->task_finished = task_finished ? task_finished : default_task_finished;\n+\n+\tpp->nr_processes = 0;\n+\tpp->output_owner = 0;\n+\tpp->shutdown = 0;\n+\tpp->children = xcalloc(n, sizeof(*pp->children));\n+\tpp->pfd = xcalloc(n, sizeof(*pp->pfd));\n+\tstrbuf_init(&pp->buffered_output, 0);\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 | POLLHUP;\n+\t\tpp->pfd[i].fd = -1;\n+\t}\n+\n+\tpp_for_signal = pp;\n+\tsigchain_push_common(handle_children_on_signal);\n+}\n+\n+static void pp_cleanup(struct parallel_processes *pp)\n+{\n+\tint i;\n+\n+\ttrace_printf(\"run_processes_parallel: done\");\n+\tfor (i = 0; i < pp->max_processes; i++) {\n+\t\tstrbuf_release(&pp->children[i].err);\n+\t\tchild_process_clear(&pp->children[i].process);\n+\t}\n+\n+\tfree(pp->children);\n+\tfree(pp->pfd);\n+\n+\t/*\n+\t * When get_next_task added messages to the buffer in its last\n+\t * iteration, the buffered output is non empty.\n+\t */\n+\tfputs(pp->buffered_output.buf, stderr);\n+\tstrbuf_release(&pp->buffered_output);\n+\n+\tsigchain_pop_common();\n+}\n+\n+/* returns\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+ * <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, code;\n+\n+\tfor (i = 0; i < pp->max_processes; i++)\n+\t\tif (pp->children[i].state == GIT_CP_FREE)\n+\t\t\tbreak;\n+\tif (i == pp->max_processes)\n+\t\tdie(\"BUG: bookkeeping is hard\");\n+\n+\tcode = pp->get_next_task(&pp->children[i].process,\n+\t\t\t\t &pp->children[i].err,\n+\t\t\t\t pp->data,\n+\t\t\t\t &pp->children[i].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\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\tif (code)\n+\t\t\tpp->shutdown = 1;\n+\t\treturn code;\n+\t}\n+\n+\tpp->nr_processes++;\n+\tpp->children[i].state = GIT_CP_WORKING;\n+\tpp->pfd[i].fd = pp->children[i].process.err;\n+\treturn 0;\n+}\n+\n+static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n+{\n+\tint i;\n+\n+\twhile ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\tcontinue;\n+\t\tpp_cleanup(pp);\n+\t\tdie_errno(\"poll\");\n+\t}\n+\n+\t/* Buffer output from all pipes. */\n+\tfor (i = 0; i < pp->max_processes; i++) {\n+\t\tif (pp->children[i].state == GIT_CP_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 = GIT_CP_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].state == GIT_CP_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+\t}\n+}\n+\n+static int pp_collect_finished(struct parallel_processes *pp)\n+{\n+\tint i, code;\n+\tint n = pp->max_processes;\n+\tint result = 0;\n+\n+\twhile (pp->nr_processes > 0) {\n+\t\tfor (i = 0; i < pp->max_processes; i++)\n+\t\t\tif (pp->children[i].state == GIT_CP_WAIT_CLEANUP)\n+\t\t\t\tbreak;\n+\t\tif (i == pp->max_processes)\n+\t\t\tbreak;\n+\n+\t\tcode = finish_command(&pp->children[i].process);\n+\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].state = GIT_CP_FREE;\n+\t\tpp->pfd[i].fd = -1;\n+\t\tchild_process_init(&pp->children[i].process);\n+\n+\t\tif (i != pp->output_owner) {\n+\t\t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n+\t\t\tstrbuf_reset(&pp->children[i].err);\n+\t\t} else {\n+\t\t\tfputs(pp->children[i].err.buf, stderr);\n+\t\t\tstrbuf_reset(&pp->children[i].err);\n+\n+\t\t\t/* Output all other finished child processes */\n+\t\t\tfputs(pp->buffered_output.buf, stderr);\n+\t\t\tstrbuf_reset(&pp->buffered_output);\n+\n+\t\t\t/*\n+\t\t\t * Pick next process to output live.\n+\t\t\t * NEEDSWORK:\n+\t\t\t * For now we pick it randomly by doing a round\n+\t\t\t * robin. Later we may want to pick the one with\n+\t\t\t * the most output or the longest or shortest\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].state == GIT_CP_WORKING)\n+\t\t\t\t\tbreak;\n+\t\t\tpp->output_owner = (pp->output_owner + i) % n;\n+\t\t}\n+\t}\n+\treturn result;\n+}\n+\n+int run_processes_parallel(int n,\n+\t\t\t   get_next_task_fn get_next_task,\n+\t\t\t   start_failure_fn start_failure,\n+\t\t\t   task_finished_fn task_finished,\n+\t\t\t   void *pp_cb)\n+{\n+\tint i, code;\n+\tint output_timeout = 100;\n+\tint spawn_cap = 4;\n+\tstruct parallel_processes pp;\n+\n+\tpp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);\n+\twhile (1) {\n+\t\tfor (i = 0;\n+\t\t    i < spawn_cap && !pp.shutdown &&\n+\t\t    pp.nr_processes < pp.max_processes;\n+\t\t    i++) {\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, -code);\n+\t\t\t}\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!pp.nr_processes)\n+\t\t\tbreak;\n+\t\tpp_buffer_stderr(&pp, output_timeout);\n+\t\tpp_output(&pp);\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+\n+\tpp_cleanup(&pp);\n+\treturn 0;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex 12bb26c..d5a57f9 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -122,4 +122,84 @@ int start_async(struct async *async);\n int finish_async(struct async *async);\n int in_async(void);\n \n+/**\n+ * This callback should initialize the child process and preload the\n+ * error channel if desired. The preloading of is useful if you want to\n+ * have a message printed directly before the output of the child process.\n+ * pp_cb is the callback cookie as passed to run_processes_parallel.\n+ * You can store a child process specific callback cookie in pp_task_cb.\n+ *\n+ * Even after returning 0 to indicate that there are no more processes,\n+ * this function will be called again until there are no more running\n+ * child processes.\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 the negative signal number.\n+ */\n+typedef int (*get_next_task_fn)(struct child_process *cp,\n+\t\t\t\tstruct strbuf *err,\n+\t\t\t\tvoid *pp_cb,\n+\t\t\t\tvoid **pp_task_cb);\n+\n+/**\n+ * This callback is called whenever there are problems starting\n+ * a new process.\n+ *\n+ * You must not write to stdout or stderr in this function. Add your\n+ * message to the strbuf err instead, which will be printed without\n+ * messing up the output of the other parallel processes.\n+ *\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 return non zero.\n+ * To send a signal to other child processes for abortion, return\n+ * the negative signal number.\n+ */\n+typedef int (*start_failure_fn)(struct child_process *cp,\n+\t\t\t\tstruct strbuf *err,\n+\t\t\t\tvoid *pp_cb,\n+\t\t\t\tvoid *pp_task_cb);\n+\n+/**\n+ * This callback is called on every child process that finished processing.\n+ *\n+ * You must not write to stdout or stderr in this function. Add your\n+ * message to the strbuf err instead, which will be printed without\n+ * messing up the output of the other parallel processes.\n+ *\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 return non zero.\n+ * To send a signal to other child processes for abortion, return\n+ * the negative signal number.\n+ */\n+typedef int (*task_finished_fn)(int result,\n+\t\t\t\tstruct child_process *cp,\n+\t\t\t\tstruct strbuf *err,\n+\t\t\t\tvoid *pp_cb,\n+\t\t\t\tvoid *pp_task_cb);\n+\n+/**\n+ * Runs up to n processes at the same time. Whenever a process can be\n+ * started, the callback get_next_task_fn is called to obtain the data\n+ * required to start another child process.\n+ *\n+ * The children started via this function run in parallel. Their output\n+ * (both stdout and stderr) is routed to stderr in a manner that output\n+ * from different tasks does not interleave.\n+ *\n+ * If start_failure_fn or task_finished_fn are NULL, default handlers\n+ * will be used. The default handlers will print an error message on\n+ * error without issuing an emergency stop.\n+ */\n+int run_processes_parallel(int n,\n+\t\t\t   get_next_task_fn,\n+\t\t\t   start_failure_fn,\n+\t\t\t   task_finished_fn,\n+\t\t\t   void *pp_cb);\n+\n #endif\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex 9acf628..12228b4 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -47,4 +47,57 @@ test_expect_success POSIXPERM,SANITY 'unreadable directory in PATH' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <<-EOF\n+preloaded output of a child\n+Hello\n+World\n+preloaded output of a child\n+Hello\n+World\n+preloaded output of a child\n+Hello\n+World\n+preloaded output of a child\n+Hello\n+World\n+EOF\n+\n+test_expect_success 'run_command runs in parallel with more jobs available than tasks' '\n+\ttest-run-command run-command-parallel 5 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'run_command runs in parallel with as many jobs as tasks' '\n+\ttest-run-command run-command-parallel 4 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'run_command runs in parallel with more tasks than jobs available' '\n+\ttest-run-command run-command-parallel 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<-EOF\n+preloaded output of a child\n+asking for a quick stop\n+preloaded output of a child\n+asking for a quick stop\n+preloaded output of a child\n+asking for a quick stop\n+EOF\n+\n+test_expect_success 'run_command is asked to abort gracefully' '\n+\ttest-run-command run-command-abort 3 false 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<-EOF\n+no further jobs available\n+EOF\n+\n+test_expect_success 'run_command outputs ' '\n+\ttest-run-command run-command-no-jobs 3 sh -c \"printf \\\"%s\\n%s\\n\\\" Hello World\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/test-run-command.c b/test-run-command.c\nindex 89c7de2..f964507 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -10,16 +10,54 @@\n \n #include \"git-compat-util.h\"\n #include \"run-command.h\"\n+#include \"argv-array.h\"\n+#include \"strbuf.h\"\n #include <string.h>\n #include <errno.h>\n \n+static int number_callbacks;\n+static int parallel_next(struct child_process *cp,\n+\t\t\t struct strbuf *err,\n+\t\t\t void *cb,\n+\t\t\t void** task_cb)\n+{\n+\tstruct child_process *d = cb;\n+\tif (number_callbacks >= 4)\n+\t\treturn 0;\n+\n+\targv_array_pushv(&cp->args, d->argv);\n+\tstrbuf_addf(err, \"preloaded output of a child\\n\");\n+\tnumber_callbacks++;\n+\treturn 1;\n+}\n+\n+static int no_job(struct child_process *cp,\n+\t\t  struct strbuf *err,\n+\t\t  void *cb,\n+\t\t  void** task_cb)\n+{\n+\tstrbuf_addf(err, \"no further jobs available\\n\");\n+\treturn 0;\n+}\n+\n+static int task_finished(int result,\n+\t\t\t struct child_process *cp,\n+\t\t\t struct strbuf *err,\n+\t\t\t void *pp_cb,\n+\t\t\t void *pp_task_cb)\n+{\n+\tstrbuf_addf(err, \"asking for a quick stop\\n\");\n+\treturn 1;\n+}\n+\n int main(int argc, char **argv)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n+\tint jobs;\n \n \tif (argc < 3)\n \t\treturn 1;\n-\tproc.argv = (const char **)argv+2;\n+\tproc.argv = (const char **)argv + 2;\n \n \tif (!strcmp(argv[1], \"start-command-ENOENT\")) {\n \t\tif (start_command(&proc) < 0 && errno == ENOENT)\n@@ -30,6 +68,21 @@ int main(int argc, char **argv)\n \tif (!strcmp(argv[1], \"run-command\"))\n \t\texit(run_command(&proc));\n \n+\tjobs = atoi(argv[2]);\n+\tproc.argv = (const char **)argv + 3;\n+\n+\tif (!strcmp(argv[1], \"run-command-parallel\"))\n+\t\texit(run_processes_parallel(jobs, parallel_next,\n+\t\t\t\t\t    NULL, NULL, &proc));\n+\n+\tif (!strcmp(argv[1], \"run-command-abort\"))\n+\t\texit(run_processes_parallel(jobs, parallel_next,\n+\t\t\t\t\t    NULL, task_finished, &proc));\n+\n+\tif (!strcmp(argv[1], \"run-command-no-jobs\"))\n+\t\texit(run_processes_parallel(jobs, no_job,\n+\t\t\t\t\t    NULL, task_finished, &proc));\n+\n \tfprintf(stderr, \"check usage\\n\");\n \treturn 1;\n }\n-- \n2.6.4.443.ge094245.dirty\n"},{"id":"274533","messageId":"1450224252-16833-7-git-send-email-sbeller@google.com","threadId":"41011","inReplyTo":"1450224252-16833-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 6/7] fetch_populated_submodules: use new parallel job processing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-16T00:04:11Z","receivedAt":"2015-12-16T00:04:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we enable parallel processing of submodules, this\nonly adds the possibility for it. So this change should not change\nany user facing behavior.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n submodule.c | 142 +++++++++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 98 insertions(+), 44 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 8386477..6a2d786 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -12,6 +12,7 @@\n #include \"sha1-array.h\"\n #include \"argv-array.h\"\n #include \"blob.h\"\n+#include \"thread-utils.h\"\n \n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static struct string_list changed_submodule_paths;\n@@ -610,37 +611,28 @@ static void calculate_changed_submodule_paths(void)\n \tinitialized_fetch_ref_tips = 0;\n }\n \n-int fetch_populated_submodules(const struct argv_array *options,\n-\t\t\t       const char *prefix, int command_line_option,\n-\t\t\t       int quiet)\n+struct submodule_parallel_fetch {\n+\tint count;\n+\tstruct argv_array args;\n+\tconst char *work_tree;\n+\tconst char *prefix;\n+\tint command_line_option;\n+\tint quiet;\n+\tint result;\n+};\n+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0}\n+\n+static int get_next_submodule(struct child_process *cp,\n+\t\t\t      struct strbuf *err, void *data, void **task_cb)\n {\n-\tint i, result = 0;\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tconst char *work_tree = get_git_work_tree();\n-\tif (!work_tree)\n-\t\tgoto out;\n-\n-\tif (read_cache() < 0)\n-\t\tdie(\"index file corrupt\");\n-\n-\targv_array_push(&argv, \"fetch\");\n-\tfor (i = 0; i < options->argc; i++)\n-\t\targv_array_push(&argv, options->argv[i]);\n-\targv_array_push(&argv, \"--recurse-submodules-default\");\n-\t/* default value, \"--submodule-prefix\" and its value are added later */\n-\n-\tcp.env = local_repo_env;\n-\tcp.git_cmd = 1;\n-\tcp.no_stdin = 1;\n-\n-\tcalculate_changed_submodule_paths();\n+\tint ret = 0;\n+\tstruct submodule_parallel_fetch *spf = data;\n \n-\tfor (i = 0; i < active_nr; i++) {\n+\tfor ( ; spf->count < active_nr; spf->count++) {\n \t\tstruct strbuf submodule_path = STRBUF_INIT;\n \t\tstruct strbuf submodule_git_dir = STRBUF_INIT;\n \t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n-\t\tconst struct cache_entry *ce = active_cache[i];\n+\t\tconst struct cache_entry *ce = active_cache[spf->count];\n \t\tconst char *git_dir, *default_argv;\n \t\tconst struct submodule *submodule;\n \n@@ -652,7 +644,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\tsubmodule = submodule_from_name(null_sha1, ce->name);\n \n \t\tdefault_argv = \"yes\";\n-\t\tif (command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n+\t\tif (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n \t\t\tif (submodule &&\n \t\t\t    submodule->fetch_recurse !=\n \t\t\t\t\t\tRECURSE_SUBMODULES_NONE) {\n@@ -675,40 +667,102 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t\t\tdefault_argv = \"on-demand\";\n \t\t\t\t}\n \t\t\t}\n-\t\t} else if (command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n+\t\t} else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {\n \t\t\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))\n \t\t\t\tcontinue;\n \t\t\tdefault_argv = \"on-demand\";\n \t\t}\n \n-\t\tstrbuf_addf(&submodule_path, \"%s/%s\", work_tree, ce->name);\n+\t\tstrbuf_addf(&submodule_path, \"%s/%s\", spf->work_tree, ce->name);\n \t\tstrbuf_addf(&submodule_git_dir, \"%s/.git\", submodule_path.buf);\n-\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", prefix, ce->name);\n+\t\tstrbuf_addf(&submodule_prefix, \"%s%s/\", spf->prefix, ce->name);\n \t\tgit_dir = read_gitfile(submodule_git_dir.buf);\n \t\tif (!git_dir)\n \t\t\tgit_dir = submodule_git_dir.buf;\n \t\tif (is_directory(git_dir)) {\n-\t\t\tif (!quiet)\n-\t\t\t\tfprintf(stderr, \"Fetching submodule %s%s\\n\", prefix, ce->name);\n-\t\t\tcp.dir = submodule_path.buf;\n-\t\t\targv_array_push(&argv, default_argv);\n-\t\t\targv_array_push(&argv, \"--submodule-prefix\");\n-\t\t\targv_array_push(&argv, submodule_prefix.buf);\n-\t\t\tcp.argv = argv.argv;\n-\t\t\tif (run_command(&cp))\n-\t\t\t\tresult = 1;\n-\t\t\targv_array_pop(&argv);\n-\t\t\targv_array_pop(&argv);\n-\t\t\targv_array_pop(&argv);\n+\t\t\tchild_process_init(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\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);\n+\t\t\targv_array_init(&cp->args);\n+\t\t\targv_array_pushv(&cp->args, spf->args.argv);\n+\t\t\targv_array_push(&cp->args, default_argv);\n+\t\t\targv_array_push(&cp->args, \"--submodule-prefix\");\n+\t\t\targv_array_push(&cp->args, submodule_prefix.buf);\n+\t\t\tret = 1;\n \t\t}\n \t\tstrbuf_release(&submodule_path);\n \t\tstrbuf_release(&submodule_git_dir);\n \t\tstrbuf_release(&submodule_prefix);\n+\t\tif (ret) {\n+\t\t\tspf->count++;\n+\t\t\treturn 1;\n+\t\t}\n \t}\n-\targv_array_clear(&argv);\n+\treturn 0;\n+}\n+\n+static int fetch_start_failure(struct child_process *cp,\n+\t\t\t       struct strbuf *err,\n+\t\t\t       void *cb, void *task_cb)\n+{\n+\tstruct submodule_parallel_fetch *spf = cb;\n+\n+\tspf->result = 1;\n+\n+\treturn 0;\n+}\n+\n+static int fetch_finish(int retvalue, struct child_process *cp,\n+\t\t\tstruct strbuf *err, void *cb, void *task_cb)\n+{\n+\tstruct submodule_parallel_fetch *spf = cb;\n+\n+\tif (retvalue)\n+\t\tspf->result = 1;\n+\n+\treturn 0;\n+}\n+\n+int fetch_populated_submodules(const struct argv_array *options,\n+\t\t\t       const char *prefix, int command_line_option,\n+\t\t\t       int quiet)\n+{\n+\tint i;\n+\tint max_parallel_jobs = 1;\n+\tstruct submodule_parallel_fetch spf = SPF_INIT;\n+\n+\tspf.work_tree = get_git_work_tree();\n+\tspf.command_line_option = command_line_option;\n+\tspf.quiet = quiet;\n+\tspf.prefix = prefix;\n+\n+\tif (!spf.work_tree)\n+\t\tgoto out;\n+\n+\tif (read_cache() < 0)\n+\t\tdie(\"index file corrupt\");\n+\n+\targv_array_push(&spf.args, \"fetch\");\n+\tfor (i = 0; i < options->argc; i++)\n+\t\targv_array_push(&spf.args, options->argv[i]);\n+\targv_array_push(&spf.args, \"--recurse-submodules-default\");\n+\t/* default value, \"--submodule-prefix\" and its value are added later */\n+\n+\tcalculate_changed_submodule_paths();\n+\trun_processes_parallel(max_parallel_jobs,\n+\t\t\t       get_next_submodule,\n+\t\t\t       fetch_start_failure,\n+\t\t\t       fetch_finish,\n+\t\t\t       &spf);\n+\n+\targv_array_clear(&spf.args);\n out:\n \tstring_list_clear(&changed_submodule_paths, 1);\n-\treturn result;\n+\treturn spf.result;\n }\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n-- \n2.6.4.443.ge094245.dirty\n"},{"id":"274535","messageId":"1450224252-16833-8-git-send-email-sbeller@google.com","threadId":"41011","inReplyTo":"1450224252-16833-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 7/7] submodules: allow parallel fetching, add tests and documentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-16T00:04:12Z","receivedAt":"2015-12-16T00:04:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This enables the work of the previous patches.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/fetch-options.txt |  7 +++++++\n builtin/fetch.c                 |  6 +++++-\n builtin/pull.c                  |  6 ++++++\n submodule.c                     |  3 +--\n submodule.h                     |  2 +-\n t/t5526-fetch-submodules.sh     | 20 ++++++++++++++++++++\n 6 files changed, 40 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 45583d8..6b109f6 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -100,6 +100,13 @@ ifndef::git-pull[]\n \treference to a commit that isn't already in the local submodule\n \tclone.\n \n+-j::\n+--jobs=<n>::\n+\tNumber of parallel children to be used for fetching submodules.\n+\tEach will fetch from different submodules, such that fetching many\n+\tsubmodules will be faster. By default submodules will be fetched\n+\tone at a time.\n+\n --no-recurse-submodules::\n \tDisable recursive fetching of submodules (this has the same effect as\n \tusing the '--recurse-submodules=no' option).\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c85f347..586840d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -37,6 +37,7 @@ static int prune = -1; /* unspecified */\n static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;\n static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow;\n+static int max_children = 1;\n static const char *depth;\n static const char *upload_pack;\n static struct strbuf default_rla = STRBUF_INIT;\n@@ -99,6 +100,8 @@ static struct option builtin_fetch_options[] = {\n \t\t    N_(\"fetch all tags and associated objects\"), TAGS_SET),\n \tOPT_SET_INT('n', NULL, &tags,\n \t\t    N_(\"do not fetch all tags (--no-tags)\"), TAGS_UNSET),\n+\tOPT_INTEGER('j', \"jobs\", &max_children,\n+\t\t    N_(\"number of submodules fetched in parallel\")),\n \tOPT_BOOL('p', \"prune\", &prune,\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", NULL, N_(\"on-demand\"),\n@@ -1213,7 +1216,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_populated_submodules(&options,\n \t\t\t\t\t\t    submodule_prefix,\n \t\t\t\t\t\t    recurse_submodules,\n-\t\t\t\t\t\t    verbosity < 0);\n+\t\t\t\t\t\t    verbosity < 0,\n+\t\t\t\t\t\t    max_children);\n \t\targv_array_clear(&options);\n \t}\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 5145fc6..9e3c738 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -95,6 +95,7 @@ static int opt_force;\n static char *opt_tags;\n static char *opt_prune;\n static char *opt_recurse_submodules;\n+static char *max_children;\n static int opt_dry_run;\n static char *opt_keep;\n static char *opt_depth;\n@@ -178,6 +179,9 @@ static struct option pull_options[] = {\n \t\tN_(\"on-demand\"),\n \t\tN_(\"control recursive fetching of submodules\"),\n \t\tPARSE_OPT_OPTARG),\n+\tOPT_PASSTHRU('j', \"jobs\", &max_children, N_(\"n\"),\n+\t\tN_(\"number of submodules pulled in parallel\"),\n+\t\tPARSE_OPT_OPTARG),\n \tOPT_BOOL(0, \"dry-run\", &opt_dry_run,\n \t\tN_(\"dry run\")),\n \tOPT_PASSTHRU('k', \"keep\", &opt_keep, NULL,\n@@ -525,6 +529,8 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\targv_array_push(&args, opt_prune);\n \tif (opt_recurse_submodules)\n \t\targv_array_push(&args, opt_recurse_submodules);\n+\tif (max_children)\n+\t\targv_array_push(&args, max_children);\n \tif (opt_dry_run)\n \t\targv_array_push(&args, \"--dry-run\");\n \tif (opt_keep)\ndiff --git a/submodule.c b/submodule.c\nindex 6a2d786..0b48734 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -729,10 +729,9 @@ static int fetch_finish(int retvalue, struct child_process *cp,\n \n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t       const char *prefix, int command_line_option,\n-\t\t\t       int quiet)\n+\t\t\t       int quiet, int max_parallel_jobs)\n {\n \tint i;\n-\tint max_parallel_jobs = 1;\n \tstruct submodule_parallel_fetch spf = SPF_INIT;\n \n \tspf.work_tree = get_git_work_tree();\ndiff --git a/submodule.h b/submodule.h\nindex 5507c3d..cbc0003 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -31,7 +31,7 @@ void set_config_fetch_recurse_submodules(int value);\n void check_for_new_submodule_commits(unsigned char new_sha1[20]);\n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t       const char *prefix, int command_line_option,\n-\t\t\t       int quiet);\n+\t\t\t       int quiet, int max_parallel_jobs);\n unsigned is_submodule_modified(const char *path, int ignore_untracked);\n int submodule_uses_gitfile(const char *path);\n int ok_to_remove_submodule(const char *path);\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 17759b14..1241146 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -71,6 +71,17 @@ test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n \ttest_i18ncmp expect.err actual.err\n '\n \n+test_expect_success \"fetch --recurse-submodules -j2 has the same output behaviour\" '\n+\tadd_upstream_commit &&\n+\t(\n+\t\tcd downstream &&\n+\t\tGIT_TRACE=$(pwd)/../trace.out git fetch --recurse-submodules -j2 2>../actual.err\n+\t) &&\n+\ttest_must_be_empty actual.out &&\n+\ttest_i18ncmp expect.err actual.err &&\n+\tgrep \"2 tasks\" trace.out\n+'\n+\n test_expect_success \"fetch alone only fetches superproject\" '\n \tadd_upstream_commit &&\n \t(\n@@ -140,6 +151,15 @@ test_expect_success \"--quiet propagates to submodules\" '\n \t! test -s actual.err\n '\n \n+test_expect_success \"--quiet propagates to parallel submodules\" '\n+\t(\n+\t\tcd downstream &&\n+\t\tgit fetch --recurse-submodules -j 2 --quiet  >../actual.out 2>../actual.err\n+\t) &&\n+\t! test -s actual.out &&\n+\t! test -s actual.err\n+'\n+\n test_expect_success \"--dry-run propagates to submodules\" '\n \tadd_upstream_commit &&\n \t(\n-- \n2.6.4.443.ge094245.dirty\n"},{"id":"274537","messageId":"1450225194-18386-1-git-send-email-sbeller@google.com","threadId":"41011","inReplyTo":"1450224252-16833-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 0/7] Rerolling sb/submodule-parallel-fetch for the time after 2.7","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-16T00:19:54Z","receivedAt":"2015-12-16T00:19:54Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"with the interdiff below:\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 87456a3..8e39867 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -723,7 +723,6 @@ extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_\n extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern int xopen(const char *path, int flags, ...);\n extern ssize_t xread(int fd, void *buf, size_t len);\n-extern ssize_t xread_nonblock(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n extern int xdup(int fd);\ndiff --git a/strbuf.c b/strbuf.c\nindex b552a13..38686ff 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -389,7 +389,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n \tssize_t cnt;\n\n \tstrbuf_grow(sb, hint ? hint : 8192);\n-\tcnt = xread_nonblock(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n+\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n \tif (cnt > 0)\n \t\tstrbuf_setlen(sb, sb->len + cnt);\n \treturn cnt;\ndiff --git a/strbuf.h b/strbuf.h\nindex c3e5980..2bf90e7 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -367,10 +367,10 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n\n /**\n- * Returns the number of new bytes appended to the sb.\n- * Negative return value signals there was an error returned from\n- * underlying read(2), in which case the caller should check errno.\n- * e.g. errno == EAGAIN when the read may have blocked.\n+ * Read the contents of a given file descriptor partially by using only one\n+ * attempt of xread. The third argument can be used to give a hint about the\n+ * file size, to avoid reallocs. Returns the number of new bytes appended to\n+ * the sb.\n  */\n extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);\ndiff --git a/wrapper.c b/wrapper.c\nindex f71237c..1770efa 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -243,7 +243,14 @@ ssize_t xread(int fd, void *buf, size_t len)\n \t\t\t\tstruct pollfd pfd;\n \t\t\t\tpfd.events = POLLIN;\n \t\t\t\tpfd.fd = fd;\n-\t\t\t\t/* We deliberately ignore the return value */\n+\t\t\t\t/*\n+\t\t\t\t * it is OK if this poll() failed; we\n+\t\t\t\t * want to leave this infinite loop\n+\t\t\t\t * only when read() returns with\n+\t\t\t\t * success, or an expected failure,\n+\t\t\t\t * which would be checked by the next\n+\t\t\t\t * call to read(2).\n+\t\t\t\t */\n \t\t\t\tpoll(&pfd, 1, -1);\n \t\t\t}\n \t\t}\n@@ -252,28 +259,6 @@ ssize_t xread(int fd, void *buf, size_t len)\n }\n\n /*\n- * xread_nonblock() is the same a read(), but it automatically restarts read()\n- * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that\n- * \"len\" bytes is read. EWOULDBLOCK is turned into EAGAIN.\n- */\n-ssize_t xread_nonblock(int fd, void *buf, size_t len)\n-{\n-\tssize_t nr;\n-\tif (len > MAX_IO_SIZE)\n-\t\tlen = MAX_IO_SIZE;\n-\twhile (1) {\n-\t\tnr = read(fd, buf, len);\n-\t\tif (nr < 0) {\n-\t\t\tif (errno == EINTR)\n-\t\t\t\tcontinue;\n-\t\t\tif (errno == EWOULDBLOCK)\n-\t\t\t\terrno = EAGAIN;\n-\t\t}\n-\t\treturn nr;\n-\t}\n-}\n-\n-/*\n  * xwrite() is the same a write(), but it automatically restarts write()\n  * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT\n  * GUARANTEE that \"len\" bytes is written even if the operation is successful.\n"},{"id":"274599","messageId":"xmqqy4cucfhg.fsf@gitster.mtv.corp.google.com","threadId":"41011","inReplyTo":"1450224252-16833-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 0/7] Rerolling sb/submodule-parallel-fetch for the time after 2.7","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-16T20:20:43Z","receivedAt":"2015-12-16T20:20:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I am sending out a new version for replacing sb/submodule-parallel-fetch for\n> the time after the 2.7 release.\n>\n> * Dropped the patch, which introduces xread_nonblock\n> * strbuf_read_once uses xread now. This is safe as we poll before using\n>   strbuf_read_once, so we know we won't stall.\n\n\"That is only true for the current callers\" was my first reaction,\nbut it is safe and sensible even for future callers.  If they have\nsomething better to do than getting stuck before reading some, they\nought to be checking for POLLIN before calling this, and if not,\nthey do not have to be forced to spin for themselves until this\nreturns some new payload.\n\nI like that.\n\n> * have the commit message reworded for \"run-command: add an\n>   asynchronous parallel child processor\" with Johannes' suggestion.\n\nThanks.\n"},{"id":"274686","messageId":"56731715.9000509@web.de","threadId":"41011","inReplyTo":"1450224252-16833-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 2/7] xread: poll on non blocking fds","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-12-17T20:12:05Z","receivedAt":"2015-12-17T20:12:05Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 16.12.15 01:04, Stefan Beller wrote:\n> The man page of read(2) says:\n> \n>   EAGAIN The file descriptor fd refers to a file other than a socket\n> \t and has been marked nonblocking (O_NONBLOCK), and the read\n> \t would block.\n> \n>   EAGAIN or EWOULDBLOCK\n> \t The file descriptor fd refers to a socket and has been marked\n> \t nonblocking (O_NONBLOCK), and the read would block.  POSIX.1-2001\n> \t allows either error to be returned for this case, and does not\n> \t require these constants to have the same value, so a portable\n> \t application should check for both possibilities.\n> \n> If we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.\n> As the intent of xread is to read as much as possible either until the\n> fd is EOF or an actual error occurs, we can ease the feeder of the fd\n> by not spinning the whole time, but rather wait for it politely by not\n> busy waiting.\n> \n> We should not care if the call to poll failed, as we're in an infinite\n> loop and can only get out with the correct read().\nI'm not sure if this is valid under all circumstances:\nThis is what \"man poll\" says under Linux: \n[]\n ENOMEM There was no space to allocate file descriptor tables.\n[]\nAnd this is from Mac OS, (\"BSD System Calls Manual\")\nERRORS\n     Poll() will fail if:\n\n     [EAGAIN]           Allocation of internal data structures fails.  A sub-\n                        sequent request may succeed.\nAnd this is opengroup:\nhttp://pubs.opengroup.org/onlinepubs/9699919799//functions/poll.html:\n[EAGAIN]\n    The allocation of internal data structures failed but a subsequent request may succeed.\n\nread() may return EAGAIN, but poll() may fail to allocate memory, and fail.\nIs it always guaranteed that the loop is terminated?\n\n\n\n> \n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> Acked-by: Johannes Sixt <j6t@kdbg.org>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  wrapper.c | 20 ++++++++++++++++++--\n>  1 file changed, 18 insertions(+), 2 deletions(-)\n> \n> diff --git a/wrapper.c b/wrapper.c\n> index 6fcaa4d..1770efa 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -236,8 +236,24 @@ ssize_t xread(int fd, void *buf, size_t len)\n>  \t    len = MAX_IO_SIZE;\n>  \twhile (1) {\n>  \t\tnr = read(fd, buf, len);\n> -\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n> -\t\t\tcontinue;\n> +\t\tif (nr < 0) {\n> +\t\t\tif (errno == EINTR)\n> +\t\t\t\tcontinue;\n> +\t\t\tif (errno == EAGAIN || errno == EWOULDBLOCK) {\n> +\t\t\t\tstruct pollfd pfd;\n> +\t\t\t\tpfd.events = POLLIN;\n> +\t\t\t\tpfd.fd = fd;\n> +\t\t\t\t/*\n> +\t\t\t\t * it is OK if this poll() failed; we\n> +\t\t\t\t * want to leave this infinite loop\n> +\t\t\t\t * only when read() returns with\n> +\t\t\t\t * success, or an expected failure,\n> +\t\t\t\t * which would be checked by the next\n> +\t\t\t\t * call to read(2).\n> +\t\t\t\t */\n> +\t\t\t\tpoll(&pfd, 1, -1);\n> +\t\t\t}\n> +\t\t}\n>  \t\treturn nr;\n>  \t}\n>  }\n> \n"},{"id":"274687","messageId":"CAGZ79kZD_vrwHyd2WZzx-9FF3D6CVVi6X=Cx1=HAgr1gqNKyaA@mail.gmail.com","threadId":"41011","inReplyTo":"56731715.9000509@web.de","subject":"Re: [PATCHv2 2/7] xread: poll on non blocking fds","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-17T20:22:21Z","receivedAt":"2015-12-17T20:22:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 17, 2015 at 12:12 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n> On 16.12.15 01:04, Stefan Beller wrote:\n>> The man page of read(2) says:\n>>\n>>   EAGAIN The file descriptor fd refers to a file other than a socket\n>>        and has been marked nonblocking (O_NONBLOCK), and the read\n>>        would block.\n>>\n>>   EAGAIN or EWOULDBLOCK\n>>        The file descriptor fd refers to a socket and has been marked\n>>        nonblocking (O_NONBLOCK), and the read would block.  POSIX.1-2001\n>>        allows either error to be returned for this case, and does not\n>>        require these constants to have the same value, so a portable\n>>        application should check for both possibilities.\n>>\n>> If we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.\n>> As the intent of xread is to read as much as possible either until the\n>> fd is EOF or an actual error occurs, we can ease the feeder of the fd\n>> by not spinning the whole time, but rather wait for it politely by not\n>> busy waiting.\n>>\n>> We should not care if the call to poll failed, as we're in an infinite\n>> loop and can only get out with the correct read().\n> I'm not sure if this is valid under all circumstances:\n> This is what \"man poll\" says under Linux:\n> []\n>  ENOMEM There was no space to allocate file descriptor tables.\n> []\n> And this is from Mac OS, (\"BSD System Calls Manual\")\n> ERRORS\n>      Poll() will fail if:\n>\n>      [EAGAIN]           Allocation of internal data structures fails.  A sub-\n>                         sequent request may succeed.\n> And this is opengroup:\n> http://pubs.opengroup.org/onlinepubs/9699919799//functions/poll.html:\n> [EAGAIN]\n>     The allocation of internal data structures failed but a subsequent request may succeed.\n>\n> read() may return EAGAIN, but poll() may fail to allocate memory, and fail.\n> Is it always guaranteed that the loop is terminated?\n\nIn case poll fails (assume a no op for it), the logic should not have\nchanged by this patch?\n\nLooking closely:\n\n>>       while (1) {\n>>               nr = read(fd, buf, len);\n>> -             if ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n>> -                     continue;\n>> +             if (nr < 0) {\n>> +                     if (errno == EINTR)\n>> +                             continue;\n>> +                     if (errno == EAGAIN || errno == EWOULDBLOCK) {\n>> +                             struct pollfd pfd;\n>> +                             pfd.events = POLLIN;\n>> +                             pfd.fd = fd;\n>> +                             /*\n>> +                              * it is OK if this poll() failed; we\n>> +                              * want to leave this infinite loop\n>> +                              * only when read() returns with\n>> +                              * success, or an expected failure,\n>> +                              * which would be checked by the next\n>> +                              * call to read(2).\n>> +                              */\n>> +                             poll(&pfd, 1, -1);\n\nOr do you mean to insert another continue in here?\n\n>> +                     }\n>> +             }\n>>               return nr;\n>>       }\n>>  }\n>>\n>\n"},{"id":"274689","messageId":"56731E19.7050504@web.de","threadId":"41011","inReplyTo":"CAGZ79kZD_vrwHyd2WZzx-9FF3D6CVVi6X=Cx1=HAgr1gqNKyaA@mail.gmail.com","subject":"Re: [PATCHv2 2/7] xread: poll on non blocking fds","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-12-17T20:42:01Z","receivedAt":"2015-12-17T20:42:01Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 17.12.15 21:22, Stefan Beller wrote:\n> On Thu, Dec 17, 2015 at 12:12 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n>> On 16.12.15 01:04, Stefan Beller wrote:\n>>> The man page of read(2) says:\n>>>\n>>>   EAGAIN The file descriptor fd refers to a file other than a socket\n>>>        and has been marked nonblocking (O_NONBLOCK), and the read\n>>>        would block.\n>>>\n>>>   EAGAIN or EWOULDBLOCK\n>>>        The file descriptor fd refers to a socket and has been marked\n>>>        nonblocking (O_NONBLOCK), and the read would block.  POSIX.1-2001\n>>>        allows either error to be returned for this case, and does not\n>>>        require these constants to have the same value, so a portable\n>>>        application should check for both possibilities.\n>>>\n>>> If we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.\n>>> As the intent of xread is to read as much as possible either until the\n>>> fd is EOF or an actual error occurs, we can ease the feeder of the fd\n>>> by not spinning the whole time, but rather wait for it politely by not\n>>> busy waiting.\n>>>\n>>> We should not care if the call to poll failed, as we're in an infinite\n>>> loop and can only get out with the correct read().\n>> I'm not sure if this is valid under all circumstances:\n>> This is what \"man poll\" says under Linux:\n>> []\n>>  ENOMEM There was no space to allocate file descriptor tables.\n>> []\n>> And this is from Mac OS, (\"BSD System Calls Manual\")\n>> ERRORS\n>>      Poll() will fail if:\n>>\n>>      [EAGAIN]           Allocation of internal data structures fails.  A sub-\n>>                         sequent request may succeed.\n>> And this is opengroup:\n>> http://pubs.opengroup.org/onlinepubs/9699919799//functions/poll.html:\n>> [EAGAIN]\n>>     The allocation of internal data structures failed but a subsequent request may succeed.\n>>\n>> read() may return EAGAIN, but poll() may fail to allocate memory, and fail.\n>> Is it always guaranteed that the loop is terminated?\n> \n> In case poll fails (assume a no op for it), the logic should not have\n> changed by this patch?\n> \n> Looking closely:\n> \n>>>       while (1) {\n>>>               nr = read(fd, buf, len);\n>>> -             if ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n>>> -                     continue;\n>>> +             if (nr < 0) {\n>>> +                     if (errno == EINTR)\n>>> +                             continue;\n>>> +                     if (errno == EAGAIN || errno == EWOULDBLOCK) {\n>>> +                             struct pollfd pfd;\n>>> +                             pfd.events = POLLIN;\n>>> +                             pfd.fd = fd;\n>>> +                             /*\n>>> +                              * it is OK if this poll() failed; we\n>>> +                              * want to leave this infinite loop\n>>> +                              * only when read() returns with\n>>> +                              * success, or an expected failure,\n>>> +                              * which would be checked by the next\n>>> +                              * call to read(2).\n>>> +                              */\n>>> +                             poll(&pfd, 1, -1);\n> \n> Or do you mean to insert another continue in here?\nI was thinking that we run into similar loop as before:\nread() returns -1; errno = EAGAIN  /* No data to read */\npoll() returns -1; errno = EAGAIN /* poll failed. If the fd was OK, the failure may be temporaly,\n                                    as much as poll() can see.\n                                    But most probably we run out ouf memory */\n\nSo the code would look like this:\n\n   if (!poll(&pfd, 1, -1))\n      return -1;\n\n\n> \n\n>>> +                     }\n>>> +             }\n>>>               return nr;\n>>>       }\n>>>  }\n>>>\n>>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"274691","messageId":"CAGZ79kYHjH7QgChn6_GU9iLrtyouFQFuMAZKADZofgHjBS73-A@mail.gmail.com","threadId":"41011","inReplyTo":"56731E19.7050504@web.de","subject":"Re: [PATCHv2 2/7] xread: poll on non blocking fds","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-12-17T20:51:08Z","receivedAt":"2015-12-17T20:51:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 17, 2015 at 12:42 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n> On 17.12.15 21:22, Stefan Beller wrote:\n>> On Thu, Dec 17, 2015 at 12:12 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n>>> On 16.12.15 01:04, Stefan Beller wrote:\n>>>> The man page of read(2) says:\n>>>>\n>>>>   EAGAIN The file descriptor fd refers to a file other than a socket\n>>>>        and has been marked nonblocking (O_NONBLOCK), and the read\n>>>>        would block.\n>>>>\n>>>>   EAGAIN or EWOULDBLOCK\n>>>>        The file descriptor fd refers to a socket and has been marked\n>>>>        nonblocking (O_NONBLOCK), and the read would block.  POSIX.1-2001\n>>>>        allows either error to be returned for this case, and does not\n>>>>        require these constants to have the same value, so a portable\n>>>>        application should check for both possibilities.\n>>>>\n>>>> If we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.\n>>>> As the intent of xread is to read as much as possible either until the\n>>>> fd is EOF or an actual error occurs, we can ease the feeder of the fd\n>>>> by not spinning the whole time, but rather wait for it politely by not\n>>>> busy waiting.\n>>>>\n>>>> We should not care if the call to poll failed, as we're in an infinite\n>>>> loop and can only get out with the correct read().\n>>> I'm not sure if this is valid under all circumstances:\n>>> This is what \"man poll\" says under Linux:\n>>> []\n>>>  ENOMEM There was no space to allocate file descriptor tables.\n>>> []\n>>> And this is from Mac OS, (\"BSD System Calls Manual\")\n>>> ERRORS\n>>>      Poll() will fail if:\n>>>\n>>>      [EAGAIN]           Allocation of internal data structures fails.  A sub-\n>>>                         sequent request may succeed.\n>>> And this is opengroup:\n>>> http://pubs.opengroup.org/onlinepubs/9699919799//functions/poll.html:\n>>> [EAGAIN]\n>>>     The allocation of internal data structures failed but a subsequent request may succeed.\n>>>\n>>> read() may return EAGAIN, but poll() may fail to allocate memory, and fail.\n>>> Is it always guaranteed that the loop is terminated?\n>>\n>> In case poll fails (assume a no op for it), the logic should not have\n>> changed by this patch?\n>>\n>> Looking closely:\n>>\n>>>>       while (1) {\n>>>>               nr = read(fd, buf, len);\n>>>> -             if ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n>>>> -                     continue;\n>>>> +             if (nr < 0) {\n>>>> +                     if (errno == EINTR)\n>>>> +                             continue;\n>>>> +                     if (errno == EAGAIN || errno == EWOULDBLOCK) {\n>>>> +                             struct pollfd pfd;\n>>>> +                             pfd.events = POLLIN;\n>>>> +                             pfd.fd = fd;\n>>>> +                             /*\n>>>> +                              * it is OK if this poll() failed; we\n>>>> +                              * want to leave this infinite loop\n>>>> +                              * only when read() returns with\n>>>> +                              * success, or an expected failure,\n>>>> +                              * which would be checked by the next\n>>>> +                              * call to read(2).\n>>>> +                              */\n>>>> +                             poll(&pfd, 1, -1);\n>>\n>> Or do you mean to insert another continue in here?\n> I was thinking that we run into similar loop as before:\n> read() returns -1; errno = EAGAIN  /* No data to read */\n\nwhich is expected for non blocking fds,\n\n> poll() returns -1; errno = EAGAIN /* poll failed. If the fd was OK, the failure may be temporaly,\n>                                     as much as poll() can see.\n>                                     But most probably we run out ouf memory */\n\nBefore this patch we would not have asked poll, but had just a continue here,\nso I think we need to have it here again no matter of the return code\nof the poll.\n\nIf poll determines it is low on memory, this should not make this function fail,\nwe can still do as good as we did before by just asking read\nrepeatedly again, though?\n\nSo I'd be convinced now we'd want to have:\n\n    poll(&pfd, 1, -1); /* this is only buying time\n                        for the fd to deliver data, in case it fails\n                        we don't care but just fall back to old\n                        behavior before this patch with busy spinning*/\n    continue;\n\n\n>\n> So the code would look like this:\n>\n>    if (!poll(&pfd, 1, -1))\n>       return -1;\n>\n>\n>>\n>\n>>>> +                     }\n>>>> +             }\n>>>>               return nr;\n>>>>       }\n>>>>  }\n>>>>\n>>>\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>>\n>\n"},{"id":"274708","messageId":"20151218030752.GA8432@sigill.intra.peff.net","threadId":"41011","inReplyTo":"1450224252-16833-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 2/7] xread: poll on non blocking fds","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-18T03:07:53Z","receivedAt":"2015-12-18T03:07:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 15, 2015 at 04:04:07PM -0800, Stefan Beller wrote:\n\n> If we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.\n> As the intent of xread is to read as much as possible either until the\n> fd is EOF or an actual error occurs, we can ease the feeder of the fd\n> by not spinning the whole time, but rather wait for it politely by not\n> busy waiting.\n\nI'm not sure this second sentence is true. The point of xread() is to do\na single read that makes forward progress (or returns a real error), not\nto read as much as we can.\n\nThe rationale seems more to me like:\n\n  The point of xread() is to perform read() until we either get some\n  data, or encounter a \"real\" error. We do not count EINTR or EAGAIN as\n  \"real\" errors, as a reader trying to make forward progress would\n  generally just repeat the read, so we loop for them as a convenience.\n\n  In the case of EINTR, trying the read() again immediately is fine; a\n  signal interrupted us, but there is no reason to think the read()\n  would not succeed if we tried it again.\n\n  For EAGAIN, however, we know that the fd must have set O_NONBLOCK. In\n  this case reading again immediately is not likely to produce results,\n  and we will chew up CPU spinning on read() calls. Instead, let's\n  insert a poll() to block until we actually get data.\n\nI also don't know that this patch necessarily needs to be part of the\nparallel-fetch series anymore. We are not setting O_NONBLOCK ourselves,\nand I don't think the correctness of the new strbuf_read_once() is\nimpacted by this. I.e., I think this optimization applies equally well\nto the hundreds of existing xread() calls, should they unexpectedly be\nfed an O_NONBLOCK descriptor.\n\nThat said, I do not mind it here, but I think it could be its own\nseparate topic if you wanted to reduce the complexity of the\nparallel-fetch topic.\n\n> We should not care if the call to poll failed, as we're in an infinite\n> loop and can only get out with the correct read().\n\nYou might want to expand on \"correct\" here. I think you mean \"can only\nget out with a read() that returns data or a \"real\" error\" (I am reusing\nthe terminology I introduced above; I am fine with other terminology,\ndepending on how you word the rest of it).\n\n-Peff\n"},{"id":"274709","messageId":"20151218031336.GA8467@sigill.intra.peff.net","threadId":"41011","inReplyTo":"56731E19.7050504@web.de","subject":"Re: [PATCHv2 2/7] xread: poll on non blocking fds","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-18T03:13:36Z","receivedAt":"2015-12-18T03:13:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 17, 2015 at 09:42:01PM +0100, Torsten Bögershausen wrote:\n\n> > Or do you mean to insert another continue in here?\n> I was thinking that we run into similar loop as before:\n> read() returns -1; errno = EAGAIN  /* No data to read */\n> poll() returns -1; errno = EAGAIN /* poll failed. If the fd was OK, the failure may be temporaly,\n>                                     as much as poll() can see.\n>                                     But most probably we run out ouf memory */\n> \n> So the code would look like this:\n> \n>    if (!poll(&pfd, 1, -1))\n>       return -1;\n\nThat changes the semantics of the function. The poll() is just a\nconvenience to avoid spinning. If it fails, with Stefan's patch[1] the\nworst case is that we would spin on read() and poll(), instead of\nactually blocking in the poll().\n\nBut if we return on poll() failure, now the caller will see errors from\npoll() even though they don't know or care that we called poll() in the\nfirst place. Consider what would happen with your code if read got\nEAGAIN and then poll got EINTR. We would report an error, even though\nthe whole point of xread() is to loop on these conditions.\n\n-Peff\n\n[1] Stefan's patch does have a bug. It forgets to \"continue\" after\n    calling poll, which means we will block with poll and _then_ exit\n    with -1, instead of restarting the loop.\n"},{"id":"274710","messageId":"20151218031432.GB8467@sigill.intra.peff.net","threadId":"41011","inReplyTo":"CAGZ79kYHjH7QgChn6_GU9iLrtyouFQFuMAZKADZofgHjBS73-A@mail.gmail.com","subject":"Re: [PATCHv2 2/7] xread: poll on non blocking fds","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-18T03:14:33Z","receivedAt":"2015-12-18T03:14:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 17, 2015 at 12:51:08PM -0800, Stefan Beller wrote:\n\n> > poll() returns -1; errno = EAGAIN /* poll failed. If the fd was OK, the failure may be temporaly,\n> >                                     as much as poll() can see.\n> >                                     But most probably we run out ouf memory */\n> \n> Before this patch we would not have asked poll, but had just a continue here,\n> so I think we need to have it here again no matter of the return code\n> of the poll.\n> \n> If poll determines it is low on memory, this should not make this function fail,\n> we can still do as good as we did before by just asking read\n> repeatedly again, though?\n> \n> So I'd be convinced now we'd want to have:\n> \n>     poll(&pfd, 1, -1); /* this is only buying time\n>                         for the fd to deliver data, in case it fails\n>                         we don't care but just fall back to old\n>                         behavior before this patch with busy spinning*/\n>     continue;\n\nRight. I think that is the only sensible thing, and your comment\nexplains perfectly what is going on.\n\n-Peff\n"},{"id":"274718","messageId":"5673C8E6.8010001@web.de","threadId":"41011","inReplyTo":"20151218031336.GA8467@sigill.intra.peff.net","subject":"Re: [PATCHv2 2/7] xread: poll on non blocking fds","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-12-18T08:50:46Z","receivedAt":"2015-12-18T08:50:46Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 18.12.15 04:13, Jeff King wrote:\n> On Thu, Dec 17, 2015 at 09:42:01PM +0100, Torsten Bögershausen wrote:\n> \n>>> Or do you mean to insert another continue in here?\n>> I was thinking that we run into similar loop as before:\n>> read() returns -1; errno = EAGAIN  /* No data to read */\n>> poll() returns -1; errno = EAGAIN /* poll failed. If the fd was OK, the failure may be temporaly,\n>>                                     as much as poll() can see.\n>>                                     But most probably we run out ouf memory */\n>>\n>> So the code would look like this:\n>>\n>>    if (!poll(&pfd, 1, -1))\n>>       return -1;\n> \n> That changes the semantics of the function. The poll() is just a\n> convenience to avoid spinning. If it fails, with Stefan's patch[1] the\n> worst case is that we would spin on read() and poll(), instead of\n> actually blocking in the poll().\n> \n> But if we return on poll() failure, now the caller will see errors from\n> poll() even though they don't know or care that we called poll() in the\n> first place. Consider what would happen with your code if read got\n> EAGAIN and then poll got EINTR. We would report an error, even though\n> the whole point of xread() is to loop on these conditions.\n> \n> -Peff\n> \n> [1] Stefan's patch does have a bug. It forgets to \"continue\" after\n>     calling poll, which means we will block with poll and _then_ exit\n>     with -1, instead of restarting the loop.\n> --\nI love this group: Curing one bug with another doesn't work :-)\n\n/* So the code v2 would look like this: */\n\n    if (!poll(&pfd, 1, -1)) {\n        if (errno == EINTR)\n            continue;\n         return -1; /* poll() failed, this is serious. */\n    }\n"},{"id":"274725","messageId":"20151218181233.GA29057@sigill.intra.peff.net","threadId":"41011","inReplyTo":"5673C8E6.8010001@web.de","subject":"Re: [PATCHv2 2/7] xread: poll on non blocking fds","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-18T18:12:33Z","receivedAt":"2015-12-18T18:12:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 18, 2015 at 09:50:46AM +0100, Torsten Bögershausen wrote:\n\n> >> So the code would look like this:\n> >>\n> >>    if (!poll(&pfd, 1, -1))\n> >>       return -1;\n> > \n> > That changes the semantics of the function. The poll() is just a\n> > convenience to avoid spinning. If it fails, with Stefan's patch[1] the\n> > worst case is that we would spin on read() and poll(), instead of\n> > actually blocking in the poll().\n> > \n> > But if we return on poll() failure, now the caller will see errors from\n> > poll() even though they don't know or care that we called poll() in the\n> > first place. Consider what would happen with your code if read got\n> > EAGAIN and then poll got EINTR. We would report an error, even though\n> > the whole point of xread() is to loop on these conditions.\n> [...]\n>\n> /* So the code v2 would look like this: */\n> \n>     if (!poll(&pfd, 1, -1)) {\n>         if (errno == EINTR)\n>             continue;\n>          return -1; /* poll() failed, this is serious. */\n>     }\n\nThat solves the EINTR problem, but I still don't see why we want to\nreturn -1. The caller asked us to read(). We know that read() did not\nfail with an actual error. Yet we are going to return an error to the\nuser, with errno set to something related only to poll(). I think we are\nbetter off to keep the same semantics from the caller's point of view:\nwe loop until read() returns forward progress or a real error, and\nanything else we do is a behind-the-scenes optimization.\n\nBTW, I am assuming you mean:\n\n  if (poll(&pfd, 1, -1) < 0)\n\t...\n\nin your examples. Returning \"0\" means that poll timed out, but of course\nwe are not providing a timeout.\n\n-Peff\n"}]}