{"thread":{"id":"41495","subject":"[PATCH 0/4] robustifying t5504","startedAt":"2016-02-24T07:36:03Z","lastAt":"2016-02-24T07:48:36Z","messageCount":5,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"279126","messageId":"20160224073603.GA9163@sigill.intra.peff.net","threadId":"41495","inReplyTo":null,"subject":"[PATCH 0/4] robustifying t5504","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-24T07:36:03Z","receivedAt":"2016-02-24T07:36:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I got a spurious test failure on t5504 while running the test suite\ntoday. This is the result of my quest through the SIGPIPE rabbit hole.\n\nSince this is not the first time I've investigated tests failing under\nload, I finally broke down and wrote a helper script. It probably needs\nsome adapting for other people's environments, but I'll share it here in\ncase anyone is interested:\n\n-- >8 --\n#!/bin/sh\n\ntest=$1; shift\ntest=$(cd t && echo $test*.sh)\n: ${GIT_STRESS_LOAD:=$(( 2 * $(grep -c ^processor /proc/cpuinfo)))}\n: ${GIT_STRESS_ROOT:=/var/ram/git-stress}\n\nmkdir -p \"$GIT_STRESS_ROOT\" || exit 1\nrm -f \"$GIT_STRESS_ROOT/fail\"\ntrap 'echo aborted >\"$GIT_STRESS_ROOT/fail\"' TERM INT HUP\nfor i in $(seq $GIT_STRESS_LOAD); do\n\t(\n\t\tcd t &&\n\t\twhile ! test -e \"$GIT_STRESS_ROOT/fail\"\n\t\tdo\n\t\t\tif ./$test \\\n\t\t\t   --root=\"$GIT_STRESS_ROOT/trash-$i\" \\\n\t\t\t   -v -i >\"$GIT_STRESS_ROOT/output-$i\" 2>&1\n\t\t\tthen\n\t\t\t\techo >&2 \"OK $i\"\n\t\t\telse\n\t\t\t\techo >&2 \"FAIL $i\"\n\t\t\t\tcp \"$GIT_STRESS_ROOT/output-$i\" \"$GIT_STRESS_ROOT/fail\"\n\t\t\tfi\n\t\tdone\n\t) &\ndone\nwait\ncat \"$GIT_STRESS_ROOT/fail\"\n-- 8< --\n\nYou can run it like \"./stress t5504\", and it will run t5504 over and\nover in parallel until one instance fails. Without this patch series,\nt5504 generally fails for me within 30 seconds or so. With it, I can run\nfor several minutes without problems.\n\n  [1/4]: write_or_die: handle EPIPE in async threads\n  [2/4]: fetch-pack: ignore SIGPIPE in sideband demuxer\n  [3/4]: test_must_fail: report number of unexpected signal\n  [4/4]: t5504: handle expected output from SIGPIPE death\n\n-Peff\n"},{"id":"279127","messageId":"20160224074016.GA9481@sigill.intra.peff.net","threadId":"41495","inReplyTo":"20160224073603.GA9163@sigill.intra.peff.net","subject":"[PATCH 1/4] write_or_die: handle EPIPE in async threads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-24T07:40:16Z","receivedAt":"2016-02-24T07:40:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When write_or_die() sees EPIPE, it treats it specially by\nconverting it into a SIGPIPE death. We obviously cannot\nignore it, as the write has failed and the caller expects us\nto die. But likewise, we cannot just call die(), because\nprinting any message at all would be a nuisance during\nnormal operations.\n\nHowever, this is a problem if write_or_die() is called from\na thread. Our raised signal ends up killing the whole\nprocess, when logically we just need to kill the thread\n(after all, if we are ignoring SIGPIPE, there is good reason\nto think that the main thread is expecting to handle it).\n\nInside an async thread, the die() code already does the\nright thing, because we use our custom die_async() routine,\nwhich calls pthread_join(). So ideally we would piggy-back\non that, and simply call:\n\n  die_quietly_with_code(141);\n\nor similar. But refactoring the die code to do this is\nsurprisingly non-trivial. The die_routines themselves handle\nboth printing and the decision of the exit code. Every one\nof them would have to be modified to take new parameters for\nthe code, and to tell us to be quiet.\n\nInstead, we can just teach write_or_die() to check for the\nasync case and handle it specially. We do have to build an\ninterface to abstract the async exit, but it's simple and\nself-contained. If we had many call-sites that wanted to do\nthis die_quietly_with_code(), this approach wouldn't scale\nas well, but we don't. This is the only place where do this\nweird exit trick.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is needed for patch 2, wherein we (surprise!) call write_or_die()\nin a thread and want to ignore SIGPIPE.\n\nObviously another solution is \"don't call write_or_die() in a thread\",\nand that would work for patch 2 here. But it can be hard to know whether\nor not write_or_die() is called deep in the call stack. So even though\nthis solution is slightly more complex, I like that it behaves correctly\nin all cases.\n\n run-command.c  | 10 ++++++++++\n run-command.h  |  1 +\n write_or_die.c |  4 ++++\n 3 files changed, 15 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex cdf0184..426387b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -635,6 +635,11 @@ int in_async(void)\n \treturn !pthread_equal(main_thread, pthread_self());\n }\n \n+void async_exit(int code)\n+{\n+\tpthread_exit((void *)(intptr_t)code);\n+}\n+\n #else\n \n static struct {\n@@ -680,6 +685,11 @@ int in_async(void)\n \treturn process_is_async;\n }\n \n+int async_exit(int code)\n+{\n+\texit(code);\n+}\n+\n #endif\n \n int start_async(struct async *async)\ndiff --git a/run-command.h b/run-command.h\nindex d5a57f9..42917e8 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -121,6 +121,7 @@ struct async {\n int start_async(struct async *async);\n int finish_async(struct async *async);\n int in_async(void);\n+void NORETURN async_exit(int code);\n \n /**\n  * This callback should initialize the child process and preload the\ndiff --git a/write_or_die.c b/write_or_die.c\nindex e7afe7a..49e80aa 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -1,8 +1,12 @@\n #include \"cache.h\"\n+#include \"run-command.h\"\n \n static void check_pipe(int err)\n {\n \tif (err == EPIPE) {\n+\t\tif (in_async())\n+\t\t\tasync_exit(141);\n+\n \t\tsignal(SIGPIPE, SIG_DFL);\n \t\traise(SIGPIPE);\n \t\t/* Should never happen, but just in case... */\n-- \n2.7.2.645.g4e1306c\n"},{"id":"279128","messageId":"20160224074457.GB9481@sigill.intra.peff.net","threadId":"41495","inReplyTo":"20160224073603.GA9163@sigill.intra.peff.net","subject":"[PATCH 2/4] fetch-pack: ignore SIGPIPE in sideband demuxer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-24T07:44:58Z","receivedAt":"2016-02-24T07:44:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the other side feeds us a bogus pack, index-pack (or\nunpack-objects) may die early, before consuming all of its\ninput. As a result, the sideband demuxer may get SIGPIPE\n(racily, depending on whether our data made it into the pipe\nbuffer or not). If this happens and we are compiled with\npthread support, it will take down the main thread, too.\n\nThis isn't the end of the world, as the main process will\njust die() anyway when it sees index-pack failed. But it\ndoes mean we don't get a chance to say \"fatal: index-pack\nfailed\" or similar. And it also means that we racily fail\nt5504, as we sometimes die() and sometimes are killed by\nSIGPIPE.\n\nSo let's ignore SIGPIPE while demuxing the sideband. We are\nalready careful to check the return value of write(), so we\nwon't waste time writing to a broken pipe. The caller will\nnotice the error return from the async thread, though in\npractice we don't even get that far, as we die() as soon as\nwe see that index-pack failed.\n\nThe non-sideband case is already fine; we let index-pack\nread straight from the socket, so there is no SIGPIPE at\nall. Technically the non-threaded async case is also OK\nwithout this (the forked async process gets SIGPIPE), but\nit's not worth distinguishing from the threaded case here.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIt's tempting to just ignore SIGPIPE in all async processes,\nas other sites may have similar problems (and we cannot\ndepend on SIGPIPE taking down the main thread anyway, as\nasync code may be implemented via fork()).\n\nBut that errs in the opposite direction. If an async process\ndoes not check the return value of write(), it may\nwastefully keep writing.\n\nIt would also mean implementing a pthread_sigmask() wrapper\non Windows. I have no idea how feasible that would be.\n\n fetch-pack.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 01e34b6..f96f6df 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -15,6 +15,7 @@\n #include \"version.h\"\n #include \"prio-queue.h\"\n #include \"sha1-array.h\"\n+#include \"sigchain.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -671,9 +672,12 @@ static int everything_local(struct fetch_pack_args *args,\n static int sideband_demux(int in, int out, void *data)\n {\n \tint *xd = data;\n+\tint ret;\n \n-\tint ret = recv_sideband(\"fetch-pack\", xd[0], out);\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\tret = recv_sideband(\"fetch-pack\", xd[0], out);\n \tclose(out);\n+\tsigchain_pop(SIGPIPE);\n \treturn ret;\n }\n \n-- \n2.7.2.645.g4e1306c\n"},{"id":"279129","messageId":"20160224074548.GC9481@sigill.intra.peff.net","threadId":"41495","inReplyTo":"20160224073603.GA9163@sigill.intra.peff.net","subject":"[PATCH 3/4] test_must_fail: report number of unexpected signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-24T07:45:49Z","receivedAt":"2016-02-24T07:45:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a command is marked as test_must_fail but dies with a\nsignal, we consider that a problem and report the error to\nstderr. However, we don't say _which_ signal; knowing that\ncan make debugging easier. Let's share as much as we know.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNot necessary for the fix, obviously, but I implemented this while\ntrying to figure out what in the world was going on with the\nwrite_or_die() thing. :)\n\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex c64e5a5..8d99eb3 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -617,7 +617,7 @@ test_must_fail () {\n \t\treturn 0\n \telif test $exit_code -gt 129 && test $exit_code -le 192\n \tthen\n-\t\techo >&2 \"test_must_fail: died by signal: $*\"\n+\t\techo >&2 \"test_must_fail: died by signal $(($exit_code - 128)): $*\"\n \t\treturn 1\n \telif test $exit_code -eq 127\n \tthen\n-- \n2.7.2.645.g4e1306c\n"},{"id":"279131","messageId":"20160224074836.GD9481@sigill.intra.peff.net","threadId":"41495","inReplyTo":"20160224073603.GA9163@sigill.intra.peff.net","subject":"[PATCH 4/4] t5504: handle expected output from SIGPIPE death","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-24T07:48:36Z","receivedAt":"2016-02-24T07:48:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 8bf4bec (add \"ok=sigpipe\" to test_must_fail and use\nit to fix flaky tests, 2015-11-27) taught t5504 to handle\n\"git push\" racily exiting with SIGPIPE rather than failing.\n\nHowever, one of the tests checks the output of the command,\nas well. In the SIGPIPE case, we will not have produced any\noutput. If we want the test to be truly non-flaky, we have\nto accept either output.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm not sure we're really accomplishing anything with this test_cmp\nanymore. We'll complain only if we get _some_ output, and it's not the\nexpected output. I'm not sure if that's actually useful.\n\nIt looks like 8bf4bec just dropped the test_cmp completely in one of the\ncases. So an alternative here would be to do the same.\n\nWe _could_ also tighten this, to make sure the output matches the exit\ncode we got (right now we can get code=128 with blank output and not\ncomplain, even though that's clearly bogus). But we'd have to abandon\ntest_must_fail and do things manually.\n\n t/t5504-fetch-receive-strict.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 89224ed..a3e12d2 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -101,7 +101,10 @@ test_expect_success 'push with receive.fsckobjects' '\n \t\tgit config transfer.fsckobjects false\n \t) &&\n \ttest_must_fail ok=sigpipe git push --porcelain dst master:refs/heads/test >act &&\n-\ttest_cmp exp act\n+\t{\n+\t\ttest_cmp exp act ||\n+\t\t! test -s act\n+\t}\n '\n \n test_expect_success 'push with transfer.fsckobjects' '\n-- \n2.7.2.645.g4e1306c\n"}]}