{"thread":{"id":"33346","subject":"[PATCH 1/4] run-command: add new check_command helper","startedAt":"2013-04-01T21:46:40Z","lastAt":"2013-04-02T09:36:43Z","messageCount":21,"participants":["Felipe Contreras","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"212826","messageId":"1364852804-31875-1-git-send-email-felipe.contreras@gmail.com","threadId":"33346","inReplyTo":null,"subject":"[PATCH 0/4] run-command: new check_command helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-01T21:46:40Z","receivedAt":"2013-04-01T21:46:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nThe first patch does all the work, the second patch uses it; basically, this is\nneeded so the transport-helper code is able to check if the remote-helper child\nis stilll running. Without this support, the status of the remote-helper files\nand configuration can end up very badly when errors occur, to the point where\nthe user is unable to use it any more.\n\nThe rest of the patches are for testing purposes only. I ran all the tests with\nthese, and I didn't see any problems.\n\nCheers.\n\nFelipe Contreras (4):\n  run-command: add new check_command helper\n  transport-helper: check if remote helper is alive\n  tmp: remote-helper: add timers to catch errors\n  tmp: run-command: code to exercise check_command\n\n git-remote-testgit        | 12 +++++++++++\n run-command.c             | 52 +++++++++++++++++++++++++++++++++++++++++------\n run-command.h             |  6 ++++++\n t/t5801-remote-helpers.sh | 19 +++++++++++++++++\n transport-helper.c        | 11 ++++++++++\n 5 files changed, 94 insertions(+), 6 deletions(-)\n\n-- \n1.8.2\n"},{"id":"212822","messageId":"1364852804-31875-2-git-send-email-felipe.contreras@gmail.com","threadId":"33346","inReplyTo":"1364852804-31875-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/4] run-command: add new check_command helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-01T21:46:41Z","receivedAt":"2013-04-01T21:46:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"And persistent_waitpid() to recover the information from the last run.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n run-command.c | 46 ++++++++++++++++++++++++++++++++++++++++------\n run-command.h |  6 ++++++\n 2 files changed, 46 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 07e27ff..16833df 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -226,14 +226,27 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n-static int wait_or_whine(pid_t pid, const char *argv0)\n+static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *stat_loc)\n+{\n+\tif (cmd->last_wait.code) {\n+\t\terrno = cmd->last_wait.failed_errno;\n+\t\t*stat_loc = cmd->last_wait.status;\n+\t\treturn errno ? -1 : pid;\n+\t} else {\n+\t\tpid_t waiting;\n+\t\twhile ((waiting = waitpid(pid, stat_loc, 0)) < 0 && errno == EINTR)\n+\t\t\t;\t/* nothing */\n+\t\treturn waiting;\n+\t}\n+}\n+\n+static int wait_or_whine(struct child_process *cmd, pid_t pid, const char *argv0)\n {\n \tint status, code = -1;\n \tpid_t waiting;\n \tint failed_errno = 0;\n \n-\twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n-\t\t;\t/* nothing */\n+\twaiting = persistent_waitpid(cmd, pid, &status);\n \n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n@@ -437,7 +450,7 @@ fail_pipe:\n \t\t * At this point we know that fork() succeeded, but execvp()\n \t\t * failed. Errors have been reported to our stderr.\n \t\t */\n-\t\twait_or_whine(cmd->pid, cmd->argv[0]);\n+\t\twait_or_whine(cmd, cmd->pid, cmd->argv[0]);\n \t\tfailed_errno = errno;\n \t\tcmd->pid = -1;\n \t}\n@@ -542,7 +555,7 @@ fail_pipe:\n \n int finish_command(struct child_process *cmd)\n {\n-\treturn wait_or_whine(cmd->pid, cmd->argv[0]);\n+\treturn wait_or_whine(cmd, cmd->pid, cmd->argv[0]);\n }\n \n int run_command(struct child_process *cmd)\n@@ -553,6 +566,27 @@ int run_command(struct child_process *cmd)\n \treturn finish_command(cmd);\n }\n \n+int check_command(struct child_process *cmd)\n+{\n+\tint status;\n+\tpid_t waiting;\n+\tint failed_errno = 0;\n+\n+\twaiting = waitpid(cmd->pid, &status, WNOHANG);\n+\n+\tif (waiting != cmd->pid)\n+\t\treturn 1;\n+\n+\tif (waiting < 0)\n+\t\tfailed_errno = errno;\n+\n+\tcmd->last_wait.code = -1;\n+\tcmd->last_wait.failed_errno = failed_errno;\n+\tcmd->last_wait.status = status;\n+\n+\treturn 0;\n+}\n+\n static void prepare_run_command_v_opt(struct child_process *cmd,\n \t\t\t\t      const char **argv,\n \t\t\t\t      int opt)\n@@ -729,7 +763,7 @@ error:\n int finish_async(struct async *async)\n {\n #ifdef NO_PTHREADS\n-\treturn wait_or_whine(async->pid, \"child process\");\n+\treturn wait_or_whine(cmd, async->pid, \"child process\");\n #else\n \tvoid *ret = (void *)(intptr_t)(-1);\n \ndiff --git a/run-command.h b/run-command.h\nindex 221ce33..66aaac7 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -39,11 +39,17 @@ struct child_process {\n \tunsigned stdout_to_stderr:1;\n \tunsigned use_shell:1;\n \tunsigned clean_on_exit:1;\n+\tstruct last_wait {\n+\t\tint code;\n+\t\tint failed_errno;\n+\t\tint status;\n+\t} last_wait;\n };\n \n int start_command(struct child_process *);\n int finish_command(struct child_process *);\n int run_command(struct child_process *);\n+int check_command(struct child_process *);\n \n extern char *find_hook(const char *name);\n extern int run_hook(const char *index_file, const char *name, ...);\n-- \n1.8.2\n"},{"id":"212823","messageId":"1364852804-31875-3-git-send-email-felipe.contreras@gmail.com","threadId":"33346","inReplyTo":"1364852804-31875-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/4] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-01T21:46:42Z","receivedAt":"2013-04-01T21:46:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Otherwise transport-helper will continue checking for refs and other\nthings what will confuse the user more.\n---\n git-remote-testgit        | 11 +++++++++++\n t/t5801-remote-helpers.sh | 19 +++++++++++++++++++\n transport-helper.c        |  8 ++++++++\n 3 files changed, 38 insertions(+)\n\ndiff --git a/git-remote-testgit b/git-remote-testgit\nindex b395c8d..ca0cf09 100755\n--- a/git-remote-testgit\n+++ b/git-remote-testgit\n@@ -61,12 +61,23 @@ do\n \t\t\techo \"feature import-marks=$gitmarks\"\n \t\t\techo \"feature export-marks=$gitmarks\"\n \t\tfi\n+\n+\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n+\t\tthen\n+\t\t\texit -1\n+\t\tfi\n+\n \t\techo \"feature done\"\n \t\tgit fast-export \"${testgitmarks_args[@]}\" $refs |\n \t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n \t\techo \"done\"\n \t\t;;\n \texport)\n+\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n+\t\tthen\n+\t\t\texit -1\n+\t\tfi\n+\n \t\tbefore=$(git for-each-ref --format='%(refname) %(objectname)')\n \n \t\tgit fast-import \"${testgitmarks_args[@]}\" --quiet\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex f387027..26e9a5b 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' '\n \tcompare_refs local dup server dup\n '\n \n+test_expect_success 'proper failure checks for fetching' '\n+\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\texport GIT_REMOTE_TESTGIT_FAILURE &&\n+\tcd local &&\n+\ttest_must_fail git fetch 2>&1 | \\\n+\t\tgrep \"Error while running helper\"\n+\t)\n+'\n+\n+# We sleep to give fast-export a chance to catch the SIGPIPE\n+test_expect_failure 'proper failure checks for pushing' '\n+\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\texport GIT_REMOTE_TESTGIT_FAILURE &&\n+\tcd local &&\n+\ttest_must_fail git push --all 2>&1 | \\\n+\t\tgrep \"Error while running helper\"\n+\t)\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex cb3ef7d..dfdfa7a 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport,\n \n \tif (finish_command(&fastimport))\n \t\tdie(\"Error while running fast-import\");\n+\n+\tif (!check_command(data->helper))\n+\t\tdie(\"Error while running helper\");\n+\n \targv_array_free_detached(fastimport.argv);\n \n \t/*\n@@ -818,6 +822,10 @@ static int push_refs_with_export(struct transport *transport,\n \n \tif (finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n+\n+\tif (!check_command(data->helper))\n+\t\tdie(\"Error while running helper\");\n+\n \tpush_update_refs_status(data, remote_refs);\n \treturn 0;\n }\n-- \n1.8.2\n"},{"id":"212824","messageId":"1364852804-31875-4-git-send-email-felipe.contreras@gmail.com","threadId":"33346","inReplyTo":"1364852804-31875-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 3/4] tmp: remote-helper: add timers to catch errors","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-01T21:46:43Z","receivedAt":"2013-04-01T21:46:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This way the test reliably succeeds (in catching the failure).\n\nNot sure what's the proper way to do this, but here it is for the\nrecord.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n git-remote-testgit        | 1 +\n t/t5801-remote-helpers.sh | 2 +-\n transport-helper.c        | 3 +++\n 3 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/git-remote-testgit b/git-remote-testgit\nindex ca0cf09..6ae1b7f 100755\n--- a/git-remote-testgit\n+++ b/git-remote-testgit\n@@ -75,6 +75,7 @@ do\n \texport)\n \t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n \t\tthen\n+\t\t\tsleep 1 # don't let fast-export get SIGPIPE\n \t\t\texit -1\n \t\tfi\n \ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 26e9a5b..5bb2ca4 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -176,7 +176,7 @@ test_expect_success 'proper failure checks for fetching' '\n '\n \n # We sleep to give fast-export a chance to catch the SIGPIPE\n-test_expect_failure 'proper failure checks for pushing' '\n+test_expect_success 'proper failure checks for pushing' '\n \t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n \texport GIT_REMOTE_TESTGIT_FAILURE &&\n \tcd local &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex dfdfa7a..f0d28aa 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -823,6 +823,9 @@ static int push_refs_with_export(struct transport *transport,\n \tif (finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n \n+\tif (getenv(\"GIT_REMOTE_TESTGIT_FAILURE\"))\n+\t\tsleep(2);\n+\n \tif (!check_command(data->helper))\n \t\tdie(\"Error while running helper\");\n \n-- \n1.8.2\n"},{"id":"212827","messageId":"1364852804-31875-5-git-send-email-felipe.contreras@gmail.com","threadId":"33346","inReplyTo":"1364852804-31875-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 4/4] tmp: run-command: code to exercise check_command","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-01T21:46:44Z","receivedAt":"2013-04-01T21:46:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Do not merge!\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n run-command.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 16833df..6f20d5f 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -234,6 +234,7 @@ static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *stat_\n \t\treturn errno ? -1 : pid;\n \t} else {\n \t\tpid_t waiting;\n+\t\tdie(\"not supposed to happen\");\n \t\twhile ((waiting = waitpid(pid, stat_loc, 0)) < 0 && errno == EINTR)\n \t\t\t;\t/* nothing */\n \t\treturn waiting;\n@@ -246,6 +247,11 @@ static int wait_or_whine(struct child_process *cmd, pid_t pid, const char *argv0\n \tpid_t waiting;\n \tint failed_errno = 0;\n \n+\tdo {\n+\t\tif (!check_command(cmd))\n+\t\t\tbreak;\n+\t} while(1);\n+\n \twaiting = persistent_waitpid(cmd, pid, &status);\n \n \tif (waiting < 0) {\n-- \n1.8.2\n"},{"id":"212833","messageId":"20130401232326.GA30935@sigill.intra.peff.net","threadId":"33346","inReplyTo":"1364852804-31875-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/4] run-command: add new check_command helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-01T23:23:26Z","receivedAt":"2013-04-01T23:23:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2013 at 03:46:41PM -0600, Felipe Contreras wrote:\n\n> And persistent_waitpid() to recover the information from the last run.\n> \n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n\nA little background would be nice here...what problem are we solving?\n\n> -static int wait_or_whine(pid_t pid, const char *argv0)\n> +static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *stat_loc)\n> +{\n> +\tif (cmd->last_wait.code) {\n> +\t\terrno = cmd->last_wait.failed_errno;\n> +\t\t*stat_loc = cmd->last_wait.status;\n> +\t\treturn errno ? -1 : pid;\n> +\t} else {\n> +\t\tpid_t waiting;\n> +\t\twhile ((waiting = waitpid(pid, stat_loc, 0)) < 0 && errno == EINTR)\n> +\t\t\t;\t/* nothing */\n> +\t\treturn waiting;\n> +\t}\n> +}\n\nSo it looks we are trying to save the waitpid state from a previous run\nand use the saved value. Otherwise, waitpid as normal.\n\nWe loop on EINTR when we actually call waitpid(). But we don't check\nwhether the saved errno is waitpid. What happens if we EINTR during the\nsaved call to waitpid?\n\n> +static int wait_or_whine(struct child_process *cmd, pid_t pid, const char *argv0)\n>  {\n>  \tint status, code = -1;\n>  \tpid_t waiting;\n>  \tint failed_errno = 0;\n>  \n> -\twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n> -\t\t;\t/* nothing */\n> +\twaiting = persistent_waitpid(cmd, pid, &status);\n>  \n>  \tif (waiting < 0) {\n>  \t\tfailed_errno = errno;\n\nWe now take argv0 into wait_or_whine. But I don't see it being used.\nWhat's it for?\n\n> +int check_command(struct child_process *cmd)\n> +{\n> +\tint status;\n> +\tpid_t waiting;\n> +\tint failed_errno = 0;\n> +\n> +\twaiting = waitpid(cmd->pid, &status, WNOHANG);\n\nThis might return the pid if it has died, -1 if there was an error, or 0\nif the process still exists but hasn't died. So...\n\n> +\tif (waiting != cmd->pid)\n> +\t\treturn 1;\n> +\n> +\tif (waiting < 0)\n> +\t\tfailed_errno = errno;\n\nHow would we ever trigger this second conditional? It makes sense to\nreturn 1 when \"waiting == 0\", as that is saying \"yes, your process is\nstill running\" (though documenting the return either at the top of the\nfunction or in the commit message would be helpful)\n\nBut if we get an error from waitpid, we would also return 1, which\ndoesn't make sense (especially if it is something like EINTR -- I don't\nknow offhand if we can get EINTR during WNOHANG. It should not block,\nbut I don't know if it can race with a signal).\n\n> +\tcmd->last_wait.code = -1;\n> +\tcmd->last_wait.failed_errno = failed_errno;\n> +\tcmd->last_wait.status = status;\n\nSince we can only get here when waiting == cmd->pid, failed_errno is\nalways 0. We do correctly record the status. Why is code set to -1?  It\nseems to be used as a flag to say \"this structure is valid\". Should it\nbe defined as \"unsigned valid:1;\" instead?\n\n-Peff\n"},{"id":"212834","messageId":"20130401233313.GB30935@sigill.intra.peff.net","threadId":"33346","inReplyTo":"1364852804-31875-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/4] transport-helper: check if remote helper is alive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-01T23:33:13Z","receivedAt":"2013-04-01T23:33:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2013 at 03:46:42PM -0600, Felipe Contreras wrote:\n\n> Otherwise transport-helper will continue checking for refs and other\n> things what will confuse the user more.\n> [...]\n> diff --git a/transport-helper.c b/transport-helper.c\n> index cb3ef7d..dfdfa7a 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport,\n>  \n>  \tif (finish_command(&fastimport))\n>  \t\tdie(\"Error while running fast-import\");\n> +\n> +\tif (!check_command(data->helper))\n> +\t\tdie(\"Error while running helper\");\n> +\n>  \targv_array_free_detached(fastimport.argv);\n\nCan you be more specific about what happens when we miss the death here,\nwhat happens next, etc?\n\nChecking asynchronously for death like this is subject to a race\ncondition; the helper may be about to die but not have died yet. In\npractice we may catch some cases, but this seems like an indication that\nthe protocol is not well thought-out. Usually we would wait for a\nconfirmation over the read pipe from a child, and know that the child\nfailed when either:\n\n  1. It tells us so on the pipe.\n\n  2. The pipe closes (at which point we know it is time to reap the\n     child).\n\nWhy doesn't that scheme work here? I am not doubting you that it does\nnot; the import helper protocol is a bit of a mess, and I can easily\nbelieve it has such a problem. But I'm wondering if it's possible to\nimprove it in a more robust way.\n\n-Peff\n"},{"id":"212836","messageId":"CAMP44s3NxBexkaQa=KxJ963L29T4BAn7e+-3YVz-vgUp8jrb4A@mail.gmail.com","threadId":"33346","inReplyTo":"20130401232326.GA30935@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] run-command: add new check_command helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-01T23:58:55Z","receivedAt":"2013-04-01T23:58:55Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 1, 2013 at 5:23 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Apr 01, 2013 at 03:46:41PM -0600, Felipe Contreras wrote:\n\n>> -static int wait_or_whine(pid_t pid, const char *argv0)\n>> +static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *stat_loc)\n>> +{\n>> +     if (cmd->last_wait.code) {\n>> +             errno = cmd->last_wait.failed_errno;\n>> +             *stat_loc = cmd->last_wait.status;\n>> +             return errno ? -1 : pid;\n>> +     } else {\n>> +             pid_t waiting;\n>> +             while ((waiting = waitpid(pid, stat_loc, 0)) < 0 && errno == EINTR)\n>> +                     ;       /* nothing */\n>> +             return waiting;\n>> +     }\n>> +}\n>\n> So it looks we are trying to save the waitpid state from a previous run\n> and use the saved value. Otherwise, waitpid as normal.\n>\n> We loop on EINTR when we actually call waitpid(). But we don't check\n> whether the saved errno is waitpid. What happens if we EINTR during the\n> saved call to waitpid?\n\nAre you saying that even if we have stored the result of a waitpid\ncommand, if errno is EINTR, then we should still loop waitpid()? If\nso, I guess this would do the trick:\n\nstatic pid_t persistent_waitpid(struct child_process *cmd, pid_t pid,\nint *stat_loc)\n{\n\tpid_t waiting;\n\n\tif (cmd->last_wait.code) {\n\t\terrno = cmd->last_wait.failed_errno;\n\t\t*stat_loc = cmd->last_wait.status;\n\t\tif (errno != EINTR)\n\t\t\treturn errno ? -1 : pid;\n\t}\n\n\twhile ((waiting = waitpid(pid, stat_loc, 0)) < 0 && errno == EINTR)\n\t\t;\t/* nothing */\n\treturn waiting;\n}\n\n>> +static int wait_or_whine(struct child_process *cmd, pid_t pid, const char *argv0)\n>>  {\n>>       int status, code = -1;\n>>       pid_t waiting;\n>>       int failed_errno = 0;\n>>\n>> -     while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n>> -             ;       /* nothing */\n>> +     waiting = persistent_waitpid(cmd, pid, &status);\n>>\n>>       if (waiting < 0) {\n>>               failed_errno = errno;\n>\n> We now take argv0 into wait_or_whine. But I don't see it being used.\n> What's it for?\n\nIt was there before:\n-static int wait_or_whine(pid_t pid, const char *argv0)\n+static int wait_or_whine(struct child_process *cmd, pid_t pid, const\nchar *argv0)\n\n>> +int check_command(struct child_process *cmd)\n>> +{\n>> +     int status;\n>> +     pid_t waiting;\n>> +     int failed_errno = 0;\n>> +\n>> +     waiting = waitpid(cmd->pid, &status, WNOHANG);\n>\n> This might return the pid if it has died, -1 if there was an error, or 0\n> if the process still exists but hasn't died. So...\n>\n>> +     if (waiting != cmd->pid)\n>> +             return 1;\n>> +\n>> +     if (waiting < 0)\n>> +             failed_errno = errno;\n>\n> How would we ever trigger this second conditional? It makes sense to\n> return 1 when \"waiting == 0\", as that is saying \"yes, your process is\n> still running\" (though documenting the return either at the top of the\n> function or in the commit message would be helpful)\n>\n> But if we get an error from waitpid, we would also return 1, which\n> doesn't make sense (especially if it is something like EINTR -- I don't\n> know offhand if we can get EINTR during WNOHANG. It should not block,\n> but I don't know if it can race with a signal).\n\nHow about this?\n\nif (waiting >= 0 && waiting != cmd->pid)\n\t\treturn 1;\n\n>> +     cmd->last_wait.code = -1;\n>> +     cmd->last_wait.failed_errno = failed_errno;\n>> +     cmd->last_wait.status = status;\n>\n> Since we can only get here when waiting == cmd->pid,\n\nNo, also when waiting < 0.\n\n> Why is code set to -1?  It\n> seems to be used as a flag to say \"this structure is valid\". Should it\n> be defined as \"unsigned valid:1;\" instead?\n\nYeap. I was using it for other purposes before, 'valid' would be better now.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"212837","messageId":"CAMP44s0uJ4ivNLw984CXWYk5HcKevuUJmpYOiyqbT1QJDaYd0Q@mail.gmail.com","threadId":"33346","inReplyTo":"20130401233313.GB30935@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T00:12:45Z","receivedAt":"2013-04-02T00:12:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 1, 2013 at 5:33 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Apr 01, 2013 at 03:46:42PM -0600, Felipe Contreras wrote:\n>\n>> Otherwise transport-helper will continue checking for refs and other\n>> things what will confuse the user more.\n>> [...]\n>> diff --git a/transport-helper.c b/transport-helper.c\n>> index cb3ef7d..dfdfa7a 100644\n>> --- a/transport-helper.c\n>> +++ b/transport-helper.c\n>> @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport,\n>>\n>>       if (finish_command(&fastimport))\n>>               die(\"Error while running fast-import\");\n>> +\n>> +     if (!check_command(data->helper))\n>> +             die(\"Error while running helper\");\n>> +\n>>       argv_array_free_detached(fastimport.argv);\n>\n> Can you be more specific about what happens when we miss the death here,\n> what happens next, etc?\n\nI have seen problems sporadically, like git trying to update refs to\nobject that don't exist. I also remember seeing mismatches between the\nmarks and the remote branches refs.\n\n> Checking asynchronously for death like this is subject to a rac\n> condition; the helper may be about to die but not have died yet. In\n> practice we may catch some cases, but this seems like an indication that\n> the protocol is not well thought-out. Usually we would wait for a\n> confirmation over the read pipe from a child, and know that the child\n> failed when either:\n>\n>   1. It tells us so on the pipe.\n>\n>   2. The pipe closes (at which point we know it is time to reap the\n>      child).\n>\n> Why doesn't that scheme work here? I am not doubting you that it does\n> not; the import helper protocol is a bit of a mess, and I can easily\n> believe it has such a problem. But I'm wondering if it's possible to\n> improve it in a more robust way.\n\nThe pipe is between fast-export and the remote-helper, \"we\"\n(transport-helper) are not part of the pipe any more. That's the\nproblem.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"212838","messageId":"7v38v9ixqn.fsf@alter.siamese.dyndns.org","threadId":"33346","inReplyTo":"1364852804-31875-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/4] transport-helper: check if remote helper is alive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-02T00:26:24Z","receivedAt":"2013-04-02T00:26:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Otherwise transport-helper will continue checking for refs and other\n> things what will confuse the user more.\n> ---\n\nSign-off?\n\n>  git-remote-testgit        | 11 +++++++++++\n>  t/t5801-remote-helpers.sh | 19 +++++++++++++++++++\n>  transport-helper.c        |  8 ++++++++\n>  3 files changed, 38 insertions(+)\n>\n> diff --git a/git-remote-testgit b/git-remote-testgit\n> index b395c8d..ca0cf09 100755\n> --- a/git-remote-testgit\n> +++ b/git-remote-testgit\n> @@ -61,12 +61,23 @@ do\n>  \t\t\techo \"feature import-marks=$gitmarks\"\n>  \t\t\techo \"feature export-marks=$gitmarks\"\n>  \t\tfi\n> +\n> +\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n> +\t\tthen\n> +\t\t\texit -1\n> +\t\tfi\n> +\n>  \t\techo \"feature done\"\n>  \t\tgit fast-export \"${testgitmarks_args[@]}\" $refs |\n>  \t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n>  \t\techo \"done\"\n>  \t\t;;\n>  \texport)\n> +\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n> +\t\tthen\n> +\t\t\texit -1\n> +\t\tfi\n> +\n>  \t\tbefore=$(git for-each-ref --format='%(refname) %(objectname)')\n>  \n>  \t\tgit fast-import \"${testgitmarks_args[@]}\" --quiet\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index f387027..26e9a5b 100755\n> --- a/t/t5801-remote-helpers.sh\n> +++ b/t/t5801-remote-helpers.sh\n> @@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' '\n>  \tcompare_refs local dup server dup\n>  '\n>  \n> +test_expect_success 'proper failure checks for fetching' '\n> +\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n> +\texport GIT_REMOTE_TESTGIT_FAILURE &&\n> +\tcd local &&\n> +\ttest_must_fail git fetch 2>&1 | \\\n> +\t\tgrep \"Error while running helper\"\n\nThis will not care if \"git fetch\" succeeds or fails and returns the\nexit code from grep.  Perhaps something like this instead?\n\n\t(\n\t\tGIT_REMOTE_TESTGIT_FAILURE=1 &&\n\t\texport GIT_REMOTE_TESTGIT_FAILURE &&\n\t\tcd local &&\n\t\ttest_must_fail git fetch 2>error &&\n\t\tgrep \"Error while running helper\" error\n\t)\n\n\n> +# We sleep to give fast-export a chance to catch the SIGPIPE\n> +test_expect_failure 'proper failure checks for pushing' '\n> +\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n> +\texport GIT_REMOTE_TESTGIT_FAILURE &&\n> +\tcd local &&\n> +\ttest_must_fail git push --all 2>&1 | \\\n> +\t\tgrep \"Error while running helper\"\n\nDitto.\n\n> +\t)\n> +'\n> +\n>  test_done\n> diff --git a/transport-helper.c b/transport-helper.c\n> index cb3ef7d..dfdfa7a 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport,\n>  \n>  \tif (finish_command(&fastimport))\n>  \t\tdie(\"Error while running fast-import\");\n> +\n> +\tif (!check_command(data->helper))\n> +\t\tdie(\"Error while running helper\");\n> +\n>  \targv_array_free_detached(fastimport.argv);\n>  \n>  \t/*\n> @@ -818,6 +822,10 @@ static int push_refs_with_export(struct transport *transport,\n>  \n>  \tif (finish_command(&exporter))\n>  \t\tdie(\"Error while running fast-export\");\n> +\n> +\tif (!check_command(data->helper))\n> +\t\tdie(\"Error while running helper\");\n> +\n>  \tpush_update_refs_status(data, remote_refs);\n>  \treturn 0;\n>  }\n\nOK, so the idea is that fetch_with_import() does\n\n - get_helper(transport), which spawns a helper process;\n\n - get_importer(transport, &fastimport), which spawns a fast-import\n   and make it read from the output of the helper process;\n\n - we did finish_command() to wait for the fast-import to finish,\n   expecting that the fast-import would finish when the helper stops\n   feeding it, which in turn would mean the helper would have died.\n\nThe same for the pushing side.\n\nShouldn't transport_disconnect() have called release_helper() which\nin turn calls disconnect_helper() to call finish_command() on the\nhelper to wait for that procesanyway?  Is somebody discarding return\nvalue from transport_disconnect() or the current calling site of\ntransport_disconnect() is too late to notice the error?\n\nPuzzled...\n"},{"id":"212839","messageId":"20130402022214.GA719@sigill.intra.peff.net","threadId":"33346","inReplyTo":"CAMP44s3NxBexkaQa=KxJ963L29T4BAn7e+-3YVz-vgUp8jrb4A@mail.gmail.com","subject":"Re: [PATCH 1/4] run-command: add new check_command helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-02T02:22:14Z","receivedAt":"2013-04-02T02:22:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2013 at 05:58:55PM -0600, Felipe Contreras wrote:\n\n> On Mon, Apr 1, 2013 at 5:23 PM, Jeff King <peff@peff.net> wrote:\n> > On Mon, Apr 01, 2013 at 03:46:41PM -0600, Felipe Contreras wrote:\n> \n> >> -static int wait_or_whine(pid_t pid, const char *argv0)\n> >> +static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *stat_loc)\n> >> +{\n> >> +     if (cmd->last_wait.code) {\n> >> +             errno = cmd->last_wait.failed_errno;\n> >> +             *stat_loc = cmd->last_wait.status;\n> >> +             return errno ? -1 : pid;\n> >> +     } else {\n> >> +             pid_t waiting;\n> >> +             while ((waiting = waitpid(pid, stat_loc, 0)) < 0 && errno == EINTR)\n> >> +                     ;       /* nothing */\n> >> +             return waiting;\n> >> +     }\n> >> +}\n> >\n> > So it looks we are trying to save the waitpid state from a previous run\n> > and use the saved value. Otherwise, waitpid as normal.\n> >\n> > We loop on EINTR when we actually call waitpid(). But we don't check\n> > whether the saved errno is waitpid. What happens if we EINTR during the\n> > saved call to waitpid?\n> \n> Are you saying that even if we have stored the result of a waitpid\n> command, if errno is EINTR, then we should still loop waitpid()? If\n> so, I guess this would do the trick:\n\nYes, I think that would work. Though I wonder if it is even worth\nstoring EINTR at all in the first place. It tells us nothing. In fact,\ndoes storing any error condition really tell us anything? The two states\nwe are interested in at this point are:\n\n  1. We have reaped the child via waitpid; here is its status.\n\n  2. We have not (either we did not try, it was not dead yet, or we were\n     not able to due to an error). We should now try it again.\n\nIf we got EINTR the first time around, we would likely get the \"real\"\nanswer this time. If we get anything else (like EINVAL or ECHILD), then\nwe would get the same thing again calling waitpid() later.\n\n> > We now take argv0 into wait_or_whine. But I don't see it being used.\n> > What's it for?\n> \n> It was there before:\n> -static int wait_or_whine(pid_t pid, const char *argv0)\n> +static int wait_or_whine(struct child_process *cmd, pid_t pid, const\n> char *argv0)\n\nAh, sorry, I misread the diff. We are adding \"cmd\", not \"argv0\".\n\n> >> +     if (waiting != cmd->pid)\n> >> +             return 1;\n> >> +\n> >> +     if (waiting < 0)\n> >> +             failed_errno = errno;\n> >\n> > How would we ever trigger this second conditional?\n> [...]\n> How about this?\n> \n> if (waiting >= 0 && waiting != cmd->pid)\n> \t\treturn 1;\n\nThat would trigger the rest of your code in the error case, which I\nthink was your original intent. But then we return \"0\" from\ncheck_command. Is that right?\n\nThere are three states we can be in from calling waitpid:\n\n  1. The process is dead.\n\n  2. The process is not dead.\n\n  3. We could not determine which because waitpid returned an error.\n\nIt is clear that check_command is trying to tell its caller (1) or (2);\nbut what should it say in case of (3)?\n\nNaively, given how patch 2 uses it, I think it would actually make sense\nfor it to return 1. That is, the semantics are \"return 0 if and only if\nthe pid is verified to be dead; otherwise return 1\".\n\nBut if we know from reading waitpid(3) that waitpid should only fail due\nto EINTR, or due to bogus arguments (e.g., a pid that does not exist or\nhas already been reaped), then maybe something like this makes sense:\n\n  while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n          ; /* nothing */\n\n  /* pid definitely still going */\n  if (!waiting)\n          return 1;\n\n  /* pid definitely died */\n  if (waiting == cmd->pid) {\n          cmd->last_status.valid = 1;\n          cmd->last_status.status = status;\n          return 0;\n  }\n\n  /*\n   * this should never happen, since we handed waitpid() a single\n   * pid, so it should either return that pid, 0, or an error.\n   */\n  if (waiting > 0)\n          die(\"BUG: waitpid reported a random pid?\");\n\n  /*\n   * otherwise, we have an error. Assume the pid is gone, since that\n   * is the only reason for waitpid to report a problem besides EINTR.\n   * We don't bother recording errno, since we can just repeat\n   * the waitpid again later.\n   */\n   return 0;\n\n> >> +     cmd->last_wait.code = -1;\n> >> +     cmd->last_wait.failed_errno = failed_errno;\n> >> +     cmd->last_wait.status = status;\n> >\n> > Since we can only get here when waiting == cmd->pid,\n> \n> No, also when waiting < 0.\n\nAfter the fix above, yes; in the original we would always have exited\nalready.\n\nAs an aside, should check_command be able to be called twice? That is,\nshould it first check for cmd->last_status.valid and return early if\nsomebody has already reaped the child? It doesn't matter for the code\nyou add in patch 2, but it seems like it would give the least surprise\nto somebody trying to use it later.\n\n-Peff\n"},{"id":"212840","messageId":"20130402023024.GB719@sigill.intra.peff.net","threadId":"33346","inReplyTo":"CAMP44s0uJ4ivNLw984CXWYk5HcKevuUJmpYOiyqbT1QJDaYd0Q@mail.gmail.com","subject":"Re: [PATCH 2/4] transport-helper: check if remote helper is alive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-02T02:30:25Z","receivedAt":"2013-04-02T02:30:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2013 at 06:12:45PM -0600, Felipe Contreras wrote:\n\n> > Checking asynchronously for death like this is subject to a rac\n> > condition; the helper may be about to die but not have died yet. In\n> > practice we may catch some cases, but this seems like an indication that\n> > the protocol is not well thought-out. Usually we would wait for a\n> > confirmation over the read pipe from a child, and know that the child\n> > failed when either:\n> >\n> >   1. It tells us so on the pipe.\n> >\n> >   2. The pipe closes (at which point we know it is time to reap the\n> >      child).\n> >\n> > Why doesn't that scheme work here? I am not doubting you that it does\n> > not; the import helper protocol is a bit of a mess, and I can easily\n> > believe it has such a problem. But I'm wondering if it's possible to\n> > improve it in a more robust way.\n> \n> The pipe is between fast-export and the remote-helper, \"we\"\n> (transport-helper) are not part of the pipe any more. That's the\n> problem.\n\nSo in fetch_with_import, we have a remote-helper, and we have a\nbidirectional pipe to it. We then call get_importer, which starts\nfast-import, whose stdin is connected to the stdout of the remote\nhelper. We tell the remote-helper to run the import, then we wait for\nfast-import to finish (and complain if it fails).\n\nThen what? We seem to do some more work, which I think is what causes\nthe errors you see; but should we instead be reaping the helper at this\npoint unconditionally? Its stdout has presumably been flushed out to\nfast-import; is there anything else for us to get from it besides its\nexit code?\n\n-Peff\n"},{"id":"212841","messageId":"CAMP44s2VcLUE=PHE=iSsgd67_ZV4N6b5Ya76Rc22xtSzK-Xz0g@mail.gmail.com","threadId":"33346","inReplyTo":"20130402023024.GB719@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T04:51:20Z","receivedAt":"2013-04-02T04:51:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 1, 2013 at 8:30 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Apr 01, 2013 at 06:12:45PM -0600, Felipe Contreras wrote:\n>\n>> > Checking asynchronously for death like this is subject to a rac\n>> > condition; the helper may be about to die but not have died yet. In\n>> > practice we may catch some cases, but this seems like an indication that\n>> > the protocol is not well thought-out. Usually we would wait for a\n>> > confirmation over the read pipe from a child, and know that the child\n>> > failed when either:\n>> >\n>> >   1. It tells us so on the pipe.\n>> >\n>> >   2. The pipe closes (at which point we know it is time to reap the\n>> >      child).\n>> >\n>> > Why doesn't that scheme work here? I am not doubting you that it does\n>> > not; the import helper protocol is a bit of a mess, and I can easily\n>> > believe it has such a problem. But I'm wondering if it's possible to\n>> > improve it in a more robust way.\n>>\n>> The pipe is between fast-export and the remote-helper, \"we\"\n>> (transport-helper) are not part of the pipe any more. That's the\n>> problem.\n>\n> So in fetch_with_import, we have a remote-helper, and we have a\n> bidirectional pipe to it. We then call get_importer, which starts\n> fast-import, whose stdin is connected to the stdout of the remote\n> helper. We tell the remote-helper to run the import, then we wait for\n> fast-import to finish (and complain if it fails).\n>\n> Then what? We seem to do some more work, which I think is what causes\n> the errors you see; but should we instead be reaping the helper at this\n> point unconditionally? Its stdout has presumably been flushed out to\n> fast-import; is there anything else for us to get from it besides its\n> exit code?\n\nThe problem is not with import, since fast-import would generally wait\nproperly for a 'done' status, the problem is with export. Also, the\ndesign is such that the remote-helper stays alive, even after\nfast-export has finished.\n\n-- \nFelipe Contreras\n"},{"id":"212842","messageId":"CAMP44s0m_Jzn=DBEwkZjJqrxU1MoAs1V3ES5C2cdSK5Eg9cROQ@mail.gmail.com","threadId":"33346","inReplyTo":"7v38v9ixqn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/4] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T04:58:16Z","receivedAt":"2013-04-02T04:58:16Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 1, 2013 at 6:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> OK, so the idea is that fetch_with_import() does\n>\n>  - get_helper(transport), which spawns a helper process;\n>\n>  - get_importer(transport, &fastimport), which spawns a fast-import\n>    and make it read from the output of the helper process;\n>\n>  - we did finish_command() to wait for the fast-import to finish,\n>    expecting that the fast-import would finish when the helper stops\n>    feeding it, which in turn would mean the helper would have died.\n>\n> The same for the pushing side.\n\nThe difference with the pushing side is that it's the helper the one\nwaiting for fast-export and it can easily die.\n\n> Shouldn't transport_disconnect() have called release_helper() which\n> in turn calls disconnect_helper() to call finish_command() on the\n> helper to wait for that procesanyway?  Is somebody discarding return\n> value from transport_disconnect() or the current calling site of\n> transport_disconnect() is too late to notice the error?\n\nIt's too late to notice the error. However, only in the case of pushing.\n\n-- \nFelipe Contreras\n"},{"id":"212843","messageId":"20130402050154.GA21328@sigill.intra.peff.net","threadId":"33346","inReplyTo":"CAMP44s2VcLUE=PHE=iSsgd67_ZV4N6b5Ya76Rc22xtSzK-Xz0g@mail.gmail.com","subject":"Re: [PATCH 2/4] transport-helper: check if remote helper is alive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-02T05:01:54Z","receivedAt":"2013-04-02T05:01:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2013 at 10:51:20PM -0600, Felipe Contreras wrote:\n\n> > So in fetch_with_import, we have a remote-helper, and we have a\n> > bidirectional pipe to it. We then call get_importer, which starts\n> > fast-import, whose stdin is connected to the stdout of the remote\n> > helper. We tell the remote-helper to run the import, then we wait for\n> > fast-import to finish (and complain if it fails).\n> >\n> > Then what? We seem to do some more work, which I think is what causes\n> > the errors you see; but should we instead be reaping the helper at this\n> > point unconditionally? Its stdout has presumably been flushed out to\n> > fast-import; is there anything else for us to get from it besides its\n> > exit code?\n> \n> The problem is not with import, since fast-import would generally wait\n> properly for a 'done' status, the problem is with export.\n\nYour patch modified fetch_with_import. Are you saying that it isn't\nnecessary to do so?\n\n> Also, the design is such that the remote-helper stays alive, even\n> after fast-export has finished.\n\nSo if we expect to be able to communicate with the remote-helper after\nfast-export has exited, is it a protocol failure that the helper does\nnot say \"yes, I finished the export\" or similar? If so, can we fix that?\n\nI am not too familiar with this protocol, but it looks like we read from\nhelper->out right after closing the exporter, to get the ref statuses.\nShouldn't we be detecting the error if the helper hangs up there?\n\n-Peff\n"},{"id":"212844","messageId":"CAMP44s3-DnBM7Tm_+igLN+c5MGrbJNXUBwcj1HvGCPEJEwJSBQ@mail.gmail.com","threadId":"33346","inReplyTo":"20130402022214.GA719@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] run-command: add new check_command helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T05:11:20Z","receivedAt":"2013-04-02T05:11:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 1, 2013 at 8:22 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Apr 01, 2013 at 05:58:55PM -0600, Felipe Contreras wrote:\n\n>> Are you saying that even if we have stored the result of a waitpid\n>> command, if errno is EINTR, then we should still loop waitpid()? If\n>> so, I guess this would do the trick:\n>\n> Yes, I think that would work. Though I wonder if it is even worth\n> storing EINTR at all in the first place. It tells us nothing. In fact,\n> does storing any error condition really tell us anything?\n\nProbably not, I just tried to minimize the potential behavior changes.\n\n> The two states\n> we are interested in at this point are:\n>\n>   1. We have reaped the child via waitpid; here is its status.\n>\n>   2. We have not (either we did not try, it was not dead yet, or we were\n>      not able to due to an error). We should now try it again.\n>\n> If we got EINTR the first time around, we would likely get the \"real\"\n> answer this time. If we get anything else (like EINVAL or ECHILD), then\n> we would get the same thing again calling waitpid() later.\n>\n>> > We now take argv0 into wait_or_whine. But I don't see it being used.\n>> > What's it for?\n>>\n>> It was there before:\n>> -static int wait_or_whine(pid_t pid, const char *argv0)\n>> +static int wait_or_whine(struct child_process *cmd, pid_t pid, const\n>> char *argv0)\n>\n> Ah, sorry, I misread the diff. We are adding \"cmd\", not \"argv0\".\n\nYeah, which in fact was already there before.\n\n> That would trigger the rest of your code in the error case, which I\n> think was your original intent. But then we return \"0\" from\n> check_command. Is that right?\n>\n> There are three states we can be in from calling waitpid:\n>\n>   1. The process is dead.\n>\n>   2. The process is not dead.\n>\n>   3. We could not determine which because waitpid returned an error.\n>\n> It is clear that check_command is trying to tell its caller (1) or (2);\n> but what should it say in case of (3)?\n>\n> Naively, given how patch 2 uses it, I think it would actually make sense\n> for it to return 1. That is, the semantics are \"return 0 if and only if\n> the pid is verified to be dead; otherwise return 1\".\n\nI thought if there was an error that constituted a failure.\n\n> But if we know from reading waitpid(3) that waitpid should only fail due\n> to EINTR, or due to bogus arguments (e.g., a pid that does not exist or\n> has already been reaped), then maybe something like this makes sense:\n>\n>   while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n>           ; /* nothing */\n\nBut we don't want to wait synchronously here, we just want to ping.\n\n>   /* pid definitely still going */\n>   if (!waiting)\n>           return 1;\n>\n>   /* pid definitely died */\n>   if (waiting == cmd->pid) {\n>           cmd->last_status.valid = 1;\n>           cmd->last_status.status = status;\n>           return 0;\n>   }\n>\n>   /*\n>    * this should never happen, since we handed waitpid() a single\n>    * pid, so it should either return that pid, 0, or an error.\n>    */\n>   if (waiting > 0)\n>           die(\"BUG: waitpid reported a random pid?\");\n>\n>   /*\n>    * otherwise, we have an error. Assume the pid is gone, since that\n>    * is the only reason for waitpid to report a problem besides EINTR.\n>    * We don't bother recording errno, since we can just repeat\n>    * the waitpid again later.\n>    */\n>    return 0;\n\nThe rest makes sense.\n\n>> >> +     cmd->last_wait.code = -1;\n>> >> +     cmd->last_wait.failed_errno = failed_errno;\n>> >> +     cmd->last_wait.status = status;\n>> >\n>> > Since we can only get here when waiting == cmd->pid,\n>>\n>> No, also when waiting < 0.\n>\n> After the fix above, yes; in the original we would always have exited\n> already.\n\nNo:\n\n+       if (waiting != cmd->pid)\n+               return 1;\n\nIf waiting < 0, waiting != cmd->pid, and therefore this return is not\ntriggered, and there's only one more return at the end of the\nfunction.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"212845","messageId":"20130402051407.GA21906@sigill.intra.peff.net","threadId":"33346","inReplyTo":"CAMP44s3-DnBM7Tm_+igLN+c5MGrbJNXUBwcj1HvGCPEJEwJSBQ@mail.gmail.com","subject":"Re: [PATCH 1/4] run-command: add new check_command helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-02T05:14:07Z","receivedAt":"2013-04-02T05:14:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2013 at 11:11:20PM -0600, Felipe Contreras wrote:\n\n> > But if we know from reading waitpid(3) that waitpid should only fail due\n> > to EINTR, or due to bogus arguments (e.g., a pid that does not exist or\n> > has already been reaped), then maybe something like this makes sense:\n> >\n> >   while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n> >           ; /* nothing */\n> \n> But we don't want to wait synchronously here, we just want to ping.\n\nYeah, sorry, I forgot the WNOHANG there.\n\n> > After the fix above, yes; in the original we would always have exited\n> > already.\n> \n> No:\n> \n> +       if (waiting != cmd->pid)\n> +               return 1;\n> \n> If waiting < 0, waiting != cmd->pid, and therefore this return is not\n> triggered, and there's only one more return at the end of the\n> function.\n\nAre my eyes not working? If waiting < 0, then waiting != cmd->pid, and\ntherefore this return _is_ triggered.\n\n-Peff\n"},{"id":"212846","messageId":"CAMP44s3yNGfW4aoHB90_D-OFrzchgvAugzdmOYHOey3wYkUxjg@mail.gmail.com","threadId":"33346","inReplyTo":"20130402050154.GA21328@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T05:19:42Z","receivedAt":"2013-04-02T05:19:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 1, 2013 at 11:01 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Apr 01, 2013 at 10:51:20PM -0600, Felipe Contreras wrote:\n>\n>> > So in fetch_with_import, we have a remote-helper, and we have a\n>> > bidirectional pipe to it. We then call get_importer, which starts\n>> > fast-import, whose stdin is connected to the stdout of the remote\n>> > helper. We tell the remote-helper to run the import, then we wait for\n>> > fast-import to finish (and complain if it fails).\n>> >\n>> > Then what? We seem to do some more work, which I think is what causes\n>> > the errors you see; but should we instead be reaping the helper at this\n>> > point unconditionally? Its stdout has presumably been flushed out to\n>> > fast-import; is there anything else for us to get from it besides its\n>> > exit code?\n>>\n>> The problem is not with import, since fast-import would generally wait\n>> properly for a 'done' status, the problem is with export.\n>\n> Your patch modified fetch_with_import. Are you saying that it isn't\n> necessary to do so?\n\nIt's not, I added it for symmetry. But that's the case *if* the\nremote-helper is properly using the \"done\" feature.\n\n>> Also, the design is such that the remote-helper stays alive, even\n>> after fast-export has finished.\n>\n> So if we expect to be able to communicate with the remote-helper after\n> fast-export has exited, is it a protocol failure that the helper does\n> not say \"yes, I finished the export\" or similar? If so, can we fix that?\n>\n> I am not too familiar with this protocol, but it looks like we read from\n> helper->out right after closing the exporter, to get the ref statuses.\n> Shouldn't we be detecting the error if the helper hangs up there?\n\nI guess that should be possible, I'll give that a try.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"212848","messageId":"CAMP44s3oVJs_sWpemFVGSkj-u_SrjsxE1mYja+Scx3PTBXC67Q@mail.gmail.com","threadId":"33346","inReplyTo":"20130402051407.GA21906@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] run-command: add new check_command helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T05:22:36Z","receivedAt":"2013-04-02T05:22:36Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 1, 2013 at 11:14 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Apr 01, 2013 at 11:11:20PM -0600, Felipe Contreras wrote:\n>\n>> > But if we know from reading waitpid(3) that waitpid should only fail due\n>> > to EINTR, or due to bogus arguments (e.g., a pid that does not exist or\n>> > has already been reaped), then maybe something like this makes sense:\n>> >\n>> >   while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n>> >           ; /* nothing */\n>>\n>> But we don't want to wait synchronously here, we just want to ping.\n>\n> Yeah, sorry, I forgot the WNOHANG there.\n\nIt still can potentially stay in a loop for some cycles.\n\n>> > After the fix above, yes; in the original we would always have exited\n>> > already.\n>>\n>> No:\n>>\n>> +       if (waiting != cmd->pid)\n>> +               return 1;\n>>\n>> If waiting < 0, waiting != cmd->pid, and therefore this return is not\n>> triggered, and there's only one more return at the end of the\n>> function.\n>\n> Are my eyes not working? If waiting < 0, then waiting != cmd->pid, and\n> therefore this return _is_ triggered.\n\nOh, right, it's only after the modification that the code works.\n\n-- \nFelipe Contreras\n"},{"id":"212849","messageId":"20130402052630.GB22089@sigill.intra.peff.net","threadId":"33346","inReplyTo":"CAMP44s3oVJs_sWpemFVGSkj-u_SrjsxE1mYja+Scx3PTBXC67Q@mail.gmail.com","subject":"Re: [PATCH 1/4] run-command: add new check_command helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-02T05:26:30Z","receivedAt":"2013-04-02T05:26:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2013 at 11:22:36PM -0600, Felipe Contreras wrote:\n\n> On Mon, Apr 1, 2013 at 11:14 PM, Jeff King <peff@peff.net> wrote:\n> > On Mon, Apr 01, 2013 at 11:11:20PM -0600, Felipe Contreras wrote:\n> >\n> >> > But if we know from reading waitpid(3) that waitpid should only fail due\n> >> > to EINTR, or due to bogus arguments (e.g., a pid that does not exist or\n> >> > has already been reaped), then maybe something like this makes sense:\n> >> >\n> >> >   while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n> >> >           ; /* nothing */\n> >>\n> >> But we don't want to wait synchronously here, we just want to ping.\n> >\n> > Yeah, sorry, I forgot the WNOHANG there.\n> \n> It still can potentially stay in a loop for some cycles.\n\nThat should be OK; it's the same loop we use in wait_or_whine (and that\nis in fact how I managed to get the WNOHANG wrong, as I copied the loop\nfrom there but forgot to update the flag variable).  A few cycles is OK,\nas it is really about handling a simultaneous signal; it should be rare\nthat we loop at all, and even rarer to loop more than a single time. On\nLinux, I don't think we will ever get EINTR at all, according to the\nmanpage; however, POSIX seems to allow EINTR even with WNOHANG.\n\n-Peff\n"},{"id":"212868","messageId":"CAMP44s2PT4YWBNJGTaYS5oy2jBEqkGFhwGjmZZc+HLsS8MO_xA@mail.gmail.com","threadId":"33346","inReplyTo":"CAMP44s3yNGfW4aoHB90_D-OFrzchgvAugzdmOYHOey3wYkUxjg@mail.gmail.com","subject":"Re: [PATCH 2/4] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T09:36:43Z","receivedAt":"2013-04-02T09:36:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 1, 2013 at 11:19 PM, Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n> On Mon, Apr 1, 2013 at 11:01 PM, Jeff King <peff@peff.net> wrote:\n>> On Mon, Apr 01, 2013 at 10:51:20PM -0600, Felipe Contreras wrote:\n>>\n>>> > So in fetch_with_import, we have a remote-helper, and we have a\n>>> > bidirectional pipe to it. We then call get_importer, which starts\n>>> > fast-import, whose stdin is connected to the stdout of the remote\n>>> > helper. We tell the remote-helper to run the import, then we wait for\n>>> > fast-import to finish (and complain if it fails).\n>>> >\n>>> > Then what? We seem to do some more work, which I think is what causes\n>>> > the errors you see; but should we instead be reaping the helper at this\n>>> > point unconditionally? Its stdout has presumably been flushed out to\n>>> > fast-import; is there anything else for us to get from it besides its\n>>> > exit code?\n>>>\n>>> The problem is not with import, since fast-import would generally wait\n>>> properly for a 'done' status, the problem is with export.\n>>\n>> Your patch modified fetch_with_import. Are you saying that it isn't\n>> necessary to do so?\n>\n> It's not, I added it for symmetry. But that's the case *if* the\n> remote-helper is properly using the \"done\" feature.\n\nActually, it is a problem, because without this check the\ntransport-helper just goes on without realizing the whole thing has\nfailed and doesn't produce a proper error message:\n\nfatal: bad object 0000000000000000000000000000000000000000\nerror: testgit::/home/felipec/dev/git/t/trash\ndirectory.t5801-remote-helpers/server did not send all necessary\nobjects\n\nIt's possible to send a ping command to the remote-helper, but doing\nso triggers a SIGPIPE. I would rather show a proper error message as\nmy patch suggests by just checking if the command is running.\n\n>>> Also, the design is such that the remote-helper stays alive, even\n>>> after fast-export has finished.\n>>\n>> So if we expect to be able to communicate with the remote-helper after\n>> fast-export has exited, is it a protocol failure that the helper does\n>> not say \"yes, I finished the export\" or similar? If so, can we fix that?\n>>\n>> I am not too familiar with this protocol, but it looks like we read from\n>> helper->out right after closing the exporter, to get the ref statuses.\n>> Shouldn't we be detecting the error if the helper hangs up there?\n>\n> I guess that should be possible, I'll give that a try.\n\nI gave this a try and it does work, but it seems rather convoluted to me:\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex f0d28aa..10b7842 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -25,7 +25,8 @@ struct helper_data {\n \t\toption : 1,\n \t\tpush : 1,\n \t\tconnect : 1,\n-\t\tno_disconnect_req : 1;\n+\t\tno_disconnect_req : 1,\n+\t\tdone_export : 1;\n \tchar *export_marks;\n \tchar *import_marks;\n \t/* These go from remote name (as in \"list\") to private name */\n@@ -46,7 +47,7 @@ static void sendline(struct helper_data *helper,\nstruct strbuf *buffer)\n \t\tdie_errno(\"Full write to remote helper failed\");\n }\n\n-static int recvline_fh(FILE *helper, struct strbuf *buffer)\n+static int recvline_fh(FILE *helper, struct strbuf *buffer, int safe)\n {\n \tstrbuf_reset(buffer);\n \tif (debug)\n@@ -54,7 +55,10 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)\n \tif (strbuf_getline(buffer, helper, '\\n') == EOF) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Remote helper quit.\\n\");\n-\t\texit(128);\n+\t\tif (safe)\n+\t\t\treturn -1;\n+\t\telse\n+\t\t\texit(128);\n \t}\n\n \tif (debug)\n@@ -64,7 +68,12 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)\n\n static int recvline(struct helper_data *helper, struct strbuf *buffer)\n {\n-\treturn recvline_fh(helper->out, buffer);\n+\treturn recvline_fh(helper->out, buffer, 0);\n+}\n+\n+static int recvline_safe(struct helper_data *helper, struct strbuf *buffer)\n+{\n+\treturn recvline_fh(helper->out, buffer, 1);\n }\n\n static void xchgline(struct helper_data *helper, struct strbuf *buffer)\n@@ -201,6 +210,8 @@ static struct child_process *get_helper(struct\ntransport *transport)\n \t\t\tstrbuf_addstr(&arg, \"--import-marks=\");\n \t\t\tstrbuf_addstr(&arg, capname + strlen(\"import-marks \"));\n \t\t\tdata->import_marks = strbuf_detach(&arg, NULL);\n+\t\t} else if (!strcmp(capname, \"done-export\")) {\n+\t\t\tdata->done_export = 1;\n \t\t} else if (mandatory) {\n \t\t\tdie(\"Unknown mandatory capability %s. This remote \"\n \t\t\t    \"helper probably needs newer version of Git.\",\n@@ -540,7 +551,7 @@ static int process_connect_service(struct\ntransport *transport,\n \t\tgoto exit;\n\n \tsendline(data, &cmdbuf);\n-\trecvline_fh(input, &cmdbuf);\n+\trecvline_fh(input, &cmdbuf, 0);\n \tif (!strcmp(cmdbuf.buf, \"\")) {\n \t\tdata->no_disconnect_req = 1;\n \t\tif (debug)\n@@ -704,19 +715,30 @@ static void push_update_ref_status(struct strbuf *buf,\n \t(*ref)->remote_status = msg;\n }\n\n-static void push_update_refs_status(struct helper_data *data,\n+static int push_update_refs_status(struct helper_data *data,\n \t\t\t\t    struct ref *remote_refs)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ref *ref = remote_refs;\n+\tint done = 0;\n+\n \tfor (;;) {\n-\t\trecvline(data, &buf);\n-\t\tif (!buf.len)\n+\t\tif (recvline_safe(data, &buf) || !buf.len)\n+\t\t\tbreak;\n+\n+\t\tif (!strncmp(buf.buf, \"done\", 4)) {\n+\t\t\tdone = 1;\n \t\t\tbreak;\n+\t\t}\n\n \t\tpush_update_ref_status(&buf, &ref, remote_refs);\n \t}\n \tstrbuf_release(&buf);\n+\n+\tif (!data->done_export)\n+\t\treturn 0;\n+\n+\treturn !done;\n }\n\n static int push_refs_with_push(struct transport *transport,\n@@ -776,8 +798,7 @@ static int push_refs_with_push(struct transport *transport,\n \tsendline(data, &buf);\n \tstrbuf_release(&buf);\n\n-\tpush_update_refs_status(data, remote_refs);\n-\treturn 0;\n+\treturn push_update_refs_status(data, remote_refs);\n }\n\n static int push_refs_with_export(struct transport *transport,\n@@ -829,8 +850,7 @@ static int push_refs_with_export(struct transport\n*transport,\n \tif (!check_command(data->helper))\n \t\tdie(\"Error while running helper\");\n\n-\tpush_update_refs_status(data, remote_refs);\n-\treturn 0;\n+\treturn push_update_refs_status(data, remote_refs);\n }\n\n static int push_refs(struct transport *transport,\n\n-- \nFelipe Contreras\n"}]}