{"thread":{"id":"56944","subject":"[PATCH] fetch-pack: ignore SIGPIPE when writing to index-pack","startedAt":"2021-11-19T20:59:50Z","lastAt":"2021-11-19T22:32:09Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"441790","messageId":"YZgQD3lrw4+i4EMd@coredump.intra.peff.net","threadId":"56944","inReplyTo":null,"subject":"[PATCH] fetch-pack: ignore SIGPIPE when writing to index-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-19T20:58:55Z","receivedAt":"2021-11-19T20:59:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When fetching, we send the incoming pack to index-pack (or\nunpack-objects) via the sideband demuxer. If index-pack hits an error\n(e.g., because an object fails fsck), then it will die immediately. This\nmay cause us to get SIGPIPE on the fetch, as we're still trying to write\npack contents from the sideband demuxer (which is typically a thread,\nand thus takes down the whole fetch process).\n\nYou can see this in action with:\n\n  ./t5702-protocol-v2.sh --stress --run=59\n\nwhich ends with (wrapped for readability):\n\n  test_must_fail: died by signal 13: git -c protocol.version=2 \\\n    -c transfer.fsckobjects=1 -c fetch.uriprotocols=http,https \\\n    clone http://127.0.0.1:5708/smart/http_parent http_child\n  not ok 59 - packfile-uri with transfer.fsckobjects fails on bad object\n\nThis is mostly cosmetic. The actual error of interest (in this case, the\nobject that failed the fsck check) comes from index-pack straight to\nstderr, so the user still sees it. They _might_ even see fetch-pack\ncomplaining about index-pack failing, because the main thread is racing\nwith the sideband-demuxer. But they'll definitely see the signal death\nin the exit code, which is what the test is complaining about.\n\nWe can make this more predictable by just ignoring SIGPIPE. The sideband\ndemuxer uses write_or_die(), so it will notice and stop (gracefully,\nbecause we hook die_routine() to exit just the thread). And during this\nsection we're not writing anywhere else where we'd be concerned about\nSIGPIPE preventing us from wasting effort writing to nowhere.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI wondered if the receive-pack side would have a similar problem, but\nthere I think it's accepting the input directly from the network. So the\nclient-side push may see a premature hangup. But there the SIGPIPE goes\nto pack-objects (which is writing straight to the network), and the\nparent send-pack/push process detects this; see the comment near the\n\"141\" check at the end of send-pack.c:pack_objects().\n\nI cc'd Jonathan because it's his test, but really I think that is just\nluck. AFAICT this would be a problem for any fetch where\ntransfer.fsckObjects detects a problem.\n\n fetch-pack.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a9604f35a3..8fe3a49c1c 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -25,6 +25,7 @@\n #include \"shallow.h\"\n #include \"commit-reach.h\"\n #include \"commit-graph.h\"\n+#include \"sigchain.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -956,6 +957,8 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\tstrvec_push(index_pack_args, cmd.args.v[i]);\n \t}\n \n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\n \tcmd.in = demux.out;\n \tcmd.git_cmd = 1;\n \tif (start_command(&cmd))\n@@ -986,6 +989,8 @@ static int get_pack(struct fetch_pack_args *args,\n \tif (use_sideband && finish_async(&demux))\n \t\tdie(_(\"error in sideband demultiplexer\"));\n \n+\tsigchain_pop(SIGPIPE);\n+\n \t/*\n \t * Now that index-pack has succeeded, write the promisor file using the\n \t * obtained .keep filename if necessary\n-- \n2.34.0.635.gde47f84164\n"},{"id":"441800","messageId":"xmqqy25jokft.fsf@gitster.g","threadId":"56944","inReplyTo":"YZgQD3lrw4+i4EMd@coredump.intra.peff.net","subject":"Re: [PATCH] fetch-pack: ignore SIGPIPE when writing to index-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-19T22:21:58Z","receivedAt":"2021-11-19T22:22:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When fetching, we send the incoming pack to index-pack (or\n> unpack-objects) via the sideband demuxer. If index-pack hits an error\n> (e.g., because an object fails fsck), then it will die immediately. This\n> may cause us to get SIGPIPE on the fetch, as we're still trying to write\n> pack contents from the sideband demuxer (which is typically a thread,\n> and thus takes down the whole fetch process).\n\nSo, ... we'd die anyway and won't update the refs and anything that\nleaves permanent damage to the repository either way, but we choose\na better way to die by not taking SIGPIPE, but to get an error from\none of the write()s or the final close(), which will lead us to more\n\"controlled\" death using the normal error path?\n\n> This is mostly cosmetic. The actual error of interest (in this case, the\n> object that failed the fsck check) comes from index-pack straight to\n> stderr, so the user still sees it. They _might_ even see fetch-pack\n> complaining about index-pack failing, because the main thread is racing\n> with the sideband-demuxer. But they'll definitely see the signal death\n> in the exit code, which is what the test is complaining about.\n\nOK.\n\n> We can make this more predictable by just ignoring SIGPIPE. The sideband\n> demuxer uses write_or_die(), so it will notice and stop (gracefully,\n> because we hook die_routine() to exit just the thread). And during this\n> section we're not writing anywhere else where we'd be concerned about\n> SIGPIPE preventing us from wasting effort writing to nowhere.\n\nOK.\n\n> +#include \"sigchain.h\"\n>  \n>  static int transfer_unpack_limit = -1;\n>  static int fetch_unpack_limit = -1;\n> @@ -956,6 +957,8 @@ static int get_pack(struct fetch_pack_args *args,\n>  \t\t\tstrvec_push(index_pack_args, cmd.args.v[i]);\n>  \t}\n>  \n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n> +\n>  \tcmd.in = demux.out;\n>  \tcmd.git_cmd = 1;\n>  \tif (start_command(&cmd))\n> @@ -986,6 +989,8 @@ static int get_pack(struct fetch_pack_args *args,\n>  \tif (use_sideband && finish_async(&demux))\n>  \t\tdie(_(\"error in sideband demultiplexer\"));\n>  \n> +\tsigchain_pop(SIGPIPE);\n> +\n>  \t/*\n>  \t * Now that index-pack has succeeded, write the promisor file using the\n>  \t * obtained .keep filename if necessary\n\nThanks.\n"},{"id":"441801","messageId":"YZgl5qYbAyAsixmo@coredump.intra.peff.net","threadId":"56944","inReplyTo":"xmqqy25jokft.fsf@gitster.g","subject":"Re: [PATCH] fetch-pack: ignore SIGPIPE when writing to index-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-19T22:32:06Z","receivedAt":"2021-11-19T22:32:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 19, 2021 at 02:21:58PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > When fetching, we send the incoming pack to index-pack (or\n> > unpack-objects) via the sideband demuxer. If index-pack hits an error\n> > (e.g., because an object fails fsck), then it will die immediately. This\n> > may cause us to get SIGPIPE on the fetch, as we're still trying to write\n> > pack contents from the sideband demuxer (which is typically a thread,\n> > and thus takes down the whole fetch process).\n> \n> So, ... we'd die anyway and won't update the refs and anything that\n> leaves permanent damage to the repository either way, but we choose\n> a better way to die by not taking SIGPIPE, but to get an error from\n> one of the write()s or the final close(), which will lead us to more\n> \"controlled\" death using the normal error path?\n\nYes, exactly. We'll be exiting either way; it's just a matter of racily\nchanging the exit code and possibly the error message from the parent\nfetch process.\n\nSo I do doubt this really hurts users much in practice, but making the\ntests robust seems worth it to me (because I found it after tracking\ndown a flaky test failure). And because it _is_ a race in the code, I\nfixed it there rather than papering over the SIGPIPE exit in the test\nscript.\n\n-Peff\n"}]}