{"thread":{"id":"49045","subject":"t5570-git-daemon fails with SIGPIPE on OSX","startedAt":"2018-08-06T15:11:28Z","lastAt":"2019-03-05T04:12:02Z","messageCount":21,"participants":["SZEDER Gábor","Jeff King","Johannes Schindelin","Junio C Hamano","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"354616","messageId":"CAM0VKj=MCS+cmOgzf_XyPeb+qZrFmuMH52-PV_NDMZA9X+rRoA@mail.gmail.com","threadId":"49045","inReplyTo":null,"subject":"t5570-git-daemon fails with SIGPIPE on OSX","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-06T15:11:13Z","receivedAt":"2018-08-06T15:11:28Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Travis CI changed its default OSX image to use XCode 9.4 on 2018-07-31\n[1].  Since then OSX build jobs fail rather frequently because of a\nSIGPIPE in the tests 'fetch notices corrupt pack' or 'fetch notices\ncorrupt idx' in 't5570-git-daemon.sh' [2].  I think this is a symptom\na real bug in Git affecting other platforms as well, but these tests\nare too lax to catch it.\n\nWhat it boils down to is this sequence:\n\n  - The test first prepares a repository containing a corrupt pack,\n    ready to be server via 'git daemon'.\n\n  - Then the test runs 'test_must_fail git fetch ....', which connects\n    to 'git daemon', which forks 'git upload-pack', which then\n    advertises refs (only HEAD) and capabilities.  So far so good.\n\n  - 'git fetch' eventually calls fetch-pack.c:find_common().  The\n    first half of this function assembles a request consisting of a\n    want and a flush pkt-line, and sends it via a send_request() call.\n\n    At this point the scheduling becomes important: let's suppose that\n    fetch is slow and upload-pack is fast.\n\n  - 'git upload-pack' receives the request, parses the want line,\n    notices the corrupt pack, responds with an 'ERR upload-pack: not\n    our ref' pkt-line, and die()s right away.\n\n  - 'git fetch' finally approaches the end of the function, where it\n    attempts to send a done pkt-line via another send_request() call\n    through the now closing TCP socket.\n\n  - What happens now seems to depend on the platform:\n\n    - On Linux, both on my machine and on Travis CI, it shows textbook\n      example behaviour: write() returns with error and sets errno to\n      ECONNRESET.  Since it happens in write_or_die(), 'git fetch'\n      die()s with 'fatal: write error: Connection reset by peer', and\n      doesn't show the error send by 'git upload-pack'; how could it,\n      it doesn't even get as far to receive upload-pack's ERR\n      pkt-line.\n\n      The test only checks that 'git fetch' fails, but it doesn't\n      check whether it failed with the right error message, so the\n      test still succeeds.  Had it checked the error message as well,\n      we most likely had noticed this issue already, it doesn't happen\n      all that rarely.\n\n    - On the new OSX images with XCode 9.4 on Travis CI the write()\n      triggers SIGPIPE right away, and 'test_must_fail' notices it and\n      fails the test.  I couldn't see any sign of an ECONNRESET or any\n      other error that we could act upon to avoid the SIGPIPE.\n\n    - On OSX with XCode 9.2 on Travis CI there is neither SIGPIPE, nor\n      ECONNRESET, but sending the request actually succeeds even\n      though there is no process on the other end of the socket\n      anymore.  'git fetch' then simply continues execution, reads and\n      parses the ERR pkt-line, and then dies()s with 'fatal: remote\n      error: upload-pack: not our ref'.  So, on the face of it, it\n      shows the desired behaviour, but I have no idea how that write()\n      could succeed instead of returning error.\n\nI don't know what happens on a real Mac as I don't have access to one;\nI figured out all the above by enabling packet tracing, adding a\ncouple of well placed tracing printf() and sleep() calls, running a\nbunch of builds on Travis CI, and looking through their logs.  But\nwithout access to a debugger and netstat and what not I can't really\ngo any further.  So I would now happily pass the baton to those who\nhave a Mac and know a thing or two about its porting issues to first\ncheck whether OSX on a real Mac shows the same behaviour as it does in\nTravis CI's virtualized(?) environment.  And then they can pass the\nbaton to those who know all the intricacies of the pack protocol and\nits implementation to decide what to do with this issue.\n\nFor a mostly reliable reproduction recipe you might want to fetch this\nbranch:\n\n  https://github.com/szeder/git t5570-git-daemon-sigpipe\n\nand then run 'make && cd t && ./t5570-git-daemon.sh -v -x'\n\n\nHave fun! ;)\n\n\n1 - https://blog.travis-ci.com/2018-07-19-xcode9-4-default-announce\n\n2 - On git.git's master:\n    https://travis-ci.org/git/git/jobs/411517552#L2717\n"},{"id":"354625","messageId":"CAM0VKjkM-THZALy20VrZ-JSMyZjUXUqp1CAoCPrezXRsBfRJ2A@mail.gmail.com","threadId":"49045","inReplyTo":"CAM0VKj=MCS+cmOgzf_XyPeb+qZrFmuMH52-PV_NDMZA9X+rRoA@mail.gmail.com","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-06T15:31:23Z","receivedAt":"2018-08-06T15:31:39Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"[Resending with Clemens' last used email address.\nClemens, please consider sending a patch to update our .mailmap file.]\n\n\nOn Mon, Aug 6, 2018 at 5:11 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>\n> Travis CI changed its default OSX image to use XCode 9.4 on 2018-07-31\n> [1].  Since then OSX build jobs fail rather frequently because of a\n> SIGPIPE in the tests 'fetch notices corrupt pack' or 'fetch notices\n> corrupt idx' in 't5570-git-daemon.sh' [2].  I think this is a symptom\n> a real bug in Git affecting other platforms as well, but these tests\n> are too lax to catch it.\n>\n> What it boils down to is this sequence:\n>\n>   - The test first prepares a repository containing a corrupt pack,\n>     ready to be server via 'git daemon'.\n>\n>   - Then the test runs 'test_must_fail git fetch ....', which connects\n>     to 'git daemon', which forks 'git upload-pack', which then\n>     advertises refs (only HEAD) and capabilities.  So far so good.\n>\n>   - 'git fetch' eventually calls fetch-pack.c:find_common().  The\n>     first half of this function assembles a request consisting of a\n>     want and a flush pkt-line, and sends it via a send_request() call.\n>\n>     At this point the scheduling becomes important: let's suppose that\n>     fetch is slow and upload-pack is fast.\n>\n>   - 'git upload-pack' receives the request, parses the want line,\n>     notices the corrupt pack, responds with an 'ERR upload-pack: not\n>     our ref' pkt-line, and die()s right away.\n>\n>   - 'git fetch' finally approaches the end of the function, where it\n>     attempts to send a done pkt-line via another send_request() call\n>     through the now closing TCP socket.\n>\n>   - What happens now seems to depend on the platform:\n>\n>     - On Linux, both on my machine and on Travis CI, it shows textbook\n>       example behaviour: write() returns with error and sets errno to\n>       ECONNRESET.  Since it happens in write_or_die(), 'git fetch'\n>       die()s with 'fatal: write error: Connection reset by peer', and\n>       doesn't show the error send by 'git upload-pack'; how could it,\n>       it doesn't even get as far to receive upload-pack's ERR\n>       pkt-line.\n>\n>       The test only checks that 'git fetch' fails, but it doesn't\n>       check whether it failed with the right error message, so the\n>       test still succeeds.  Had it checked the error message as well,\n>       we most likely had noticed this issue already, it doesn't happen\n>       all that rarely.\n>\n>     - On the new OSX images with XCode 9.4 on Travis CI the write()\n>       triggers SIGPIPE right away, and 'test_must_fail' notices it and\n>       fails the test.  I couldn't see any sign of an ECONNRESET or any\n>       other error that we could act upon to avoid the SIGPIPE.\n>\n>     - On OSX with XCode 9.2 on Travis CI there is neither SIGPIPE, nor\n>       ECONNRESET, but sending the request actually succeeds even\n>       though there is no process on the other end of the socket\n>       anymore.  'git fetch' then simply continues execution, reads and\n>       parses the ERR pkt-line, and then dies()s with 'fatal: remote\n>       error: upload-pack: not our ref'.  So, on the face of it, it\n>       shows the desired behaviour, but I have no idea how that write()\n>       could succeed instead of returning error.\n>\n> I don't know what happens on a real Mac as I don't have access to one;\n> I figured out all the above by enabling packet tracing, adding a\n> couple of well placed tracing printf() and sleep() calls, running a\n> bunch of builds on Travis CI, and looking through their logs.  But\n> without access to a debugger and netstat and what not I can't really\n> go any further.  So I would now happily pass the baton to those who\n> have a Mac and know a thing or two about its porting issues to first\n> check whether OSX on a real Mac shows the same behaviour as it does in\n> Travis CI's virtualized(?) environment.  And then they can pass the\n> baton to those who know all the intricacies of the pack protocol and\n> its implementation to decide what to do with this issue.\n>\n> For a mostly reliable reproduction recipe you might want to fetch this\n> branch:\n>\n>   https://github.com/szeder/git t5570-git-daemon-sigpipe\n>\n> and then run 'make && cd t && ./t5570-git-daemon.sh -v -x'\n>\n>\n> Have fun! ;)\n>\n>\n> 1 - https://blog.travis-ci.com/2018-07-19-xcode9-4-default-announce\n>\n> 2 - On git.git's master:\n>     https://travis-ci.org/git/git/jobs/411517552#L2717\n"},{"id":"355635","messageId":"20180814223246.GA2379@sigill.intra.peff.net","threadId":"49045","inReplyTo":"CAM0VKj=MCS+cmOgzf_XyPeb+qZrFmuMH52-PV_NDMZA9X+rRoA@mail.gmail.com","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-14T22:32:47Z","receivedAt":"2018-08-14T22:32:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 06, 2018 at 05:11:13PM +0200, SZEDER Gábor wrote:\n\n>   - 'git upload-pack' receives the request, parses the want line,\n>     notices the corrupt pack, responds with an 'ERR upload-pack: not\n>     our ref' pkt-line, and die()s right away.\n> \n>   - 'git fetch' finally approaches the end of the function, where it\n>     attempts to send a done pkt-line via another send_request() call\n>     through the now closing TCP socket.\n> \n>   - What happens now seems to depend on the platform:\n> \n>     - On Linux, both on my machine and on Travis CI, it shows textbook\n>       example behaviour: write() returns with error and sets errno to\n>       ECONNRESET.  Since it happens in write_or_die(), 'git fetch'\n>       die()s with 'fatal: write error: Connection reset by peer', and\n>       doesn't show the error send by 'git upload-pack'; how could it,\n>       it doesn't even get as far to receive upload-pack's ERR\n>       pkt-line.\n> \n>       The test only checks that 'git fetch' fails, but it doesn't\n>       check whether it failed with the right error message, so the\n>       test still succeeds.  Had it checked the error message as well,\n>       we most likely had noticed this issue already, it doesn't happen\n>       all that rarely.\n\nHmm. Traditionally we did not send ERR as part of upload-pack at all. It\nwas the message you got from git-daemon if it couldn't start the\nrequested sub-process. It was only later in bdb31eada7 (upload-pack:\nreport \"not our ref\" to client, 2017-02-23) that we started sending\nthem. So I think that is why it does not check the error message: it is\nnot expecting that case at all (and it is not actually interesting here,\nas the real problem is that the remote side is corrupt, but it sadly\ndoes not say anything so useful).\n\nI think that's somewhat tangential, though. The root of the issue is\nthis:\n\n>     - On the new OSX images with XCode 9.4 on Travis CI the write()\n>       triggers SIGPIPE right away, and 'test_must_fail' notices it and\n>       fails the test.  I couldn't see any sign of an ECONNRESET or any\n>       other error that we could act upon to avoid the SIGPIPE.\n\nRight, as soon as we get SIGPIPE we can't offer any useful message,\nbecause we're dead. I would argue that fetch should simply turn off\nSIGPIPE entirely, and rely on getting EPIPE from write(). But since\nwe're in write_or_die(), it actually turns EPIPE back into a SIGPIPE\ndeath!\n\nSo we'd probably also want to teach it to use a real write_in_full(),\nand then output a more useful message in this case. write_or_die()\nreally does produce bad messages regardless, because it doesn't know\nwhat it's writing to.\n\nThat would give us a baby step in the right direction, because at least\nwe'd always be doing a controlled die() then. And then the next step\nwould be to show the remote error message (even though it's not actually\nuseful in this case, in theory upload-pack could generate something\nbetter). And that would mean turning the die() on write into an attempt\nto drain any ERR messages before either dying or returning an error up\nthe stack.\n\nI suspect the (largely untested) patch below would make your test\nproblems go away. Or instead, we could simply add sigpipe=ok to the\ntest_must_fail invocation, but I agree with you that the current\nbehavior on OS X is not ideal (the user sees no error message).\n\n-Peff\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 5714bcbddd..3e80604562 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -188,8 +188,10 @@ static void send_request(struct fetch_pack_args *args,\n \tif (args->stateless_rpc) {\n \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n \t\tpacket_flush(fd);\n-\t} else\n-\t\twrite_or_die(fd, buf->buf, buf->len);\n+\t} else {\n+\t\tif (write_in_full(fd, buf->buf, buf->len) < 0)\n+\t\t\tdie_errno(\"unable to write to remote\");\n+\t}\n }\n \n static void insert_one_alternate_object(struct fetch_negotiator *negotiator,\n@@ -1167,7 +1169,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \n \t/* Send request */\n \tpacket_buf_flush(&req_buf);\n-\twrite_or_die(fd_out, req_buf.buf, req_buf.len);\n+\tif (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0)\n+\t\tdie_errno(\"unable to write request to remote\");\n \n \tstrbuf_release(&req_buf);\n \treturn ret;\ndiff --git a/pkt-line.c b/pkt-line.c\nindex a593c08aad..450d0801b1 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -88,13 +88,15 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n void packet_flush(int fd)\n {\n \tpacket_trace(\"0000\", 4, 1);\n-\twrite_or_die(fd, \"0000\", 4);\n+\tif (write_in_full(fd, \"0000\", 4) < 0)\n+\t\tdie_errno(\"unable to write flush packet\");\n }\n \n void packet_delim(int fd)\n {\n \tpacket_trace(\"0001\", 4, 1);\n-\twrite_or_die(fd, \"0001\", 4);\n+\tif (write_in_full(fd, \"0000\", 4) < 0)\n+\t\tdie_errno(\"unable to write delim packet\");\n }\n \n int packet_flush_gently(int fd)\n"},{"id":"355637","messageId":"20180814223737.GA9241@sigill.intra.peff.net","threadId":"49045","inReplyTo":"20180814223246.GA2379@sigill.intra.peff.net","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-14T22:37:37Z","receivedAt":"2018-08-14T22:37:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 14, 2018 at 06:32:47PM -0400, Jeff King wrote:\n\n> I suspect the (largely untested) patch below would make your test\n> problems go away. Or instead, we could simply add sigpipe=ok to the\n> test_must_fail invocation, but I agree with you that the current\n> behavior on OS X is not ideal (the user sees no error message).\n\nWhoops, that patch of course misses adding \"sigchain_push(SIGPIPE,\nSIG_IGN)\" during the fetch-pack operation. I'll leave that as an\nexercise to the reader. ;)\n\n-Peff\n"},{"id":"368782","messageId":"nycvar.QRO.7.76.6.1902080928500.41@tvgsbejvaqbjf.bet","threadId":"49045","inReplyTo":"CAM0VKjkM-THZALy20VrZ-JSMyZjUXUqp1CAoCPrezXRsBfRJ2A@mail.gmail.com","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-08T08:32:32Z","receivedAt":"2019-02-08T08:32:47Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Team,\n\nOn Mon, 6 Aug 2018, SZEDER Gábor wrote:\n\n> [Resending with Clemens' last used email address.\n> Clemens, please consider sending a patch to update our .mailmap file.]\n> \n> \n> On Mon, Aug 6, 2018 at 5:11 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> >\n> > Travis CI changed its default OSX image to use XCode 9.4 on 2018-07-31\n> > [1].  Since then OSX build jobs fail rather frequently because of a\n> > SIGPIPE in the tests 'fetch notices corrupt pack' or 'fetch notices\n> > corrupt idx' in 't5570-git-daemon.sh' [2].  I think this is a symptom\n> > a real bug in Git affecting other platforms as well, but these tests\n> > are too lax to catch it.\n\nI am seeing this very frequently now, as it feels like failing in the\nAzure Pipeline about half of the time.\n\nThe symptom is this:\n\n-- snip --\n++ cp -R '/Users/vsts/agent/2.146.0/work/1/s/t/trash directory.t5570-git-daemon/repo/repo_pack.git' '/Users/vsts/agent/2.146.0/work/1/s/t/trash directory.t5570-git-daemon/repo/repo_bad2.git'\n++ cd '/Users/vsts/agent/2.146.0/work/1/s/t/trash directory.t5570-git-daemon/repo/repo_bad2.git'\n+++ ls objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n++ p=objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n++ chmod u+w objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n++ printf %0256d 0\n++ dd of=objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx bs=256 count=1 seek=1 conv=notrunc\n1+0 records in\n1+0 records out\n256 bytes transferred in 0.000018 secs (14316558 bytes/sec)\n++ mkdir repo_bad2.git\n++ cd repo_bad2.git\n++ git --bare init\nInitialized empty Git repository in /Users/vsts/agent/2.146.0/work/1/s/t/trash directory.t5570-git-daemon/repo_bad2.git/\n++ test_must_fail git --bare fetch git://127.0.0.1:5570/repo_bad2.git\n++ case \"$1\" in\n++ _test_ok=\n++ git --bare fetch git://127.0.0.1:5570/repo_bad2.git\n[13879] Connection from 127.0.0.1:60504\n[13879] Extended attribute \"host\": 127.0.0.1:5570\n[13879] Extended attribute \"protocol\": version=0\n[13879] Request upload-pack for '/repo_bad2.git'\n[13879] error: non-monotonic index ./objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n[13879] error: non-monotonic index ./objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n[13879] error: non-monotonic index ./objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n[13879] error: non-monotonic index ./objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n[13879] error: refs/heads/master does not point to a valid object!\n[13879] error: non-monotonic index ./objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n[13879] error: non-monotonic index ./objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n[13879] error: refs/heads/other does not point to a valid object!\n[13879] error: non-monotonic index ./objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n[13879] error: non-monotonic index ./objects/pack/pack-b2175ab283c0abe358895777ca01c36e88a92e35.idx\n[13879] fatal: git upload-pack: not our ref b5c2e4226db03597a64815fd226648510b22ba40\n[11833] [13879] Disconnected (with error)\n++ exit_code=141\n++ test 141 -eq 0\n++ test_match_signal 13 141\n++ test 141 = 141\n++ return 0\n++ list_contains '' sigpipe\n++ case \",$1,\" in\n++ return 1\n++ test 141 -gt 129\n++ test 141 -le 192\n++ echo 'test_must_fail: died by signal 13: git --bare fetch git://127.0.0.1:5570/repo_bad2.git'\ntest_must_fail: died by signal 13: git --bare fetch git://127.0.0.1:5570/repo_bad2.git\n++ return 1\nerror: last command exited with $?=1\nnot ok 10 - fetch notices corrupt idx\n#\t\n#\t\tcp -R \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n#\t\t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n#\t\t p=$(ls objects/pack/pack-*.idx) &&\n#\t\t chmod u+w $p &&\n#\t\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n#\t\t) &&\n#\t\tmkdir repo_bad2.git &&\n#\t\t(cd repo_bad2.git &&\n#\t\t git --bare init &&\n#\t\t test_must_fail git --bare fetch \"$GIT_DAEMON_URL/repo_bad2.git\" &&\n#\t\t test 0 = $(ls objects/pack | wc -l)\n#\t\t)\n#\t\n-- snap --\n\nAny ideas how to fix this test, anyone?\n\nCiao,\nDscho\n\n> >\n> > What it boils down to is this sequence:\n> >\n> >   - The test first prepares a repository containing a corrupt pack,\n> >     ready to be server via 'git daemon'.\n> >\n> >   - Then the test runs 'test_must_fail git fetch ....', which connects\n> >     to 'git daemon', which forks 'git upload-pack', which then\n> >     advertises refs (only HEAD) and capabilities.  So far so good.\n> >\n> >   - 'git fetch' eventually calls fetch-pack.c:find_common().  The\n> >     first half of this function assembles a request consisting of a\n> >     want and a flush pkt-line, and sends it via a send_request() call.\n> >\n> >     At this point the scheduling becomes important: let's suppose that\n> >     fetch is slow and upload-pack is fast.\n> >\n> >   - 'git upload-pack' receives the request, parses the want line,\n> >     notices the corrupt pack, responds with an 'ERR upload-pack: not\n> >     our ref' pkt-line, and die()s right away.\n> >\n> >   - 'git fetch' finally approaches the end of the function, where it\n> >     attempts to send a done pkt-line via another send_request() call\n> >     through the now closing TCP socket.\n> >\n> >   - What happens now seems to depend on the platform:\n> >\n> >     - On Linux, both on my machine and on Travis CI, it shows textbook\n> >       example behaviour: write() returns with error and sets errno to\n> >       ECONNRESET.  Since it happens in write_or_die(), 'git fetch'\n> >       die()s with 'fatal: write error: Connection reset by peer', and\n> >       doesn't show the error send by 'git upload-pack'; how could it,\n> >       it doesn't even get as far to receive upload-pack's ERR\n> >       pkt-line.\n> >\n> >       The test only checks that 'git fetch' fails, but it doesn't\n> >       check whether it failed with the right error message, so the\n> >       test still succeeds.  Had it checked the error message as well,\n> >       we most likely had noticed this issue already, it doesn't happen\n> >       all that rarely.\n> >\n> >     - On the new OSX images with XCode 9.4 on Travis CI the write()\n> >       triggers SIGPIPE right away, and 'test_must_fail' notices it and\n> >       fails the test.  I couldn't see any sign of an ECONNRESET or any\n> >       other error that we could act upon to avoid the SIGPIPE.\n> >\n> >     - On OSX with XCode 9.2 on Travis CI there is neither SIGPIPE, nor\n> >       ECONNRESET, but sending the request actually succeeds even\n> >       though there is no process on the other end of the socket\n> >       anymore.  'git fetch' then simply continues execution, reads and\n> >       parses the ERR pkt-line, and then dies()s with 'fatal: remote\n> >       error: upload-pack: not our ref'.  So, on the face of it, it\n> >       shows the desired behaviour, but I have no idea how that write()\n> >       could succeed instead of returning error.\n> >\n> > I don't know what happens on a real Mac as I don't have access to one;\n> > I figured out all the above by enabling packet tracing, adding a\n> > couple of well placed tracing printf() and sleep() calls, running a\n> > bunch of builds on Travis CI, and looking through their logs.  But\n> > without access to a debugger and netstat and what not I can't really\n> > go any further.  So I would now happily pass the baton to those who\n> > have a Mac and know a thing or two about its porting issues to first\n> > check whether OSX on a real Mac shows the same behaviour as it does in\n> > Travis CI's virtualized(?) environment.  And then they can pass the\n> > baton to those who know all the intricacies of the pack protocol and\n> > its implementation to decide what to do with this issue.\n> >\n> > For a mostly reliable reproduction recipe you might want to fetch this\n> > branch:\n> >\n> >   https://github.com/szeder/git t5570-git-daemon-sigpipe\n> >\n> > and then run 'make && cd t && ./t5570-git-daemon.sh -v -x'\n> >\n> >\n> > Have fun! ;)\n> >\n> >\n> > 1 - https://blog.travis-ci.com/2018-07-19-xcode9-4-default-announce\n> >\n> > 2 - On git.git's master:\n> >     https://travis-ci.org/git/git/jobs/411517552#L2717\n> \n> "},{"id":"368785","messageId":"nycvar.QRO.7.76.6.1902080958190.41@tvgsbejvaqbjf.bet","threadId":"49045","inReplyTo":"20180814223246.GA2379@sigill.intra.peff.net","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-08T09:02:49Z","receivedAt":"2019-02-08T09:03:02Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nI just had a look at the patch you provided below (for some reason, my\nprevious search on public-inbox only turned up Gábor's mail to which you\nresponded).\n\nAdmittedly, I do not really understand all aspects of it, but it applies,\nstill, and I kicked off a stress test here:\n\n\thttps://dev.azure.com/git/git/_build/results?buildId=338\n\nIt seems that your patch fixes that t5570 flakiness on macOS, and more\nimportantly, addresses an important issue on macOS.\n\nWill play a bit more with it and keep you posted.\n\nCiao,\nDscho\n\nOn Tue, 14 Aug 2018, Jeff King wrote:\n\n> On Mon, Aug 06, 2018 at 05:11:13PM +0200, SZEDER Gábor wrote:\n> \n> >   - 'git upload-pack' receives the request, parses the want line,\n> >     notices the corrupt pack, responds with an 'ERR upload-pack: not\n> >     our ref' pkt-line, and die()s right away.\n> > \n> >   - 'git fetch' finally approaches the end of the function, where it\n> >     attempts to send a done pkt-line via another send_request() call\n> >     through the now closing TCP socket.\n> > \n> >   - What happens now seems to depend on the platform:\n> > \n> >     - On Linux, both on my machine and on Travis CI, it shows textbook\n> >       example behaviour: write() returns with error and sets errno to\n> >       ECONNRESET.  Since it happens in write_or_die(), 'git fetch'\n> >       die()s with 'fatal: write error: Connection reset by peer', and\n> >       doesn't show the error send by 'git upload-pack'; how could it,\n> >       it doesn't even get as far to receive upload-pack's ERR\n> >       pkt-line.\n> > \n> >       The test only checks that 'git fetch' fails, but it doesn't\n> >       check whether it failed with the right error message, so the\n> >       test still succeeds.  Had it checked the error message as well,\n> >       we most likely had noticed this issue already, it doesn't happen\n> >       all that rarely.\n> \n> Hmm. Traditionally we did not send ERR as part of upload-pack at all. It\n> was the message you got from git-daemon if it couldn't start the\n> requested sub-process. It was only later in bdb31eada7 (upload-pack:\n> report \"not our ref\" to client, 2017-02-23) that we started sending\n> them. So I think that is why it does not check the error message: it is\n> not expecting that case at all (and it is not actually interesting here,\n> as the real problem is that the remote side is corrupt, but it sadly\n> does not say anything so useful).\n> \n> I think that's somewhat tangential, though. The root of the issue is\n> this:\n> \n> >     - On the new OSX images with XCode 9.4 on Travis CI the write()\n> >       triggers SIGPIPE right away, and 'test_must_fail' notices it and\n> >       fails the test.  I couldn't see any sign of an ECONNRESET or any\n> >       other error that we could act upon to avoid the SIGPIPE.\n> \n> Right, as soon as we get SIGPIPE we can't offer any useful message,\n> because we're dead. I would argue that fetch should simply turn off\n> SIGPIPE entirely, and rely on getting EPIPE from write(). But since\n> we're in write_or_die(), it actually turns EPIPE back into a SIGPIPE\n> death!\n> \n> So we'd probably also want to teach it to use a real write_in_full(),\n> and then output a more useful message in this case. write_or_die()\n> really does produce bad messages regardless, because it doesn't know\n> what it's writing to.\n> \n> That would give us a baby step in the right direction, because at least\n> we'd always be doing a controlled die() then. And then the next step\n> would be to show the remote error message (even though it's not actually\n> useful in this case, in theory upload-pack could generate something\n> better). And that would mean turning the die() on write into an attempt\n> to drain any ERR messages before either dying or returning an error up\n> the stack.\n> \n> I suspect the (largely untested) patch below would make your test\n> problems go away. Or instead, we could simply add sigpipe=ok to the\n> test_must_fail invocation, but I agree with you that the current\n> behavior on OS X is not ideal (the user sees no error message).\n> \n> -Peff\n> \n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 5714bcbddd..3e80604562 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -188,8 +188,10 @@ static void send_request(struct fetch_pack_args *args,\n>  \tif (args->stateless_rpc) {\n>  \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n>  \t\tpacket_flush(fd);\n> -\t} else\n> -\t\twrite_or_die(fd, buf->buf, buf->len);\n> +\t} else {\n> +\t\tif (write_in_full(fd, buf->buf, buf->len) < 0)\n> +\t\t\tdie_errno(\"unable to write to remote\");\n> +\t}\n>  }\n>  \n>  static void insert_one_alternate_object(struct fetch_negotiator *negotiator,\n> @@ -1167,7 +1169,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n>  \n>  \t/* Send request */\n>  \tpacket_buf_flush(&req_buf);\n> -\twrite_or_die(fd_out, req_buf.buf, req_buf.len);\n> +\tif (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0)\n> +\t\tdie_errno(\"unable to write request to remote\");\n>  \n>  \tstrbuf_release(&req_buf);\n>  \treturn ret;\n> diff --git a/pkt-line.c b/pkt-line.c\n> index a593c08aad..450d0801b1 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -88,13 +88,15 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n>  void packet_flush(int fd)\n>  {\n>  \tpacket_trace(\"0000\", 4, 1);\n> -\twrite_or_die(fd, \"0000\", 4);\n> +\tif (write_in_full(fd, \"0000\", 4) < 0)\n> +\t\tdie_errno(\"unable to write flush packet\");\n>  }\n>  \n>  void packet_delim(int fd)\n>  {\n>  \tpacket_trace(\"0001\", 4, 1);\n> -\twrite_or_die(fd, \"0001\", 4);\n> +\tif (write_in_full(fd, \"0000\", 4) < 0)\n> +\t\tdie_errno(\"unable to write delim packet\");\n>  }\n>  \n>  int packet_flush_gently(int fd)\n> \n> "},{"id":"368808","messageId":"nycvar.QRO.7.76.6.1902081024550.41@tvgsbejvaqbjf.bet","threadId":"49045","inReplyTo":"nycvar.QRO.7.76.6.1902080958190.41@tvgsbejvaqbjf.bet","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-08T09:28:12Z","receivedAt":"2019-02-08T09:28:25Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 8 Feb 2019, Johannes Schindelin wrote:\n\n> I just had a look at the patch you provided below (for some reason, my\n> previous search on public-inbox only turned up Gábor's mail to which you\n> responded).\n> \n> Admittedly, I do not really understand all aspects of it, but it applies,\n> still, and I kicked off a stress test here:\n> \n> \thttps://dev.azure.com/git/git/_build/results?buildId=338\n> \n> It seems that your patch fixes that t5570 flakiness on macOS, and more\n> importantly, addresses an important issue on macOS.\n> \n> Will play a bit more with it and keep you posted.\n\nAlas, I was fooled. *Fooled*, I say. Apparently the --stress option makes\nthe script *succeed* when it fails?\n\nI say that because I wanted to make sure that your patch fixes things and\nreverted your change and started another build, which succeeded. So I\nstarted another build, then another build, and they all succeeded. Only\nthen it dawned on me that I had not looked at the *logs*. And they all\nstill report the same issue, even with your patch:\n\nhttps://dev.azure.com/git/git/_build/results?buildId=338&view=logs&jobId=51041795-01c5-57f3-5561-107b6b9e51a6&taskId=fadc714a-a906-5cf2-cc7a-335e443ad2f8&lineStart=1402&lineEnd=1505&colStart=1&colEnd=32\n\n(You will have to scroll all the way down, or press Ctrl+End, to see that\n\"fetch notices corrupt pack\" is failing.)\n\nSo I am afraid that your patch does not fix the issue nor does it work\naround it.\n\nCiao,\nDscho\n\n> On Tue, 14 Aug 2018, Jeff King wrote:\n> \n> > On Mon, Aug 06, 2018 at 05:11:13PM +0200, SZEDER Gábor wrote:\n> > \n> > >   - 'git upload-pack' receives the request, parses the want line,\n> > >     notices the corrupt pack, responds with an 'ERR upload-pack: not\n> > >     our ref' pkt-line, and die()s right away.\n> > > \n> > >   - 'git fetch' finally approaches the end of the function, where it\n> > >     attempts to send a done pkt-line via another send_request() call\n> > >     through the now closing TCP socket.\n> > > \n> > >   - What happens now seems to depend on the platform:\n> > > \n> > >     - On Linux, both on my machine and on Travis CI, it shows textbook\n> > >       example behaviour: write() returns with error and sets errno to\n> > >       ECONNRESET.  Since it happens in write_or_die(), 'git fetch'\n> > >       die()s with 'fatal: write error: Connection reset by peer', and\n> > >       doesn't show the error send by 'git upload-pack'; how could it,\n> > >       it doesn't even get as far to receive upload-pack's ERR\n> > >       pkt-line.\n> > > \n> > >       The test only checks that 'git fetch' fails, but it doesn't\n> > >       check whether it failed with the right error message, so the\n> > >       test still succeeds.  Had it checked the error message as well,\n> > >       we most likely had noticed this issue already, it doesn't happen\n> > >       all that rarely.\n> > \n> > Hmm. Traditionally we did not send ERR as part of upload-pack at all. It\n> > was the message you got from git-daemon if it couldn't start the\n> > requested sub-process. It was only later in bdb31eada7 (upload-pack:\n> > report \"not our ref\" to client, 2017-02-23) that we started sending\n> > them. So I think that is why it does not check the error message: it is\n> > not expecting that case at all (and it is not actually interesting here,\n> > as the real problem is that the remote side is corrupt, but it sadly\n> > does not say anything so useful).\n> > \n> > I think that's somewhat tangential, though. The root of the issue is\n> > this:\n> > \n> > >     - On the new OSX images with XCode 9.4 on Travis CI the write()\n> > >       triggers SIGPIPE right away, and 'test_must_fail' notices it and\n> > >       fails the test.  I couldn't see any sign of an ECONNRESET or any\n> > >       other error that we could act upon to avoid the SIGPIPE.\n> > \n> > Right, as soon as we get SIGPIPE we can't offer any useful message,\n> > because we're dead. I would argue that fetch should simply turn off\n> > SIGPIPE entirely, and rely on getting EPIPE from write(). But since\n> > we're in write_or_die(), it actually turns EPIPE back into a SIGPIPE\n> > death!\n> > \n> > So we'd probably also want to teach it to use a real write_in_full(),\n> > and then output a more useful message in this case. write_or_die()\n> > really does produce bad messages regardless, because it doesn't know\n> > what it's writing to.\n> > \n> > That would give us a baby step in the right direction, because at least\n> > we'd always be doing a controlled die() then. And then the next step\n> > would be to show the remote error message (even though it's not actually\n> > useful in this case, in theory upload-pack could generate something\n> > better). And that would mean turning the die() on write into an attempt\n> > to drain any ERR messages before either dying or returning an error up\n> > the stack.\n> > \n> > I suspect the (largely untested) patch below would make your test\n> > problems go away. Or instead, we could simply add sigpipe=ok to the\n> > test_must_fail invocation, but I agree with you that the current\n> > behavior on OS X is not ideal (the user sees no error message).\n> > \n> > -Peff\n> > \n> > diff --git a/fetch-pack.c b/fetch-pack.c\n> > index 5714bcbddd..3e80604562 100644\n> > --- a/fetch-pack.c\n> > +++ b/fetch-pack.c\n> > @@ -188,8 +188,10 @@ static void send_request(struct fetch_pack_args *args,\n> >  \tif (args->stateless_rpc) {\n> >  \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n> >  \t\tpacket_flush(fd);\n> > -\t} else\n> > -\t\twrite_or_die(fd, buf->buf, buf->len);\n> > +\t} else {\n> > +\t\tif (write_in_full(fd, buf->buf, buf->len) < 0)\n> > +\t\t\tdie_errno(\"unable to write to remote\");\n> > +\t}\n> >  }\n> >  \n> >  static void insert_one_alternate_object(struct fetch_negotiator *negotiator,\n> > @@ -1167,7 +1169,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n> >  \n> >  \t/* Send request */\n> >  \tpacket_buf_flush(&req_buf);\n> > -\twrite_or_die(fd_out, req_buf.buf, req_buf.len);\n> > +\tif (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0)\n> > +\t\tdie_errno(\"unable to write request to remote\");\n> >  \n> >  \tstrbuf_release(&req_buf);\n> >  \treturn ret;\n> > diff --git a/pkt-line.c b/pkt-line.c\n> > index a593c08aad..450d0801b1 100644\n> > --- a/pkt-line.c\n> > +++ b/pkt-line.c\n> > @@ -88,13 +88,15 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n> >  void packet_flush(int fd)\n> >  {\n> >  \tpacket_trace(\"0000\", 4, 1);\n> > -\twrite_or_die(fd, \"0000\", 4);\n> > +\tif (write_in_full(fd, \"0000\", 4) < 0)\n> > +\t\tdie_errno(\"unable to write flush packet\");\n> >  }\n> >  \n> >  void packet_delim(int fd)\n> >  {\n> >  \tpacket_trace(\"0001\", 4, 1);\n> > -\twrite_or_die(fd, \"0001\", 4);\n> > +\tif (write_in_full(fd, \"0000\", 4) < 0)\n> > +\t\tdie_errno(\"unable to write delim packet\");\n> >  }\n> >  \n> >  int packet_flush_gently(int fd)\n> > \n> > "},{"id":"368834","messageId":"20190208125435.GW10587@szeder.dev","threadId":"49045","inReplyTo":"nycvar.QRO.7.76.6.1902080928500.41@tvgsbejvaqbjf.bet","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-08T12:54:35Z","receivedAt":"2019-02-08T12:54:42Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Feb 08, 2019 at 09:32:32AM +0100, Johannes Schindelin wrote:\n> Team,\n> \n> On Mon, 6 Aug 2018, SZEDER Gábor wrote:\n> \n> > [Resending with Clemens' last used email address.\n> > Clemens, please consider sending a patch to update our .mailmap file.]\n> > \n> > \n> > On Mon, Aug 6, 2018 at 5:11 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> > >\n> > > Travis CI changed its default OSX image to use XCode 9.4 on 2018-07-31\n> > > [1].  Since then OSX build jobs fail rather frequently because of a\n> > > SIGPIPE in the tests 'fetch notices corrupt pack' or 'fetch notices\n> > > corrupt idx' in 't5570-git-daemon.sh' [2].  I think this is a symptom\n> > > a real bug in Git affecting other platforms as well, but these tests\n> > > are too lax to catch it.\n> \n> I am seeing this very frequently now, as it feels like failing in the\n> Azure Pipeline about half of the time.\n\nI was wondering whether it's only an issue on Travis CI, and was\nwaiting whether you'll complain about it :)  Evidently it is not, but\nI still would like to see a report about macOS running directly on\nsomeone's Mac hardware, i.e. without all the CI/cloud/whatnot magic.\n\n> Any ideas how to fix this test, anyone?\n\nI'm afraid that this is not merely an issue with the test, but a\nplatform issue that we should work around somehow.  I would have\nsuggested to follow up on what Peff suggested, but I see that you\nalready did, and it didn't work out...\n\nIn the meantime, for lack of a better option, I started to skip the\ntwo failure-prone tests in the OSX build jobs in my automated CI\nbuilds with:\n\n -- >8 --\n\nSubject: [PATCH] travis-ci: skip flaky tests in 't5570-git-daemon.sh' in OSX\n build jobs\n\nSee: https://public-inbox.org/git/CAM0VKj=MCS+cmOgzf_XyPeb+qZrFmuMH52-PV_NDMZA9X+rRoA@mail.gmail.com/\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n ci/lib-travisci.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh\nindex 69dff4d1ec..1cd2c1db7d 100755\n--- a/ci/lib-travisci.sh\n+++ b/ci/lib-travisci.sh\n@@ -121,7 +121,9 @@ osx-clang|osx-gcc)\n \t# t9810 occasionally fails on Travis CI OS X\n \t# t9816 occasionally fails with \"TAP out of sequence errors\" on\n \t# Travis CI OS X\n-\texport GIT_SKIP_TESTS=\"t9810 t9816\"\n+\t# In 't5570-git-daemon.sh', the tests 'fetch notices corrupt pack'\n+\t# and 'fetch notices corrupt idx' fail rather frequently.\n+\texport GIT_SKIP_TESTS=\"t9810 t9816 t5570.9 t5570.10\"\n \t;;\n GIT_TEST_GETTEXT_POISON)\n \texport GIT_TEST_GETTEXT_POISON=YesPlease\n-- \n2.20.1.940.g8404bb2d1a\n\n -- >8 --\n\n\n> > > What it boils down to is this sequence:\n> > >\n> > >   - The test first prepares a repository containing a corrupt pack,\n> > >     ready to be server via 'git daemon'.\n> > >\n> > >   - Then the test runs 'test_must_fail git fetch ....', which connects\n> > >     to 'git daemon', which forks 'git upload-pack', which then\n> > >     advertises refs (only HEAD) and capabilities.  So far so good.\n> > >\n> > >   - 'git fetch' eventually calls fetch-pack.c:find_common().  The\n> > >     first half of this function assembles a request consisting of a\n> > >     want and a flush pkt-line, and sends it via a send_request() call.\n> > >\n> > >     At this point the scheduling becomes important: let's suppose that\n> > >     fetch is slow and upload-pack is fast.\n> > >\n> > >   - 'git upload-pack' receives the request, parses the want line,\n> > >     notices the corrupt pack, responds with an 'ERR upload-pack: not\n> > >     our ref' pkt-line, and die()s right away.\n> > >\n> > >   - 'git fetch' finally approaches the end of the function, where it\n> > >     attempts to send a done pkt-line via another send_request() call\n> > >     through the now closing TCP socket.\n> > >\n> > >   - What happens now seems to depend on the platform:\n> > >\n> > >     - On Linux, both on my machine and on Travis CI, it shows textbook\n> > >       example behaviour: write() returns with error and sets errno to\n> > >       ECONNRESET.  Since it happens in write_or_die(), 'git fetch'\n> > >       die()s with 'fatal: write error: Connection reset by peer', and\n> > >       doesn't show the error send by 'git upload-pack'; how could it,\n> > >       it doesn't even get as far to receive upload-pack's ERR\n> > >       pkt-line.\n> > >\n> > >       The test only checks that 'git fetch' fails, but it doesn't\n> > >       check whether it failed with the right error message, so the\n> > >       test still succeeds.  Had it checked the error message as well,\n> > >       we most likely had noticed this issue already, it doesn't happen\n> > >       all that rarely.\n> > >\n> > >     - On the new OSX images with XCode 9.4 on Travis CI the write()\n> > >       triggers SIGPIPE right away, and 'test_must_fail' notices it and\n> > >       fails the test.  I couldn't see any sign of an ECONNRESET or any\n> > >       other error that we could act upon to avoid the SIGPIPE.\n> > >\n> > >     - On OSX with XCode 9.2 on Travis CI there is neither SIGPIPE, nor\n> > >       ECONNRESET, but sending the request actually succeeds even\n> > >       though there is no process on the other end of the socket\n> > >       anymore.  'git fetch' then simply continues execution, reads and\n> > >       parses the ERR pkt-line, and then dies()s with 'fatal: remote\n> > >       error: upload-pack: not our ref'.  So, on the face of it, it\n> > >       shows the desired behaviour, but I have no idea how that write()\n> > >       could succeed instead of returning error.\n> > >\n> > > I don't know what happens on a real Mac as I don't have access to one;\n> > > I figured out all the above by enabling packet tracing, adding a\n> > > couple of well placed tracing printf() and sleep() calls, running a\n> > > bunch of builds on Travis CI, and looking through their logs.  But\n> > > without access to a debugger and netstat and what not I can't really\n> > > go any further.  So I would now happily pass the baton to those who\n> > > have a Mac and know a thing or two about its porting issues to first\n> > > check whether OSX on a real Mac shows the same behaviour as it does in\n> > > Travis CI's virtualized(?) environment.  And then they can pass the\n> > > baton to those who know all the intricacies of the pack protocol and\n> > > its implementation to decide what to do with this issue.\n> > >\n> > > For a mostly reliable reproduction recipe you might want to fetch this\n> > > branch:\n> > >\n> > >   https://github.com/szeder/git t5570-git-daemon-sigpipe\n> > >\n> > > and then run 'make && cd t && ./t5570-git-daemon.sh -v -x'\n> > >\n> > >\n> > > Have fun! ;)\n> > >\n> > >\n> > > 1 - https://blog.travis-ci.com/2018-07-19-xcode9-4-default-announce\n> > >\n> > > 2 - On git.git's master:\n> > >     https://travis-ci.org/git/git/jobs/411517552#L2717\n> > \n> > \n\n"},{"id":"368871","messageId":"20190208195459.GA32556@sigill.intra.peff.net","threadId":"49045","inReplyTo":"nycvar.QRO.7.76.6.1902081024550.41@tvgsbejvaqbjf.bet","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-08T19:54:59Z","receivedAt":"2019-02-08T19:55:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 08, 2019 at 10:28:12AM +0100, Johannes Schindelin wrote:\n\n> Hi Peff,\n> \n> On Fri, 8 Feb 2019, Johannes Schindelin wrote:\n> \n> > I just had a look at the patch you provided below (for some reason, my\n> > previous search on public-inbox only turned up Gábor's mail to which you\n> > responded).\n> > \n> > Admittedly, I do not really understand all aspects of it, but it applies,\n> > still, and I kicked off a stress test here:\n> > \n> > \thttps://dev.azure.com/git/git/_build/results?buildId=338\n> > \n> > It seems that your patch fixes that t5570 flakiness on macOS, and more\n> > importantly, addresses an important issue on macOS.\n> > \n> > Will play a bit more with it and keep you posted.\n> \n> Alas, I was fooled. *Fooled*, I say. Apparently the --stress option makes\n> the script *succeed* when it fails?\n> [...]\n> So I am afraid that your patch does not fix the issue nor does it work\n> around it.\n\nI think that patch does the write_or_die conversion to handle EPIPE, but\nit would still need to turn off SIGPIPE for the whole process.\n\nSo you'd also need to stick a:\n\n  sigchain_push(SIGPIPE, SIG_IGN);\n\nsomewhere near the start of cmd_fetch(). (There may be a less\ncoarse-grained place to put it, but at this point I think we're just\ntrying to find out whether this approach even solves the problem).\n\n-Peff\n"},{"id":"370420","messageId":"nycvar.QRO.7.76.6.1903011557430.41@tvgsbejvaqbjf.bet","threadId":"49045","inReplyTo":"20190208195459.GA32556@sigill.intra.peff.net","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-01T15:02:22Z","receivedAt":"2019-03-01T15:02:50Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 8 Feb 2019, Jeff King wrote:\n\n> On Fri, Feb 08, 2019 at 10:28:12AM +0100, Johannes Schindelin wrote:\n> \n> > On Fri, 8 Feb 2019, Johannes Schindelin wrote:\n> > \n> > > I just had a look at the patch you provided below (for some reason, my\n> > > previous search on public-inbox only turned up Gábor's mail to which you\n> > > responded).\n> > > \n> > > Admittedly, I do not really understand all aspects of it, but it applies,\n> > > still, and I kicked off a stress test here:\n> > > \n> > > \thttps://dev.azure.com/git/git/_build/results?buildId=338\n> > > \n> > > It seems that your patch fixes that t5570 flakiness on macOS, and more\n> > > importantly, addresses an important issue on macOS.\n> > > \n> > > Will play a bit more with it and keep you posted.\n> > \n> > Alas, I was fooled. *Fooled*, I say. Apparently the --stress option makes\n> > the script *succeed* when it fails?\n> > [...]\n> > So I am afraid that your patch does not fix the issue nor does it work\n> > around it.\n> \n> I think that patch does the write_or_die conversion to handle EPIPE, but\n> it would still need to turn off SIGPIPE for the whole process.\n> \n> So you'd also need to stick a:\n> \n>   sigchain_push(SIGPIPE, SIG_IGN);\n> \n> somewhere near the start of cmd_fetch(). (There may be a less\n> coarse-grained place to put it, but at this point I think we're just\n> trying to find out whether this approach even solves the problem).\n\nThis fixes it, it seems. I let the job run with `--stress=50` and even\nafter half an hour, it did not fail:\n\nattempts ://git.visualstudio.com/git/_build/results?buildId=354\n\n(I had to cancel it, I thought that `--stress=50` would stop trying after\n50 runs, but I was obviously incorrect in that assumption...)\n\nReverting the patch made it fail within a hundred runs:\n\nhttps://git.visualstudio.com/git/_build/results?buildId=356\n\nSo. Good, we have a diff that works. But you mentioned that you'd like to\nput it somewhere else? I am a bit unfamiliar with the code paths of\n`cmd_fetch()`, so I would be hard pressed to find a better spot. Any hint?\n\nCiao,\nDscho"},{"id":"370442","messageId":"20190301190042.GF30847@sigill.intra.peff.net","threadId":"49045","inReplyTo":"nycvar.QRO.7.76.6.1903011557430.41@tvgsbejvaqbjf.bet","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-01T19:00:42Z","receivedAt":"2019-03-01T19:00:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2019 at 04:02:22PM +0100, Johannes Schindelin wrote:\n\n> > I think that patch does the write_or_die conversion to handle EPIPE, but\n> > it would still need to turn off SIGPIPE for the whole process.\n> > \n> > So you'd also need to stick a:\n> > \n> >   sigchain_push(SIGPIPE, SIG_IGN);\n> > \n> > somewhere near the start of cmd_fetch(). (There may be a less\n> > coarse-grained place to put it, but at this point I think we're just\n> > trying to find out whether this approach even solves the problem).\n> \n> This fixes it, it seems. I let the job run with `--stress=50` and even\n> after half an hour, it did not fail:\n> \n> attempts ://git.visualstudio.com/git/_build/results?buildId=354\n\nCool, I'm glad it works!\n\n> (I had to cancel it, I thought that `--stress=50` would stop trying after\n> 50 runs, but I was obviously incorrect in that assumption...)\n\nNo, that will do 50 simultaneous jobs. You want --stress-limit=50.\n\nIt might make more sense to have --stress-jobs, and make --stress=X set\nthe limit, as I think it's much more useful to set the limit than it is\nto set the number of jobs. At least that's my experience.\n\n> So. Good, we have a diff that works. But you mentioned that you'd like to\n> put it somewhere else? I am a bit unfamiliar with the code paths of\n> `cmd_fetch()`, so I would be hard pressed to find a better spot. Any hint?\n\nWell, I'm of two minds there. If we want to make the minimum change,\nthen we'd just want to disable SIGPIPE while we're actually conversing\nwith the other side. So I guess that would be somewhere in do_fetch(),\nbefore we start talking to the other side, and restoring it after we've\nfinished our half of the conversation (i.e., after we've done all the\nwant/have bits and we're receiving the pack). But that's actually pretty\nawkward to do, because most of those details are handled under the hood\nby the transport code.\n\nSo probably something like this is the least-terrible place to put it:\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b620fd54b4..4ba63d5ac6 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1556,7 +1556,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru\n \n \tsigchain_push_common(unlock_pack_on_signal);\n \tatexit(unlock_pack);\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n \texit_code = do_fetch(gtransport, &rs);\n+\tsigchain_pop(SIGPIPE);\n \trefspec_clear(&rs);\n \ttransport_disconnect(gtransport);\n \tgtransport = NULL;\n\nThat said, I actually think it's kind of pointless for git-fetch to use\nSIGPIPE at all. The purpose of SIGPIPE is that you can write a naive\nprogram that spews output, and you'll get informed (forcefully) by the\nOS if the process consuming your output stops listening. That makes\nsense for programs like \"git log\", whose primary purpose is generating\noutput.\n\nBut for git-fetch, our primary purpose is receiving data and writing it\nto disk. We should be checking all of our write()s already, and SIGPIPE\nis just confusing. The only \"big\" output we generate is the status table\nat the end. And even if that is going to a pipe that closes, I don't\nthink we'd want to fail the whole command (we'd want to finalize any\nwrites for what we just fetched, clean up after ourselves, etc).\n\nSo I'd actually be fine with just declaring that certain commands (like\nfetch) just ignore SIGPIPE entirely.\n\n-Peff\n"},{"id":"370477","messageId":"nycvar.QRO.7.76.6.1903022209110.45@tvgsbejvaqbjf.bet","threadId":"49045","inReplyTo":"20190301190042.GF30847@sigill.intra.peff.net","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-02T21:21:00Z","receivedAt":"2019-03-02T21:21:05Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 1 Mar 2019, Jeff King wrote:\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b620fd54b4..4ba63d5ac6 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1556,7 +1556,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru\n>  \n>  \tsigchain_push_common(unlock_pack_on_signal);\n>  \tatexit(unlock_pack);\n> +\tsigchain_push(SIGPIPE, SIG_IGN);\n>  \texit_code = do_fetch(gtransport, &rs);\n> +\tsigchain_pop(SIGPIPE);\n>  \trefspec_clear(&rs);\n>  \ttransport_disconnect(gtransport);\n>  \tgtransport = NULL;\n\nThat indeed does the job:\n\n\thttps://git.visualstudio.com/git/_build/results?buildId=358\n\n> That said, I actually think it's kind of pointless for git-fetch to use\n> SIGPIPE at all. The purpose of SIGPIPE is that you can write a naive\n> program that spews output, and you'll get informed (forcefully) by the\n> OS if the process consuming your output stops listening. That makes\n> sense for programs like \"git log\", whose primary purpose is generating\n> output.\n> \n> But for git-fetch, our primary purpose is receiving data and writing it\n> to disk. We should be checking all of our write()s already, and SIGPIPE\n> is just confusing. The only \"big\" output we generate is the status table\n> at the end. And even if that is going to a pipe that closes, I don't\n> think we'd want to fail the whole command (we'd want to finalize any\n> writes for what we just fetched, clean up after ourselves, etc).\n> \n> So I'd actually be fine with just declaring that certain commands (like\n> fetch) just ignore SIGPIPE entirely.\n\nThat's a bigger change than I'm comfortable with, so I'd like to go with\ntge diff you gave above.\n\nDo you want to turn these two patches into a proper patch series?\nOtherwise I can take care of it, probably this Monday or Tuesday.\n\nCiao,\nDscho\n"},{"id":"370485","messageId":"xmqq8sxw69d5.fsf@gitster-ct.c.googlers.com","threadId":"49045","inReplyTo":"20190301190042.GF30847@sigill.intra.peff.net","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-03T01:21:26Z","receivedAt":"2019-03-03T01:21:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But for git-fetch, our primary purpose is receiving data and writing it\n> to disk. We should be checking all of our write()s already, and SIGPIPE\n> is just confusing.\n\nYup, sounds like a very sensible argument.\n\n> So I'd actually be fine with just declaring that certain commands (like\n> fetch) just ignore SIGPIPE entirely.\n\nMe too (to me, \"actually be fine with\" would mean \"that's a larger\nhammer alternative, which is OK, while the base solution is fine,\ntoo\").\n\nThanks.\n\n\n"},{"id":"370527","messageId":"nycvar.QRO.7.76.6.1903031551540.45@tvgsbejvaqbjf.bet","threadId":"49045","inReplyTo":"xmqq8sxw69d5.fsf@gitster-ct.c.googlers.com","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-03T14:56:32Z","receivedAt":"2019-03-03T14:56:45Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 3 Mar 2019, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > But for git-fetch, our primary purpose is receiving data and writing it\n> > to disk. We should be checking all of our write()s already, and SIGPIPE\n> > is just confusing.\n> \n> Yup, sounds like a very sensible argument.\n> \n> > So I'd actually be fine with just declaring that certain commands (like\n> > fetch) just ignore SIGPIPE entirely.\n> \n> Me too (to me, \"actually be fine with\" would mean \"that's a larger\n> hammer alternative, which is OK, while the base solution is fine,\n> too\").\n\nSo do I understand that you'd like something like this?\n\n-- snipsnap --\nt a/git.c b/git.c\nindex 2f604a41ea..c4122cc699 100644\n--- a/git.c\n+++ b/git.c\n@@ -4,6 +4,7 @@\n #include \"help.h\"\n #include \"run-command.h\"\n #include \"alias.h\"\n+#include \"sigchain.h\"\n \n #define RUN_SETUP\t\t(1<<0)\n #define RUN_SETUP_GENTLY\t(1<<1)\n@@ -16,6 +17,7 @@\n #define SUPPORT_SUPER_PREFIX\t(1<<4)\n #define DELAY_PAGER_CONFIG\t(1<<5)\n #define NO_PARSEOPT\t\t(1<<6) /* parse-options is not used */\n+#define IGNORE_SIGPIPE\t\t(1<<7)\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -388,6 +390,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \tprefix = NULL;\n \thelp = argc == 2 && !strcmp(argv[1], \"-h\");\n \tif (!help) {\n+\t\tif (p->option & IGNORE_SIGPIPE)\n+\t\t\tsigchain_push(SIGPIPE, SIG_IGN);\n \t\tif (p->option & RUN_SETUP)\n \t\t\tprefix = setup_git_directory();\n \t\telse if (p->option & RUN_SETUP_GENTLY) {\n@@ -477,7 +481,7 @@ static struct cmd_struct commands[] = {\n \t{ \"diff-tree\", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },\n \t{ \"difftool\", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"fast-export\", cmd_fast_export, RUN_SETUP },\n-\t{ \"fetch\", cmd_fetch, RUN_SETUP },\n+\t{ \"fetch\", cmd_fetch, RUN_SETUP | IGNORE_SIGPIPE },\n \t{ \"fetch-pack\", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },\n \t{ \"fmt-merge-msg\", cmd_fmt_merge_msg, RUN_SETUP },\n \t{ \"for-each-ref\", cmd_for_each_ref, RUN_SETUP },\n\n"},{"id":"370535","messageId":"20190303165447.GA31116@sigill.intra.peff.net","threadId":"49045","inReplyTo":"nycvar.QRO.7.76.6.1903022209110.45@tvgsbejvaqbjf.bet","subject":"Re: t5570-git-daemon fails with SIGPIPE on OSX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-03T16:54:47Z","receivedAt":"2019-03-03T16:54:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 02, 2019 at 10:21:00PM +0100, Johannes Schindelin wrote:\n\n> Do you want to turn these two patches into a proper patch series?\n> Otherwise I can take care of it, probably this Monday or Tuesday.\n\nHere they are. The old email sending the first one actually had a typo\n(it sent \"0000\" instead of \"0001\" for some packets; oops), which is\nfixed here.\n\nI'm actually not sure of the placement for the second one; see the\ncomments there.\n\n  [1/2]: fetch: avoid calling write_or_die()\n  [2/2]: fetch: ignore SIGPIPE during network operation\n\n builtin/fetch.c | 2 ++\n fetch-pack.c    | 9 ++++++---\n pkt-line.c      | 6 ++++--\n 3 files changed, 12 insertions(+), 5 deletions(-)\n\n-Peff\n"},{"id":"370536","messageId":"20190303165537.GA23755@sigill.intra.peff.net","threadId":"49045","inReplyTo":"20190303165447.GA31116@sigill.intra.peff.net","subject":"[PATCH 1/2] fetch: avoid calling write_or_die()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-03T16:55:37Z","receivedAt":"2019-03-03T16:55:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The write_or_die() function has one quirk that a caller might not\nexpect: when it sees EPIPE from the write() call, it translates that\ninto a death by SIGPIPE. This doesn't change the overall behavior (the\nprogram exits either way), but it does potentially confuse test scripts\nlooking for a non-signal exit code.\n\nLet's switch away from using write_or_die() in a few code paths, which\nwill give us more consistent exit codes. It also gives us the\nopportunity to write more descriptive error messages, since we have\ncontext that write_or_die() does not.\n\nNote that this won't do much by itself, since we'd typically be killed\nby SIGPIPE before write_or_die() even gets a chance to do its thing.\nThat will be addressed in the next patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fetch-pack.c | 9 ++++++---\n pkt-line.c   | 6 ++++--\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 812be15d7e..dca249e9d7 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -191,8 +191,10 @@ static void send_request(struct fetch_pack_args *args,\n \tif (args->stateless_rpc) {\n \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n \t\tpacket_flush(fd);\n-\t} else\n-\t\twrite_or_die(fd, buf->buf, buf->len);\n+\t} else {\n+\t\tif (write_in_full(fd, buf->buf, buf->len) < 0)\n+\t\t\tdie_errno(\"unable to write to remote\");\n+\t}\n }\n \n static void insert_one_alternate_object(struct fetch_negotiator *negotiator,\n@@ -1163,7 +1165,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \n \t/* Send request */\n \tpacket_buf_flush(&req_buf);\n-\twrite_or_die(fd_out, req_buf.buf, req_buf.len);\n+\tif (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0)\n+\t\tdie_errno(\"unable to write request to remote\");\n \n \tstrbuf_release(&req_buf);\n \treturn ret;\ndiff --git a/pkt-line.c b/pkt-line.c\nindex d4b71d3e82..093b2f3976 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -88,13 +88,15 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n void packet_flush(int fd)\n {\n \tpacket_trace(\"0000\", 4, 1);\n-\twrite_or_die(fd, \"0000\", 4);\n+\tif (write_in_full(fd, \"0000\", 4) < 0)\n+\t\tdie_errno(\"unable to write flush packet\");\n }\n \n void packet_delim(int fd)\n {\n \tpacket_trace(\"0001\", 4, 1);\n-\twrite_or_die(fd, \"0001\", 4);\n+\tif (write_in_full(fd, \"0001\", 4) < 0)\n+\t\tdie_errno(\"unable to write delim packet\");\n }\n \n int packet_flush_gently(int fd)\n-- \n2.21.0.675.g01c085a870\n\n"},{"id":"370537","messageId":"20190303165843.GB23755@sigill.intra.peff.net","threadId":"49045","inReplyTo":"20190303165447.GA31116@sigill.intra.peff.net","subject":"[PATCH 2/2] fetch: ignore SIGPIPE during network operation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-03T16:58:43Z","receivedAt":"2019-03-03T16:58:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The default SIGPIPE behavior can be useful for a command that generates\na lot of output: if the receiver of our output goes away, we'll be\nnotified asynchronously to stop generating it (typically by killing the\nprogram).\n\nBut for a command like fetch, which is primarily concerned with\nreceiving data and writing it to disk, an unexpected SIGPIPE can be\nawkward. We're already checking the return value of all of our write()\ncalls, and dying due to the signal takes away our chance to gracefully\nhandle the error.\n\nOn Linux, we wouldn't generally see SIGPIPE at all during fetch. If the\nother side of the network connection hangs up, we'll see ECONNRESET. But\non OS X, we get a SIGPIPE, and the process is killed. This causes t5570\nto racily fail, as we sometimes die by signal (instead of the expected\ndie() call) when the server side hangs up.\n\nLet's ignore SIGPIPE during the network portion of the fetch, which will\ncause our write() to return EPIPE, giving us consistent behavior across\nplatforms.\n\nThis fixes the test flakiness, but note that it stops short of fixing\nthe larger problem. The server side hit a fatal error, sent us an \"ERR\"\npacket, and then hung up. We notice the failure because we're trying to\nwrite to a closed socket. But by dying immediately, we never actually\nread the ERR packet and report its content to the user. This is a (racy)\nproblem on all platforms. So this patch lays the groundwork from which\nthat problem might be fixed consistently, but it doesn't actually fix\nit.\n\nNote the placement of the SIGPIPE handling. The absolute minimal change\nwould be to ignore SIGPIPE only when we're writing. But twiddling the\nsignal handler for each write call is inefficient and maintenance\nburden. On the opposite end of the spectrum, we could simply declare\nthat fetch does not need SIGPIPE handling, since it doesn't generate a\nlot of output, and we could just ignore it at the start of cmd_fetch().\n\nThis patch takes a middle ground. It ignores SIGPIPE during the network\noperation (which is admittedly most of the program, since the actual\nnetwork operations are all done under the hood by the transport code).\nSo it's still pretty coarse.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI had a hard time trying to write the last two paragraphs. I think the\nplacement here really is not significantly different than just ignoring\nSIGPIPE for the entirety of cmd_fetch(). I kind of like your suggestion\nelsewhere in the thread to just teach the git wrapper an IGNORE_SIGPIPE\nflag.\n\n builtin/fetch.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b620fd54b4..4ba63d5ac6 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1556,7 +1556,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru\n \n \tsigchain_push_common(unlock_pack_on_signal);\n \tatexit(unlock_pack);\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n \texit_code = do_fetch(gtransport, &rs);\n+\tsigchain_pop(SIGPIPE);\n \trefspec_clear(&rs);\n \ttransport_disconnect(gtransport);\n \tgtransport = NULL;\n-- \n2.21.0.675.g01c085a870\n"},{"id":"370566","messageId":"xmqqk1hfzbn1.fsf@gitster-ct.c.googlers.com","threadId":"49045","inReplyTo":"20190303165843.GB23755@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fetch: ignore SIGPIPE during network operation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-04T01:11:46Z","receivedAt":"2019-03-04T01:11:51Z","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> ... But by dying immediately, we never actually\n> read the ERR packet and report its content to the user. This is a (racy)\n> problem on all platforms.\n\nYeah, I do not think of a good solution for it (nor I am not\nconvinced that it is worth fixing, to be honest.  The cable may get\ncut before we have a chance to see the ERR packet, or the other side\nmay have died before producing one.\n\nThe fix obviously looks good.  Thanks.\n"},{"id":"370600","messageId":"CACsJy8BWp9+SDwcXv_a6aFN=YSoQN3E7sOGDRm5a6Pi7QhzexQ@mail.gmail.com","threadId":"49045","inReplyTo":"20190303165537.GA23755@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] fetch: avoid calling write_or_die()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-04T13:42:40Z","receivedAt":"2019-03-04T13:43:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 3, 2019 at 11:55 PM Jeff King <peff@peff.net> wrote:\n>\n> The write_or_die() function has one quirk that a caller might not\n> expect: when it sees EPIPE from the write() call, it translates that\n> into a death by SIGPIPE. This doesn't change the overall behavior (the\n> program exits either way), but it does potentially confuse test scripts\n> looking for a non-signal exit code.\n>\n> Let's switch away from using write_or_die() in a few code paths, which\n> will give us more consistent exit codes. It also gives us the\n> opportunity to write more descriptive error messages, since we have\n> context that write_or_die() does not.\n>\n> Note that this won't do much by itself, since we'd typically be killed\n> by SIGPIPE before write_or_die() even gets a chance to do its thing.\n> That will be addressed in the next patch.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  fetch-pack.c | 9 ++++++---\n>  pkt-line.c   | 6 ++++--\n>  2 files changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 812be15d7e..dca249e9d7 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -191,8 +191,10 @@ static void send_request(struct fetch_pack_args *args,\n>         if (args->stateless_rpc) {\n>                 send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n>                 packet_flush(fd);\n> -       } else\n> -               write_or_die(fd, buf->buf, buf->len);\n> +       } else {\n> +               if (write_in_full(fd, buf->buf, buf->len) < 0)\n> +                       die_errno(\"unable to write to remote\");\n\nmaybe _() these strings.\n\n> +       }\n>  }\n>\n>  static void insert_one_alternate_object(struct fetch_negotiator *negotiator,\n> @@ -1163,7 +1165,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n>\n>         /* Send request */\n>         packet_buf_flush(&req_buf);\n> -       write_or_die(fd_out, req_buf.buf, req_buf.len);\n> +       if (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0)\n> +               die_errno(\"unable to write request to remote\");\n>\n>         strbuf_release(&req_buf);\n>         return ret;\n> diff --git a/pkt-line.c b/pkt-line.c\n> index d4b71d3e82..093b2f3976 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -88,13 +88,15 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n>  void packet_flush(int fd)\n>  {\n>         packet_trace(\"0000\", 4, 1);\n> -       write_or_die(fd, \"0000\", 4);\n> +       if (write_in_full(fd, \"0000\", 4) < 0)\n> +               die_errno(\"unable to write flush packet\");\n>  }\n>\n>  void packet_delim(int fd)\n>  {\n>         packet_trace(\"0001\", 4, 1);\n> -       write_or_die(fd, \"0001\", 4);\n> +       if (write_in_full(fd, \"0001\", 4) < 0)\n> +               die_errno(\"unable to write delim packet\");\n>  }\n>\n>  int packet_flush_gently(int fd)\n> --\n> 2.21.0.675.g01c085a870\n>\n\n\n-- \nDuy\n"},{"id":"370655","messageId":"20190305041139.GA19800@sigill.intra.peff.net","threadId":"49045","inReplyTo":"xmqqk1hfzbn1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] fetch: ignore SIGPIPE during network operation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-05T04:11:39Z","receivedAt":"2019-03-05T04:11:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2019 at 10:11:46AM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > ... But by dying immediately, we never actually\n> > read the ERR packet and report its content to the user. This is a (racy)\n> > problem on all platforms.\n> \n> Yeah, I do not think of a good solution for it (nor I am not\n> convinced that it is worth fixing, to be honest.  The cable may get\n> cut before we have a chance to see the ERR packet, or the other side\n> may have died before producing one.\n\nWe definitely can never cover this 100%. But I wonder if we could put a\nlittle more effort into \"best effort\". Specifically, I was thinking that\non seeing the write error, we might do something like:\n\n  void NORETURN last_ditch_proto_read(const char *msg)\n  {\n\tchar *line;\n\n\t/*\n\t * we had a write error; see if the server sent us anything\n\t * useful to report\n\t */\n\tif (packet_read_line_gently(fd, NULL, &line) &&\n\t    skip_prefix(line, \"ERR \", &line)) {\n\t        die(\"remote error: %s\", line);\n\t}\n\n\t/* otherwise, just report the write error */\n\tdie(\"%s\", msg);\n  }\n\nThe tricky thing is that the writer does not always know the correct fd\nto read more packets from (since the write errors may be deep in generic\ncode). I suspect we could rig up some kind of hacky global \"if this\ndescriptor variable is non-negative, then do a last ditch read from it\".\n\nI do wonder if the race is better or worse when doing local fetches in\nthe test suite. On a real network with actual transit times, I suspect\nwe'd do better, because our writes would still be fast (we dump bytes to\nthe OS buffer) and we'd spend a higher percentage of our time waiting to\nread back from the other side (which is what we want, because then we\nsee the ERR they wrote to us). That's just a guess, though.\n\n> The fix obviously looks good.  Thanks.\n\nYeah, I don't think any of the above discussion needs to block the fix\nhere.\n\nHere's a re-roll of the first patch, though, marked for translation as\nrequested by Duy.\n\n-- >8 --\nSubject: [PATCH] fetch: avoid calling write_or_die()\n\nThe write_or_die() function has one quirk that a caller might not\nexpect: when it sees EPIPE from the write() call, it translates that\ninto a death by SIGPIPE. This doesn't change the overall behavior (the\nprogram exits either way), but it does potentially confuse test scripts\nlooking for a non-signal exit code.\n\nLet's switch away from using write_or_die() in a few code paths, which\nwill give us more consistent exit codes. It also gives us the\nopportunity to write more descriptive error messages, since we have\ncontext that write_or_die() does not.\n\nNote that this won't do much by itself, since we'd typically be killed\nby SIGPIPE before write_or_die() even gets a chance to do its thing.\nThat will be addressed in the next patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fetch-pack.c | 9 ++++++---\n pkt-line.c   | 6 ++++--\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 812be15d7e..e69993b2eb 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -191,8 +191,10 @@ static void send_request(struct fetch_pack_args *args,\n \tif (args->stateless_rpc) {\n \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n \t\tpacket_flush(fd);\n-\t} else\n-\t\twrite_or_die(fd, buf->buf, buf->len);\n+\t} else {\n+\t\tif (write_in_full(fd, buf->buf, buf->len) < 0)\n+\t\t\tdie_errno(_(\"unable to write to remote\"));\n+\t}\n }\n \n static void insert_one_alternate_object(struct fetch_negotiator *negotiator,\n@@ -1163,7 +1165,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \n \t/* Send request */\n \tpacket_buf_flush(&req_buf);\n-\twrite_or_die(fd_out, req_buf.buf, req_buf.len);\n+\tif (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0)\n+\t\tdie_errno(_(\"unable to write request to remote\"));\n \n \tstrbuf_release(&req_buf);\n \treturn ret;\ndiff --git a/pkt-line.c b/pkt-line.c\nindex d4b71d3e82..6bd496a9bb 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -88,13 +88,15 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n void packet_flush(int fd)\n {\n \tpacket_trace(\"0000\", 4, 1);\n-\twrite_or_die(fd, \"0000\", 4);\n+\tif (write_in_full(fd, \"0000\", 4) < 0)\n+\t\tdie_errno(_(\"unable to write flush packet\"));\n }\n \n void packet_delim(int fd)\n {\n \tpacket_trace(\"0001\", 4, 1);\n-\twrite_or_die(fd, \"0001\", 4);\n+\tif (write_in_full(fd, \"0001\", 4) < 0)\n+\t\tdie_errno(_(\"unable to write delim packet\"));\n }\n \n int packet_flush_gently(int fd)\n-- \n2.21.0.684.gc9dc8b89c9\n\n"},{"id":"370656","messageId":"20190305041158.GB19800@sigill.intra.peff.net","threadId":"49045","inReplyTo":"CACsJy8BWp9+SDwcXv_a6aFN=YSoQN3E7sOGDRm5a6Pi7QhzexQ@mail.gmail.com","subject":"Re: [PATCH 1/2] fetch: avoid calling write_or_die()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-05T04:11:58Z","receivedAt":"2019-03-05T04:12:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2019 at 08:42:40PM +0700, Duy Nguyen wrote:\n\n> > -       } else\n> > -               write_or_die(fd, buf->buf, buf->len);\n> > +       } else {\n> > +               if (write_in_full(fd, buf->buf, buf->len) < 0)\n> > +                       die_errno(\"unable to write to remote\");\n> \n> maybe _() these strings.\n\nGood thinking. I just sent an update in another part of the thread.\n\n-Peff\n"}]}