{"thread":{"id":"58254","subject":"[RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking","startedAt":"2022-08-02T04:13:13Z","lastAt":"2022-08-20T07:04:54Z","messageCount":44,"participants":["Jeff King","Junio C Hamano","René Scharfe","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"460404","messageId":"YuikU//9OrdpKQcE@coredump.intra.peff.net","threadId":"58254","inReplyTo":null,"subject":"[RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-02T04:13:07Z","receivedAt":"2022-08-02T04:13:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our pipe_command() helper lets you both write to and read from a child\nprocess on its stdin/stdout. It's supposed to work without deadlocks\nbecause we use poll() to check when descriptors are ready for reading or\nwriting. But there's a bug: if both the data to be written and the data\nto be read back exceed the pipe buffer, we'll deadlock.\n\nThe issue is that the code assumes that if you have, say, a 2MB buffer\nto write and poll() tells you that the pipe descriptor is ready for\nwriting, that calling:\n\n write(cmd->in, buf, 2*1024*1024);\n\nwill do a partial write, filling the pipe buffer and then returning what\nit did write. And that is what it would do on a socket, but not for a\npipe. When writing to a pipe, at least on Linux, it will block waiting\nfor the child process to read() more. And now we have a potential\ndeadlock, because the child may be writing back to us, waiting for us to\nread() ourselves.\n\nAn easy way to trigger this is:\n\n git -c add.interactive.useBuiltin=true \\\n -c interactive.diffFilter=cat \\\n checkout -p HEAD~200\n\nThe diff against HEAD~200 will be big, and the filter wants to write all\nof it back to us (obviously this is a dummy filter, but in the real\nworld something like diff-highlight would similarly stream back a big\noutput).\n\nIf you set add.interactive.useBuiltin to false, the problem goes away,\nbecause now we're not using pipe_command() anymore (instead, that part\nhappens in perl). But this isn't a bug in the interactive code at all.\nIt's the underlying pipe_command() code which is broken, and has been\nall along.\n\nWe presumably didn't notice because most calls only do input _or_\noutput, not both. And the few that do both, like gpg calls, may have\nlarge inputs or outputs, but never both at the same time (e.g., consider\nsigning, which has a large payload but a small signature comes back).\n\nThe obvious fix is to put the descriptor into non-blocking mode, and\nindeed, that makes the problem go away. Callers shouldn't need to\ncare, because they never see the descriptor (they hand us a buffer to\nfeed into it).\n\nSigned-off-by: Jeff King \n---\n+cc Dscho for two reasons:\n\n - this is a fallout of the builtin add-interactive, though I again\n emphasize that it's just triggering a lurking bug (which is mine, no\n less!). But I thought you'd want to know.\n\n - more importantly, I'm not sure of the portability implications of\n the fix. This is our first use of O_NONBLOCK outside of the\n compat/simple-ipc unix-socket code. Do we need to abstract this\n behind a compat/ layer for Windows?\n\n run-command.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 14f17830f5..45bffb4b11 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1418,6 +1418,14 @@ static int pump_io(struct io_pump *slots, int nr)\n \treturn 0;\n }\n \n+static int make_nonblock(int fd)\n+{\n+\tint flags = fcntl(fd, F_GETFL);\n+\tif (flags < 0)\n+\t\treturn -1;\n+\tflags |= O_NONBLOCK;\n+\treturn fcntl(fd, F_SETFL, flags);\n+}\n \n int pipe_command(struct child_process *cmd,\n \t\t const char *in, size_t in_len,\n@@ -1438,6 +1446,15 @@ int pipe_command(struct child_process *cmd,\n \t\treturn -1;\n \n \tif (in) {\n+\t\tif (make_nonblock(cmd->in) < 0) {\n+\t\t\terror_errno(\"unable to make pipe non-blocking\");\n+\t\t\tclose(cmd->in);\n+\t\t\tif (out)\n+\t\t\t\tclose(cmd->out);\n+\t\t\tif (err)\n+\t\t\t\tclose(cmd->err);\n+\t\t\treturn -1;\n+\t\t}\n \t\tio[nr].fd = cmd->in;\n \t\tio[nr].type = POLLOUT;\n \t\tio[nr].u.out.buf = in;\n-- \n2.37.1.804.g1775fa20e0\n\n"},{"id":"460433","messageId":"xmqqbkt2k7p4.fsf@gitster.g","threadId":"58254","inReplyTo":"YuikU//9OrdpKQcE@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-02T15:04:39Z","receivedAt":"2022-08-02T15:04:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Our pipe_command() helper lets you both write to and read from a child\n> process on its stdin/stdout. It's supposed to work without deadlocks\n> because we use poll() to check when descriptors are ready for reading or\n> writing. But there's a bug: if both the data to be written and the data\n> to be read back exceed the pipe buffer, we'll deadlock.\n> ...\n> If you set add.interactive.useBuiltin to false, the problem goes away,\n> because now we're not using pipe_command() anymore (instead, that part\n> happens in perl). But this isn't a bug in the interactive code at all.\n> It's the underlying pipe_command() code which is broken, and has been\n> all along.\n> ...\n> The obvious fix is to put the descriptor into non-blocking mode, and\n> indeed, that makes the problem go away. Callers shouldn't need to\n> care, because they never see the descriptor (they hand us a buffer to\n> feed into it).\n\nThanks for a very well reasoned and explained patch.\n\n> - more importantly, I'm not sure of the portability implications of\n> the fix. This is our first use of O_NONBLOCK outside of the\n> compat/simple-ipc unix-socket code. Do we need to abstract this\n> behind a compat/ layer for Windows?\n\nYup. A very good question to ask for the platform maintainer.\n\n> run-command.c | 17 +++++++++++++++++\n> 1 file changed, 17 insertions(+)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 14f17830f5..45bffb4b11 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1418,6 +1418,14 @@ static int pump_io(struct io_pump *slots, int nr)\n> \treturn 0;\n> }\n> \n> +static int make_nonblock(int fd)\n> +{\n> +\tint flags = fcntl(fd, F_GETFL);\n> +\tif (flags < 0)\n> +\t\treturn -1;\n> +\tflags |= O_NONBLOCK;\n> +\treturn fcntl(fd, F_SETFL, flags);\n> +}\n> \n> int pipe_command(struct child_process *cmd,\n> \t\t const char *in, size_t in_len,\n> @@ -1438,6 +1446,15 @@ int pipe_command(struct child_process *cmd,\n> \t\treturn -1;\n> \n> \tif (in) {\n> +\t\tif (make_nonblock(cmd->in) < 0) {\n> +\t\t\terror_errno(\"unable to make pipe non-blocking\");\n> +\t\t\tclose(cmd->in);\n> +\t\t\tif (out)\n> +\t\t\t\tclose(cmd->out);\n> +\t\t\tif (err)\n> +\t\t\t\tclose(cmd->err);\n> +\t\t\treturn -1;\n> +\t\t}\n> \t\tio[nr].fd = cmd->in;\n> \t\tio[nr].type = POLLOUT;\n> \t\tio[nr].u.out.buf = in;\n"},{"id":"460443","messageId":"YulFTSTbVaTwuQtt@coredump.intra.peff.net","threadId":"58254","inReplyTo":"YuikU//9OrdpKQcE@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-02T15:39:57Z","receivedAt":"2022-08-02T15:40:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 02, 2022 at 12:13:07AM -0400, Jeff King wrote:\n\n> - more importantly, I'm not sure of the portability implications of\n> the fix. This is our first use of O_NONBLOCK outside of the\n> compat/simple-ipc unix-socket code. Do we need to abstract this\n> behind a compat/ layer for Windows?\n\nSo I think the answer is pretty clearly \"yes\", from the Windows CI\nresults:\n\n run-command.c:1429:18: 'O_NONBLOCK' undeclared (first use in this function)\n flags |= O_NONBLOCK;\n ^~~~~~~~~~\n\nIt looks like we'd have the option of either adding F_GETFL/F_SETFL\nsupport to compat/mingw.c's fake fcntl() function, or adding a\ncompat/nonblock.c that abstracts the whole thing. I'd prefer the latter,\nas it makes the interface a bit narrower.\n\nBut I'm not sure what should go on the Windows side of that #ifdef.\nUnlike some other spots, I don't think we can just make it a noop, or\nWindows will be subject to the same deadlock (unless for some reason its\nwrite() does behave differently?).\n\n-Peff\n"},{"id":"460495","messageId":"xmqq5yjahb8u.fsf@gitster.g","threadId":"58254","inReplyTo":"YulFTSTbVaTwuQtt@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-02T16:16:17Z","receivedAt":"2022-08-02T16:17:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It looks like we'd have the option of either adding F_GETFL/F_SETFL\n> support to compat/mingw.c's fake fcntl() function, or adding a\n> compat/nonblock.c that abstracts the whole thing. I'd prefer the latter,\n> as it makes the interface a bit narrower.\n\nI agree that \"make this non-blocking\" is the preferred interface\nover \"emuilate fcntl\".\n\n> But I'm not sure what should go on the Windows side of that #ifdef.\n> Unlike some other spots, I don't think we can just make it a noop, or\n> Windows will be subject to the same deadlock (unless for some reason its\n> write() does behave differently?).\n\nLet them deadlock so that they can fix it, and until then leave it a\nnoop? That may break the CI tests for them so we could hide the\nknown-to-be-broken test behind prerequisite to buy them time,\nperhaps?\n"},{"id":"460525","messageId":"YunxHOa2sJeEpJxd@coredump.intra.peff.net","threadId":"58254","inReplyTo":"xmqq5yjahb8u.fsf@gitster.g","subject":"[PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-03T03:53:00Z","receivedAt":"2022-08-03T03:53:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 02, 2022 at 09:16:17AM -0700, Junio C Hamano wrote:\n\n> > But I'm not sure what should go on the Windows side of that #ifdef.\n> > Unlike some other spots, I don't think we can just make it a noop, or\n> > Windows will be subject to the same deadlock (unless for some reason its\n> > write() does behave differently?).\n> \n> Let them deadlock so that they can fix it, and until then leave it a\n> noop? That may break the CI tests for them so we could hide the\n> known-to-be-broken test behind prerequisite to buy them time,\n> perhaps?\n\nI was skeptical of including a test, but I was able to come up with a\nreproduction that triggers for me on Linux, but isn't too expensive to\nkeep around.\n\nMuch to my surprise (and delight), it passes on the Windows CI with the\nnoop! So I guess their pipes behave in the way we want by default.\n\nIt does feel like a bit of a landmine to have an enable_nonblock()\nfunction which might do nothing. I'm not sure if other descriptor types\nmight behave differently on Windows. But hopefully the comment above the\nfunction is sufficient to make people think twice.\n\n-- >8 --\nSubject: [PATCH] pipe_command(): mark stdin descriptor as non-blocking\n\nOur pipe_command() helper lets you both write to and read from a child\nprocess on its stdin/stdout. It's supposed to work without deadlocks\nbecause we use poll() to check when descriptors are ready for reading or\nwriting. But there's a bug: if both the data to be written and the data\nto be read back exceed the pipe buffer, we'll deadlock.\n\nThe issue is that the code assumes that if you have, say, a 2MB buffer\nto write and poll() tells you that the pipe descriptor is ready for\nwriting, that calling:\n\n write(cmd->in, buf, 2*1024*1024);\n\nwill do a partial write, filling the pipe buffer and then returning what\nit did write. And that is what it would do on a socket, but not for a\npipe. When writing to a pipe, at least on Linux, it will block waiting\nfor the child process to read() more. And now we have a potential\ndeadlock, because the child may be writing back to us, waiting for us to\nread() ourselves.\n\nAn easy way to trigger this is:\n\n git -c add.interactive.useBuiltin=true \\\n -c interactive.diffFilter=cat \\\n checkout -p HEAD~200\n\nThe diff against HEAD~200 will be big, and the filter wants to write all\nof it back to us (obviously this is a dummy filter, but in the real\nworld something like diff-highlight would similarly stream back a big\noutput).\n\nIf you set add.interactive.useBuiltin to false, the problem goes away,\nbecause now we're not using pipe_command() anymore (instead, that part\nhappens in perl). But this isn't a bug in the interactive code at all.\nIt's the underlying pipe_command() code which is broken, and has been\nall along.\n\nWe presumably didn't notice because most calls only do input _or_\noutput, not both. And the few that do both, like gpg calls, may have\nlarge inputs or outputs, but never both at the same time (e.g., consider\nsigning, which has a large payload but a small signature comes back).\n\nThe obvious fix is to put the descriptor into non-blocking mode, and\nindeed, that makes the problem go away. Callers shouldn't need to\ncare, because they never see the descriptor (they hand us a buffer to\nfeed into it).\n\nUnfortunately O_NONBLOCK isn't available everywhere, especially on\nWindows. However, the included test seems to work fine there, which\nimplies that pipes there do not behave in the same way (they will do the\npartial write by default, which is what we want). This is true even if I\nsize up the diff for a larger pipe buffer (the value chosen here\ntriggers the deadlock on Linux, but isn't too expensive to keep as a\nregression test).\n\nSigned-off-by: Jeff King \n---\n Makefile | 1 +\n compat/nonblock.c | 22 ++++++++++++++++++++++\n compat/nonblock.h | 10 ++++++++++\n run-command.c | 10 ++++++++++\n t/t3701-add-interactive.sh | 14 ++++++++++++++\n 5 files changed, 57 insertions(+)\n create mode 100644 compat/nonblock.c\n create mode 100644 compat/nonblock.h\n\ndiff --git a/Makefile b/Makefile\nindex 1624471bad..78bedf26e0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -918,6 +918,7 @@ LIB_OBJS += combine-diff.o\n LIB_OBJS += commit-graph.o\n LIB_OBJS += commit-reach.o\n LIB_OBJS += commit.o\n+LIB_OBJS += compat/nonblock.o\n LIB_OBJS += compat/obstack.o\n LIB_OBJS += compat/terminal.o\n LIB_OBJS += compat/zlib-uncompress2.o\ndiff --git a/compat/nonblock.c b/compat/nonblock.c\nnew file mode 100644\nindex 0000000000..897c099010\n--- /dev/null\n+++ b/compat/nonblock.c\n@@ -0,0 +1,22 @@\n+#include \"git-compat-util.h\"\n+#include \"nonblock.h\"\n+\n+#ifdef O_NONBLOCK\n+\n+int enable_nonblock(int fd)\n+{\n+\tint flags = fcntl(fd, F_GETFL);\n+\tif (flags < 0)\n+\t\treturn -1;\n+\tflags |= O_NONBLOCK;\n+\treturn fcntl(fd, F_SETFL, flags);\n+}\n+\n+#else\n+\n+int enable_nonblock(int fd)\n+{\n+\treturn 0;\n+}\n+\n+#endif\ndiff --git a/compat/nonblock.h b/compat/nonblock.h\nnew file mode 100644\nindex 0000000000..2721f72187\n--- /dev/null\n+++ b/compat/nonblock.h\n@@ -0,0 +1,10 @@\n+#ifndef COMPAT_NONBLOCK_H\n+#define COMPAT_NONBLOCK_H\n+\n+/*\n+ * Enable non-blocking I/O for the passed-in descriptor. Note that this is a\n+ * noop on systems without O_NONBLOCK, like Windows! Use with caution.\n+ */\n+int enable_nonblock(int fd);\n+\n+#endif\ndiff --git a/run-command.c b/run-command.c\nindex 14f17830f5..ed99503b22 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -10,6 +10,7 @@\n #include \"config.h\"\n #include \"packfile.h\"\n #include \"hook.h\"\n+#include \"compat/nonblock.h\"\n \n void child_process_init(struct child_process *child)\n {\n@@ -1438,6 +1439,15 @@ int pipe_command(struct child_process *cmd,\n \t\treturn -1;\n \n \tif (in) {\n+\t\tif (enable_nonblock(cmd->in) < 0) {\n+\t\t\terror_errno(\"unable to make pipe non-blocking\");\n+\t\t\tclose(cmd->in);\n+\t\t\tif (out)\n+\t\t\t\tclose(cmd->out);\n+\t\t\tif (err)\n+\t\t\t\tclose(cmd->err);\n+\t\t\treturn -1;\n+\t\t}\n \t\tio[nr].fd = cmd->in;\n \t\tio[nr].type = POLLOUT;\n \t\tio[nr].u.out.buf = in;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex b354fb39de..01d6ce9c83 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -766,6 +766,20 @@ test_expect_success 'detect bogus diffFilter output' '\n \tforce_color test_must_fail git add -p test &&\n+\tfalse &&\n+\ttest_config interactive.diffFilter cat &&\n+\tprintf y >y &&\n+\tforce_color git add -p >output 2>&1 diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index b354fb39de..01d6ce9c83 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -766,6 +766,20 @@ test_expect_success 'detect bogus diffFilter output' '\n> \tforce_color test_must_fail git add -p '\n>\n> +test_expect_success 'handle very large filtered diff' '\n> +\tgit reset --hard &&\n> +\t# The specific number here is not important, but it must\n> +\t# be large enough that the output of \"git diff --color\"\n> +\t# fills up the pipe buffer. 10,000 results in ~200k of\n> +\t# colored output.\n> +\ttest_seq 10000 >test &&\n> +\tfalse &&\n\nIsn't this test going to end here, reporting failure before it even gets\nto the interesting part?\n\n> +\ttest_config interactive.diffFilter cat &&\n> +\tprintf y >y &&\n> +\tforce_color git add -p >output 2>&1 +\tgit diff-files --exit-code -- test\n> +'\n> +\n> test_expect_success 'diff.algorithm is passed to `git diff-files`' '\n> \tgit reset --hard &&\n>\n\n"},{"id":"460562","messageId":"YuquVEqEl6wxCLM7@coredump.intra.peff.net","threadId":"58254","inReplyTo":"a9953278-b15f-fd76-17b0-e949c7937992@web.de","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-03T17:20:20Z","receivedAt":"2022-08-03T17:20:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 03, 2022 at 06:45:23PM +0200, René Scharfe wrote:\n\n> > +test_expect_success 'handle very large filtered diff' '\n> > +\tgit reset --hard &&\n> > +\t# The specific number here is not important, but it must\n> > +\t# be large enough that the output of \"git diff --color\"\n> > +\t# fills up the pipe buffer. 10,000 results in ~200k of\n> > +\t# colored output.\n> > +\ttest_seq 10000 >test &&\n> > +\tfalse &&\n> \n> Isn't this test going to end here, reporting failure before it even gets\n> to the interesting part?\n\nUrgh, whoops. That was from some last-minute tweaking of the comment.\nThere was also a line:\n\n git diff --color | wc -c\n\nbefore it so I could measure how big the output was for a few values.\n\nIt snuck into the emailed patch, but the actual test runs (including the\nWindows CI) didn't include that (since obviously they'd have failed the\ntest).\n\nThanks for catching.\n\n-Peff\n"},{"id":"460577","messageId":"41477326-5493-4d3c-246d-8a28969fa73e@web.de","threadId":"58254","inReplyTo":"YuquVEqEl6wxCLM7@coredump.intra.peff.net","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-03T21:56:13Z","receivedAt":"2022-08-03T21:56:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.08.2022 um 19:20 schrieb Jeff King:\n> On Wed, Aug 03, 2022 at 06:45:23PM +0200, René Scharfe wrote:\n>\n>>> +test_expect_success 'handle very large filtered diff' '\n>>> +\tgit reset --hard &&\n>>> +\t# The specific number here is not important, but it must\n>>> +\t# be large enough that the output of \"git diff --color\"\n>>> +\t# fills up the pipe buffer. 10,000 results in ~200k of\n>>> +\t# colored output.\n>>> +\ttest_seq 10000 >test &&\n>>> +\tfalse &&\n>>\n>> Isn't this test going to end here, reporting failure before it even gets\n>> to the interesting part?\n>\n> Urgh, whoops. That was from some last-minute tweaking of the comment.\n> There was also a line:\n>\n> git diff --color | wc -c\n>\n> before it so I could measure how big the output was for a few values.\n>\n> It snuck into the emailed patch, but the actual test runs (including the\n> Windows CI) didn't include that (since obviously they'd have failed the\n> test).\n\nWithout that line the added test hangs for me on the Git for Windows\nSDK on Windows 11.\n\nWith the patch below it fails and reports basically nothing:\n\n expecting success of 3701.57 'handle very large filtered diff':\n git reset --hard &&\n # The specific number here is not important, but it must\n # be large enough that the output of \"git diff --color\"\n # fills up the pipe buffer. 10,000 results in ~200k of\n # colored output.\n test_seq 10000 >test &&\n test_config interactive.diffFilter cat &&\n printf y >y &&\n force_color git add -p >output 2>&1 test &&\n # test_config interactive.diffFilter cat &&\n # printf y >y &&\n # force_color git add -p >output 2>&1 Without that line the added test hangs for me on the Git for Windows\n> SDK on Windows 11.\n\nHmph. Interesting that it passes in CI, but not on your local setup.\nI wonder why pipes would behave differently. Or perhaps there is even\nsome configuration different that means we are still running the perl\nadd--interactive there, though I kind of doubt it (and couldn't find\nanything pointing there).\n\nStill, if it fails in at least one spot, it's something we need to deal\nwith. On the plus side, once we figure out how to fix it, the\nhand-grenade of \"enable_nonblock() does nothing on Windows\" will not be\npresent anymore. ;)\n\n> With the patch below it fails and reports basically nothing:\n> [...]\n> not ok 57 - handle very large filtered diff\n> #\n> # git reset --hard &&\n> # # The specific number here is not important, but it must\n> # # be large enough that the output of \"git diff --color\"\n> # # fills up the pipe buffer. 10,000 results in ~200k of\n> # # colored output.\n> # test_seq 10000 >test &&\n> # test_config interactive.diffFilter cat &&\n> # printf y >y &&\n> # force_color git add -p >output 2>&1 # git diff-files --exit-code -- test\n> #\n> 1..57\n> \n> The file \"output\" contains \"error: failed to run 'cat'\". This is\n> add-patch.c::parse_diff() reporting that pipe_command() failed. So\n> that's not it, yet. (I don't actually know what I'm doing here.)\n\nThat implies that your call to enable_nonblock() succeeded, or\npipe_command() itself would have complained, too. Maybe instrument\nit like this:\n\ndiff --git a/run-command.c b/run-command.c\nindex 8ea609d4ae..27e79c928a 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1473,11 +1473,17 @@ int pipe_command(struct child_process *cmd,\n \t}\n \n \tif (pump_io(io, nr) < 0) {\n+\t\terror_errno(\"pumping io failed\");\n \t\tfinish_command(cmd); /* throw away exit code */\n \t\treturn -1;\n \t}\n \n-\treturn finish_command(cmd);\n+\t{\n+\t\tint ret = finish_command(cmd);\n+\t\tif (ret)\n+\t\t\terror(\"child returned failure %d\", ret);\n+\t\treturn ret;\n+\t}\n }\n \n enum child_state {\n\nNormally we stay pretty quiet there and let the caller report any\nproblems, but it lacks enough context to make a more specific error\nreport.\n\n> int enable_nonblock(int fd)\n> {\n> +\tDWORD mode;\n> +\tHANDLE handle = winansi_get_osfhandle(fd);\n> +\tif (!handle)\n> +\t\treturn -1;\n> +\tif (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))\n> +\t\treturn -1;\n> +\tif (mode & PIPE_NOWAIT)\n> +\t\treturn 0;\n> +\tmode |= PIPE_NOWAIT;\n> +\tif (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))\n> +\t\treturn -1;\n> \treturn 0;\n> }\n\nThis looks plausibly correct to me. ;) We might want to change the name\nof the compat layer to enable_pipe_nonblock(), since one assumes from\nthe function names this only works for pipes.\n\nThanks for poking at this.\n\n-Peff\n"},{"id":"460741","messageId":"6854c54c-12ff-f613-4cdc-18b3b1a55ef1@web.de","threadId":"58254","inReplyTo":"Yu05GjncDaGRTgce@coredump.intra.peff.net","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-05T21:13:28Z","receivedAt":"2022-08-05T21:13:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.08.2022 um 17:36 schrieb Jeff King:\n> On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:\n>\n>> Without that line the added test hangs for me on the Git for Windows\n>> SDK on Windows 11.\n>\n> Hmph. Interesting that it passes in CI, but not on your local setup.\n> I wonder why pipes would behave differently. Or perhaps there is even\n> some configuration different that means we are still running the perl\n> add--interactive there, though I kind of doubt it (and couldn't find\n> anything pointing there).\n>\n> Still, if it fails in at least one spot, it's something we need to deal\n> with. On the plus side, once we figure out how to fix it, the\n> hand-grenade of \"enable_nonblock() does nothing on Windows\" will not be\n> present anymore. ;)\n>\n>> With the patch below it fails and reports basically nothing:\n>> [...]\n>> not ok 57 - handle very large filtered diff\n>> #\n>> # git reset --hard &&\n>> # # The specific number here is not important, but it must\n>> # # be large enough that the output of \"git diff --color\"\n>> # # fills up the pipe buffer. 10,000 results in ~200k of\n>> # # colored output.\n>> # test_seq 10000 >test &&\n>> # test_config interactive.diffFilter cat &&\n>> # printf y >y &&\n>> # force_color git add -p >output 2>&1 > # git diff-files --exit-code -- test\n>> #\n>> 1..57\n>>\n>> The file \"output\" contains \"error: failed to run 'cat'\". This is\n>> add-patch.c::parse_diff() reporting that pipe_command() failed. So\n>> that's not it, yet. (I don't actually know what I'm doing here.)\n>\n> That implies that your call to enable_nonblock() succeeded, or\n> pipe_command() itself would have complained, too. Maybe instrument\n> it like this:\n>\n> diff --git a/run-command.c b/run-command.c\n> index 8ea609d4ae..27e79c928a 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1473,11 +1473,17 @@ int pipe_command(struct child_process *cmd,\n> \t}\n>\n> \tif (pump_io(io, nr) < 0) {\n> +\t\terror_errno(\"pumping io failed\");\n> \t\tfinish_command(cmd); /* throw away exit code */\n> \t\treturn -1;\n> \t}\n>\n> -\treturn finish_command(cmd);\n> +\t{\n> +\t\tint ret = finish_command(cmd);\n> +\t\tif (ret)\n> +\t\t\terror(\"child returned failure %d\", ret);\n> +\t\treturn ret;\n> +\t}\n> }\n>\n> enum child_state {\n>\n> Normally we stay pretty quiet there and let the caller report any\n> problems, but it lacks enough context to make a more specific error\n> report.\n\nThis adds \"error: pumping io failed: No space left on device\" to output.\nWhich kinda makes sense: With the pipe no longer blocking, there can be\na moment when the buffer is full and writes have to be rejected. This\ncondition should be reported with EAGAIN, though.\n\nAdding \"if (len < 0 && errno == ENOSPC) continue;\" after the xwrite()\ncall in pump_io_round() lets the test pass.\n\nPerhaps the translation from Windows error code to POSIX is wrong here?\n\n>\n>> int enable_nonblock(int fd)\n>> {\n>> +\tDWORD mode;\n>> +\tHANDLE handle = winansi_get_osfhandle(fd);\n>> +\tif (!handle)\n>> +\t\treturn -1;\n>> +\tif (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))\n>> +\t\treturn -1;\n>> +\tif (mode & PIPE_NOWAIT)\n>> +\t\treturn 0;\n>> +\tmode |= PIPE_NOWAIT;\n>> +\tif (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))\n>> +\t\treturn -1;\n>> \treturn 0;\n>> }\n>\n> This looks plausibly correct to me. ;) We might want to change the name\n> of the compat layer to enable_pipe_nonblock(), since one assumes from\n> the function names this only works for pipes.\n\nRight, and the \"Named\" part should be explained: As [1] says: \"[...] you\ncan often pass a handle to an anonymous pipe to a function that requires\na handle to a named pipe.\".\n\nAnd perhaps that's not the right thing to do after all, as [2] says:\n\"Note that nonblocking mode is supported for compatibility with\nMicrosoft LAN Manager version 2.0 and should not be used to achieve\nasynchronous input and output (I/O) [...]\".\n\n[1] https://docs.microsoft.com/en-us/windows/win32/ipc/anonymous-pipe-operations\n[2] https://docs.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-setnamedpipehandlestate\n\n>\n> Thanks for poking at this.\n>\n> -Peff\n"},{"id":"460785","messageId":"b3310324-7969-f9fb-a2e0-46e881d37786@web.de","threadId":"58254","inReplyTo":"6854c54c-12ff-f613-4cdc-18b3b1a55ef1@web.de","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-07T10:15:06Z","receivedAt":"2022-08-07T10:15:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.08.2022 um 23:13 schrieb René Scharfe:\n> Am 05.08.2022 um 17:36 schrieb Jeff King:\n>> On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:\n>>\n>>> Without that line the added test hangs for me on the Git for Windows\n>>> SDK on Windows 11.\n>>\n>> Hmph. Interesting that it passes in CI, but not on your local setup.\n>> I wonder why pipes would behave differently. Or perhaps there is even\n>> some configuration different that means we are still running the perl\n>> add--interactive there, though I kind of doubt it (and couldn't find\n>> anything pointing there).\n>>\n>> Still, if it fails in at least one spot, it's something we need to deal\n>> with. On the plus side, once we figure out how to fix it, the\n>> hand-grenade of \"enable_nonblock() does nothing on Windows\" will not be\n>> present anymore. ;)\n>>\n>>> With the patch below it fails and reports basically nothing:\n>>> [...]\n>>> not ok 57 - handle very large filtered diff\n>>> #\n>>> # git reset --hard &&\n>>> # # The specific number here is not important, but it must\n>>> # # be large enough that the output of \"git diff --color\"\n>>> # # fills up the pipe buffer. 10,000 results in ~200k of\n>>> # # colored output.\n>>> # test_seq 10000 >test &&\n>>> # test_config interactive.diffFilter cat &&\n>>> # printf y >y &&\n>>> # force_color git add -p >output 2>&1 >> # git diff-files --exit-code -- test\n>>> #\n>>> 1..57\n>>>\n>>> The file \"output\" contains \"error: failed to run 'cat'\". This is\n>>> add-patch.c::parse_diff() reporting that pipe_command() failed. So\n>>> that's not it, yet. (I don't actually know what I'm doing here.)\n>>\n>> That implies that your call to enable_nonblock() succeeded, or\n>> pipe_command() itself would have complained, too. Maybe instrument\n>> it like this:\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index 8ea609d4ae..27e79c928a 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -1473,11 +1473,17 @@ int pipe_command(struct child_process *cmd,\n>> \t}\n>>\n>> \tif (pump_io(io, nr) < 0) {\n>> +\t\terror_errno(\"pumping io failed\");\n>> \t\tfinish_command(cmd); /* throw away exit code */\n>> \t\treturn -1;\n>> \t}\n>>\n>> -\treturn finish_command(cmd);\n>> +\t{\n>> +\t\tint ret = finish_command(cmd);\n>> +\t\tif (ret)\n>> +\t\t\terror(\"child returned failure %d\", ret);\n>> +\t\treturn ret;\n>> +\t}\n>> }\n>>\n>> enum child_state {\n>>\n>> Normally we stay pretty quiet there and let the caller report any\n>> problems, but it lacks enough context to make a more specific error\n>> report.\n\nSide note: The above patch gives better insight on error, but also\nbreaks t4205.105 (%(describe) vs git describe), t7400.16 (submodule add\nto .gitignored path fails) and t7406.59 (submodule update --quiet passes\nquietness to fetch with a shallow clone).\n\n>\n> This adds \"error: pumping io failed: No space left on device\" to output.\n> Which kinda makes sense: With the pipe no longer blocking, there can be\n> a moment when the buffer is full and writes have to be rejected. This\n> condition should be reported with EAGAIN, though.\n>\n> Adding \"if (len < 0 && errno == ENOSPC) continue;\" after the xwrite()\n> call in pump_io_round() lets the test pass.\n>\n> Perhaps the translation from Windows error code to POSIX is wrong here?\n\nSo if we fix that with the patch below, t3701.57 still hangs, but this\ntime it goes through wrapper.c::handle_nonblock() again and again.\nReplacing the \"errno = EAGAIN\" with a \"return 0\" to fake report a\nsuccessful write of nothing instead lets the test pass.\n\nThis seems to make sense -- looping in xwrite() won't help, as we need\nto read from the other fd first, to allow the process on the other end\nof the pipe to make some progress first, as otherwise the pipe buffer\nwill stay full in this scenario. Shouldn't that be a problem on other\nsystems as well?\n\nRené\n\n---\n compat/mingw.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex b5502997e2..e34cceb151 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -702,6 +702,17 @@ ssize_t mingw_write(int fd, const void *buf, size_t len)\n \t\telse\n \t\t\terrno = EINVAL;\n \t}\n+\tif (result < 0 && errno == ENOSPC) {\n+\t\t/* check if fd is a non-blocking pipe */\n+\t\tHANDLE h = (HANDLE) _get_osfhandle(fd);\n+\t\tDWORD m;\n+\t\tif (GetFileType(h) == FILE_TYPE_PIPE &&\n+\t\t GetNamedPipeHandleState(h, &m, NULL, NULL, NULL, NULL, 0) &&\n+\t\t (m & PIPE_NOWAIT))\n+\t\t\terrno = EAGAIN;\n+\t\telse\n+\t\t\terrno = ENOSPC;\n+\t}\n\n \treturn result;\n }\n--\n2.37.1.windows.1\n"},{"id":"460786","messageId":"362560ca-1e37-bab3-e508-4d5bf5429e66@web.de","threadId":"58254","inReplyTo":"Yu05GjncDaGRTgce@coredump.intra.peff.net","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-07T10:14:54Z","receivedAt":"2022-08-07T10:16:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.08.2022 um 17:36 schrieb Jeff King:\n> On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:\n> >> int enable_nonblock(int fd)\n>> {\n>> +\tDWORD mode;\n>> +\tHANDLE handle = winansi_get_osfhandle(fd);\n>> +\tif (!handle)\n>> +\t\treturn -1;\n>> +\tif (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))\n>> +\t\treturn -1;\n>> +\tif (mode & PIPE_NOWAIT)\n>> +\t\treturn 0;\n>> +\tmode |= PIPE_NOWAIT;\n>> +\tif (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))\n>> +\t\treturn -1;\n>> \treturn 0;\n>> }\n>\n> This looks plausibly correct to me. ;) We might want to change the name\n> of the compat layer to enable_pipe_nonblock(), since one assumes from\n> the function names this only works for pipes.\n\nOr how about this? Squashable. Needs testing.\n\n--- >8 ---\nSubject: [PATCH] nonblock: support Windows\n\nImplement enable_nonblock() using the Windows API. Supports only named\nand anonymous pipes for now, which suffices for the current caller.\n\nSigned-off-by: René Scharfe \n---\n compat/nonblock.c | 31 +++++++++++++++++++++++++++++++\n 1 file changed, 31 insertions(+)\n\ndiff --git a/compat/nonblock.c b/compat/nonblock.c\nindex 897c099010..78923cd2c3 100644\n--- a/compat/nonblock.c\n+++ b/compat/nonblock.c\n@@ -14,9 +14,40 @@ int enable_nonblock(int fd)\n\n #else\n\n+#ifdef GIT_WINDOWS_NATIVE\n+\n+#include \"win32.h\"\n+\n+int enable_nonblock(int fd)\n+{\n+\tHANDLE h = (HANDLE)_get_osfhandle(fd);\n+\tDWORD mode;\n+\tDWORD type = GetFileType(h);\n+\tif (type == FILE_TYPE_UNKNOWN && GetLastError() != NO_ERROR) {\n+\t\terrno = EBADF;\n+\t\treturn -1;\n+\t}\n+\tif (type != FILE_TYPE_PIPE)\n+\t\tBUG(\"unsupported file type: %lu\", type);\n+\tif (!GetNamedPipeHandleState(h, &mode, NULL, NULL, NULL, NULL, 0)) {\n+\t\terrno = err_win_to_posix(GetLastError());\n+\t\treturn -1;\n+\t}\n+\tmode |= PIPE_NOWAIT;\n+\tif (!SetNamedPipeHandleState(h, &mode, NULL, NULL)) {\n+\t\terrno = err_win_to_posix(GetLastError());\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n+}\n+\n+#else\n+\n int enable_nonblock(int fd)\n {\n \treturn 0;\n }\n\n #endif\n+\n+#endif\n--\n2.37.1.windows.1\n"},{"id":"460794","messageId":"Yu/5LU+ZhbVRnSdM@coredump.intra.peff.net","threadId":"58254","inReplyTo":"b3310324-7969-f9fb-a2e0-46e881d37786@web.de","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-07T17:41:01Z","receivedAt":"2022-08-07T17:41:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 07, 2022 at 12:15:06PM +0200, René Scharfe wrote:\n\n> > This adds \"error: pumping io failed: No space left on device\" to output.\n> > Which kinda makes sense: With the pipe no longer blocking, there can be\n> > a moment when the buffer is full and writes have to be rejected. This\n> > condition should be reported with EAGAIN, though.\n> >\n> > Adding \"if (len < 0 && errno == ENOSPC) continue;\" after the xwrite()\n> > call in pump_io_round() lets the test pass.\n> >\n> > Perhaps the translation from Windows error code to POSIX is wrong here?\n> \n> So if we fix that with the patch below, t3701.57 still hangs, but this\n> time it goes through wrapper.c::handle_nonblock() again and again.\n> Replacing the \"errno = EAGAIN\" with a \"return 0\" to fake report a\n> successful write of nothing instead lets the test pass.\n> \n> This seems to make sense -- looping in xwrite() won't help, as we need\n> to read from the other fd first, to allow the process on the other end\n> of the pipe to make some progress first, as otherwise the pipe buffer\n> will stay full in this scenario. Shouldn't that be a problem on other\n> systems as well?\n\nIt doesn't happen on Linux; I suspect there's something funny either\nabout partial writes, or about poll() on Windows. What's supposed to\nhappen is:\n\n 1. pump_io() calls poll(), which tells us the descriptor is ready to\n write\n\n 2. we call xwrite(), and our actual write() call returns a partial\n write (i.e., reports \"ret\" bytes < \"len\" we passed in)\n\n 3. we return back to pump_io() do another round of poll(). If the\n other side consumed some bytes from the pipe, then we may get\n triggered to do another (possibly partial) write. If it didn't, and\n we'd get EAGAIN writing, then poll shouldn't trigger at all!\n\nSo it's weird that you'd see EAGAIN in this instance. Either the\nunderlying write() is refusing to do a partial write (and just returning\nan error with EAGAIN in the first place), or the poll emulation is wrong\n(telling us the descriptor is ready for writing when it isn't).\n\nCan you instrument pump_io_round() (or use some strace equivalent, if\nthere is one) to see if we do a successful partial write first (which\nimplies poll() is wrong in telling us we can write more for the second\nround), or if the very first write() is failing (which implies write()\nis wrong for returning EAGAIN when it could do a partial write).\n\n-Peff\n"},{"id":"460813","messageId":"099911o8-43q1-o29s-5s36-34r5qn37nq8n@tzk.qr","threadId":"58254","inReplyTo":"362560ca-1e37-bab3-e508-4d5bf5429e66@web.de","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-08T12:55:39Z","receivedAt":"2022-08-08T12:55:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Sun, 7 Aug 2022, René Scharfe wrote:\n\n> Am 05.08.2022 um 17:36 schrieb Jeff King:\n> > On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:\n> > >> int enable_nonblock(int fd)\n> >> {\n> >> +\tDWORD mode;\n> >> +\tHANDLE handle = winansi_get_osfhandle(fd);\n> >> +\tif (!handle)\n> >> +\t\treturn -1;\n> >> +\tif (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))\n> >> +\t\treturn -1;\n> >> +\tif (mode & PIPE_NOWAIT)\n> >> +\t\treturn 0;\n> >> +\tmode |= PIPE_NOWAIT;\n> >> +\tif (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))\n> >> +\t\treturn -1;\n> >> \treturn 0;\n> >> }\n> >\n> > This looks plausibly correct to me. ;) We might want to change the name\n> > of the compat layer to enable_pipe_nonblock(), since one assumes from\n> > the function names this only works for pipes.\n>\n> Or how about this? Squashable. Needs testing.\n>\n> --- >8 ---\n> Subject: [PATCH] nonblock: support Windows\n>\n> Implement enable_nonblock() using the Windows API. Supports only named\n> and anonymous pipes for now, which suffices for the current caller.\n>\n> Signed-off-by: René Scharfe \n> ---\n> compat/nonblock.c | 31 +++++++++++++++++++++++++++++++\n> 1 file changed, 31 insertions(+)\n>\n> diff --git a/compat/nonblock.c b/compat/nonblock.c\n> index 897c099010..78923cd2c3 100644\n> --- a/compat/nonblock.c\n> +++ b/compat/nonblock.c\n> @@ -14,9 +14,40 @@ int enable_nonblock(int fd)\n>\n> #else\n>\n> +#ifdef GIT_WINDOWS_NATIVE\n\nMaybe use an `#elif defined(GIT_WINDOWS_NATIVE)` here? That would make the\ncode structures clearer, methinks.\n\n> +\n> +#include \"win32.h\"\n> +\n> +int enable_nonblock(int fd)\n> +{\n> +\tHANDLE h = (HANDLE)_get_osfhandle(fd);\n> +\tDWORD mode;\n> +\tDWORD type = GetFileType(h);\n> +\tif (type == FILE_TYPE_UNKNOWN && GetLastError() != NO_ERROR) {\n> +\t\terrno = EBADF;\n> +\t\treturn -1;\n> +\t}\n> +\tif (type != FILE_TYPE_PIPE)\n> +\t\tBUG(\"unsupported file type: %lu\", type);\n> +\tif (!GetNamedPipeHandleState(h, &mode, NULL, NULL, NULL, NULL, 0)) {\n> +\t\terrno = err_win_to_posix(GetLastError());\n> +\t\treturn -1;\n> +\t}\n> +\tmode |= PIPE_NOWAIT;\n> +\tif (!SetNamedPipeHandleState(h, &mode, NULL, NULL)) {\n\nNice.\n\nFWIW the documentation of `PIPE_NOWAIT` says:\n\n\tNote that nonblocking mode is supported for compatibility with\n\tMicrosoft LAN Manager version 2.0 and should not be used to\n\tachieve asynchronous input and output (I/O) with named pipes.\n\n(see\nhttps://docs.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-setnamedpipehandlestate\nfor full details)\n\nThere is little more information to be found on the interwebs, the closest\nto an in-depth explanation is here:\nhttps://devblogs.microsoft.com/oldnewthing/20110114-00/?p=11753\n\nEven if that comment suggests that this mode is deprecated, I think it is\nsafe to rely on it in Git's source code.\n\nThanks,\nDscho\n\n> +\t\terrno = err_win_to_posix(GetLastError());\n> +\t\treturn -1;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +#else\n> +\n> int enable_nonblock(int fd)\n> {\n> \treturn 0;\n> }\n>\n> #endif\n> +\n> +#endif\n> --\n> 2.37.1.windows.1\n>\n"},{"id":"460814","messageId":"q341oso8-1ps6-65n6-s394-n8q433q79nr2@tzk.qr","threadId":"58254","inReplyTo":"YunxHOa2sJeEpJxd@coredump.intra.peff.net","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-08T12:59:49Z","receivedAt":"2022-08-08T13:00:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 2 Aug 2022, Jeff King wrote:\n\n> diff --git a/run-command.c b/run-command.c\n> index 14f17830f5..ed99503b22 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1438,6 +1439,15 @@ int pipe_command(struct child_process *cmd,\n> \t\treturn -1;\n>\n> \tif (in) {\n> +\t\tif (enable_nonblock(cmd->in) < 0) {\n> +\t\t\terror_errno(\"unable to make pipe non-blocking\");\n\nIt might be a bit heavy-handed to error out in this case, as it usually\ndoes not cause problems. At least that's what the fact suggests to me that\nI personally never encountered the dead-lock myself, and neither do I\nrecall anybody piping more than two megabytes through `git checkout -p`.\n\nCould we turn this into `warning_errno()` and avoid reporting an error\nhere? We could add \"; This might hang the process if large amounts of data\nare piped to/from \" to improve the user experience.\n\nCiao,\nDscho\n\n> +\t\t\tclose(cmd->in);\n> +\t\t\tif (out)\n> +\t\t\t\tclose(cmd->out);\n> +\t\t\tif (err)\n> +\t\t\t\tclose(cmd->err);\n> +\t\t\treturn -1;\n> +\t\t}\n> \t\tio[nr].fd = cmd->in;\n> \t\tio[nr].type = POLLOUT;\n> \t\tio[nr].u.out.buf = in;\n"},{"id":"460904","messageId":"YvJbXJyaKz5QPYdz@coredump.intra.peff.net","threadId":"58254","inReplyTo":"q341oso8-1ps6-65n6-s394-n8q433q79nr2@tzk.qr","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-09T13:04:28Z","receivedAt":"2022-08-09T13:04:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 08, 2022 at 02:59:49PM +0200, Johannes Schindelin wrote:\n\n> On Tue, 2 Aug 2022, Jeff King wrote:\n> \n> > diff --git a/run-command.c b/run-command.c\n> > index 14f17830f5..ed99503b22 100644\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -1438,6 +1439,15 @@ int pipe_command(struct child_process *cmd,\n> > \t\treturn -1;\n> >\n> > \tif (in) {\n> > +\t\tif (enable_nonblock(cmd->in) < 0) {\n> > +\t\t\terror_errno(\"unable to make pipe non-blocking\");\n> \n> It might be a bit heavy-handed to error out in this case, as it usually\n> does not cause problems. At least that's what the fact suggests to me that\n> I personally never encountered the dead-lock myself, and neither do I\n> recall anybody piping more than two megabytes through `git checkout -p`.\n\nThat thought crossed my mind, as well, but I'm hesitant to leave a known\nbug in place that can cause a deadlock. It would be one thing if we\ncould muddle through without nonblock in a slower way, but I don't think\nwe can easily detect this situation after the fact.\n\nSo maybe some options are:\n\n - don't bother with O_NONBLOCK unless the size of the input is over N\n bytes. The trouble there is that it's not clear what N should be.\n It's fcntl(F_GETPIPE_SZ) on Linux, but that's not portable. We could\n possibly come up with a conservative value if we had a ballpark for\n pipe size on Windows. It feels a bit hacky, though.\n\n - we could actually guess at a deadlock by putting a timeout on the\n poll(). That would also catch hanging or slow filter processes. I\n really hate putting clock-based limits on things, though, as it\n means the tool behaves differently under load. And keep in mind this\n is deep in the pipe_command() code. It happens to only trigger for\n diff filters now, but it may be used in other spots (in fact it\n already is, and it's only the size of current gpg payloads/responses\n that means it doesn't happen to trigger).\n\nStepping back, though, I think we should consider why we'd see an error\nhere. I wouldn't expect it to ever fail on a system where O_NONBLOCK was\nsupported. If we want to make it a silent noop on some platforms, then\nwe can stick that into the enable_nonblock() function (which is what I\ndid, but as René showed, that is probably not a good enough solution).\n\n-Peff\n"},{"id":"460951","messageId":"4o5p308o-9r66-2853-qon0-r1464p2o83qn@tzk.qr","threadId":"58254","inReplyTo":"YvJbXJyaKz5QPYdz@coredump.intra.peff.net","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-09T22:10:07Z","receivedAt":"2022-08-09T22:13:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 9 Aug 2022, Jeff King wrote:\n\n> On Mon, Aug 08, 2022 at 02:59:49PM +0200, Johannes Schindelin wrote:\n>\n> > On Tue, 2 Aug 2022, Jeff King wrote:\n> >\n> > > diff --git a/run-command.c b/run-command.c\n> > > index 14f17830f5..ed99503b22 100644\n> > > --- a/run-command.c\n> > > +++ b/run-command.c\n> > > @@ -1438,6 +1439,15 @@ int pipe_command(struct child_process *cmd,\n> > > \t\treturn -1;\n> > >\n> > > \tif (in) {\n> > > +\t\tif (enable_nonblock(cmd->in) < 0) {\n> > > +\t\t\terror_errno(\"unable to make pipe non-blocking\");\n> >\n> > It might be a bit heavy-handed to error out in this case, as it usually\n> > does not cause problems. At least that's what the fact suggests to me that\n> > I personally never encountered the dead-lock myself, and neither do I\n> > recall anybody piping more than two megabytes through `git checkout -p`.\n\nUgh, I think that my reasoning was flawed, as I somehow based it on the\nassumption that `enable_nonblock()` would return -1 on platforms without\nO_NONBLOCK. Even if I had read that you fall back to returning 0 on those\nplatforms.\n\nAnd only when reading your reply did it occur to me that this was a thinko\non my part.\n\nSo I would like to retract my assessment that it is heavy-handed to error\nout in this case. It would have been if we had errored out on platforms\nwithout O_NONBLOCK support, but we don't.\n\nSorry for the noise,\nDscho\n\n>\n> That thought crossed my mind, as well, but I'm hesitant to leave a known\n> bug in place that can cause a deadlock. It would be one thing if we\n> could muddle through without nonblock in a slower way, but I don't think\n> we can easily detect this situation after the fact.\n>\n> So maybe some options are:\n>\n> - don't bother with O_NONBLOCK unless the size of the input is over N\n> bytes. The trouble there is that it's not clear what N should be.\n> It's fcntl(F_GETPIPE_SZ) on Linux, but that's not portable. We could\n> possibly come up with a conservative value if we had a ballpark for\n> pipe size on Windows. It feels a bit hacky, though.\n>\n> - we could actually guess at a deadlock by putting a timeout on the\n> poll(). That would also catch hanging or slow filter processes. I\n> really hate putting clock-based limits on things, though, as it\n> means the tool behaves differently under load. And keep in mind this\n> is deep in the pipe_command() code. It happens to only trigger for\n> diff filters now, but it may be used in other spots (in fact it\n> already is, and it's only the size of current gpg payloads/responses\n> that means it doesn't happen to trigger).\n>\n> Stepping back, though, I think we should consider why we'd see an error\n> here. I wouldn't expect it to ever fail on a system where O_NONBLOCK was\n> supported. If we want to make it a silent noop on some platforms, then\n> we can stick that into the enable_nonblock() function (which is what I\n> did, but as René showed, that is probably not a good enough solution).\n>\n> -Peff\n>\n"},{"id":"460959","messageId":"72d007c5-9429-1612-24d7-af5db906dd63@web.de","threadId":"58254","inReplyTo":"Yu/5LU+ZhbVRnSdM@coredump.intra.peff.net","subject":"[PATCH] mingw: handle writes to non-blocking pipe","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-10T05:39:40Z","receivedAt":"2022-08-10T05:48:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"write() on Windows reports ENOSPC when writing to a non-blocking pipe\nwhose buffer is full and rejects writes bigger than the buffer outright.\nChange the error code to EAGAIN and try a buffer-sized partial write to\ncomply with POSIX and the expections of our Git-internal callers.\n\nHelped-by: Jeff King \nSigned-off-by: René Scharfe \n---\n compat/mingw.c | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex b5502997e2..c6f244c0fe 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -689,6 +689,8 @@ int mingw_fflush(FILE *stream)\n \treturn ret;\n }\n\n+#define PIPE_BUFFER_SIZE (8192)\n+\n #undef write\n ssize_t mingw_write(int fd, const void *buf, size_t len)\n {\n@@ -702,6 +704,21 @@ ssize_t mingw_write(int fd, const void *buf, size_t len)\n \t\telse\n \t\t\terrno = EINVAL;\n \t}\n+\tif (result < 0 && errno == ENOSPC) {\n+\t\t/* check if fd is a non-blocking pipe */\n+\t\tHANDLE h = (HANDLE) _get_osfhandle(fd);\n+\t\tDWORD s;\n+\t\tif (GetFileType(h) == FILE_TYPE_PIPE &&\n+\t\t GetNamedPipeHandleState(h, &s, NULL, NULL, NULL, NULL, 0) &&\n+\t\t (s & PIPE_NOWAIT)) {\n+\t\t\tDWORD obuflen;\n+\t\t\tif (!GetNamedPipeInfo(h, NULL, &obuflen, NULL, NULL))\n+\t\t\t\tobuflen = PIPE_BUFFER_SIZE;\n+\t\t\tif (len > obuflen)\n+\t\t\t\treturn mingw_write(fd, buf, obuflen);\n+\t\t\terrno = EAGAIN;\n+\t\t}\n+\t}\n\n \treturn result;\n }\n@@ -1079,7 +1096,7 @@ int pipe(int filedes[2])\n \tHANDLE h[2];\n\n \t/* this creates non-inheritable handles */\n-\tif (!CreatePipe(&h[0], &h[1], NULL, 8192)) {\n+\tif (!CreatePipe(&h[0], &h[1], NULL, PIPE_BUFFER_SIZE)) {\n \t\terrno = err_win_to_posix(GetLastError());\n \t\treturn -1;\n \t}\n--\n2.37.1.windows.1\n"},{"id":"460960","messageId":"0e1b8066-3f67-cec6-675a-05d2cf54c119@web.de","threadId":"58254","inReplyTo":"Yu/5LU+ZhbVRnSdM@coredump.intra.peff.net","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-10T05:39:34Z","receivedAt":"2022-08-10T05:48:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.08.22 um 19:41 schrieb Jeff King:\n> On Sun, Aug 07, 2022 at 12:15:06PM +0200, René Scharfe wrote:\n>\n>>> This adds \"error: pumping io failed: No space left on device\" to output.\n>>> Which kinda makes sense: With the pipe no longer blocking, there can be\n>>> a moment when the buffer is full and writes have to be rejected. This\n>>> condition should be reported with EAGAIN, though.\n>>>\n>>> Adding \"if (len < 0 && errno == ENOSPC) continue;\" after the xwrite()\n>>> call in pump_io_round() lets the test pass.\n>>>\n>>> Perhaps the translation from Windows error code to POSIX is wrong here?\n>>\n>> So if we fix that with the patch below, t3701.57 still hangs, but this\n>> time it goes through wrapper.c::handle_nonblock() again and again.\n>> Replacing the \"errno = EAGAIN\" with a \"return 0\" to fake report a\n>> successful write of nothing instead lets the test pass.\n>>\n>> This seems to make sense -- looping in xwrite() won't help, as we need\n>> to read from the other fd first, to allow the process on the other end\n>> of the pipe to make some progress first, as otherwise the pipe buffer\n>> will stay full in this scenario. Shouldn't that be a problem on other\n>> systems as well?\n>\n> It doesn't happen on Linux; I suspect there's something funny either\n> about partial writes, or about poll() on Windows. What's supposed to\n> happen is:\n>\n> 1. pump_io() calls poll(), which tells us the descriptor is ready to\n> write\n>\n> 2. we call xwrite(), and our actual write() call returns a partial\n> write (i.e., reports \"ret\" bytes < \"len\" we passed in)\n>\n> 3. we return back to pump_io() do another round of poll(). If the\n> other side consumed some bytes from the pipe, then we may get\n> triggered to do another (possibly partial) write. If it didn't, and\n> we'd get EAGAIN writing, then poll shouldn't trigger at all!\n>\n> So it's weird that you'd see EAGAIN in this instance. Either the\n> underlying write() is refusing to do a partial write (and just returning\n> an error with EAGAIN in the first place), or the poll emulation is wrong\n> (telling us the descriptor is ready for writing when it isn't).\n\nYou're right, Windows' write needs two corrections. The helper below\nreports what happens when we feed a pipe with writes of different sizes.\nOn Debian on WSL 2 (Windows Subsystem for Linux) it says:\n\n chunk size: 1 bytes\n 65536 total bytes written, then got EAGAIN\n chunk size: 1000 bytes\n 64000 total bytes written, then got EAGAIN\n chunk size: 1024 bytes\n 65536 total bytes written, then got EAGAIN\n chunk size: 100000 bytes\n 0 total bytes written, then got a partial write of 65536 bytes\n 65536 total bytes written, then got EAGAIN\n\nOn Windows directly I get:\n\n chunk size: 1 bytes\n 8192 total bytes written, then got ENOSPC\n chunk size: 1000 bytes\n 8000 total bytes written, then got ENOSPC\n chunk size: 1024 bytes\n 8192 total bytes written, then got ENOSPC\n chunk size: 100000 bytes\n 0 total bytes written, then got ENOSPC\n\nhttps://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html\ndocuments what we should get: Writes smaller than the buffer should\nbe atomic, bigger writes bigger should be broken up, and the error code\nfor a full buffer should be EAGAIN. I.e. the first example is right.\nSo mingw_write() needs to translate ENOSPC to EAGAIN and break up huge\nwrites instead of giving up outright.\n\n> Can you instrument pump_io_round() (or use some strace equivalent, if\n> there is one) to see if we do a successful partial write first (which\n> implies poll() is wrong in telling us we can write more for the second\n> round), or if the very first write() is failing (which implies write()\n> is wrong for returning EAGAIN when it could do a partial write).\n\nThe two corrections mentioned above together with the enable_nonblock()\nimplementation for Windows (and the removal of \"false\") suffice to let\nt3701 pass when started directly, but it still hangs when running the\nwhole test suite using prove.\n\nI don't have time to investigate right now, but I still don't\nunderstand how xwrite() can possibly work against a non-blocking pipe.\nIt loops on EAGAIN, which is bad if the only way forward is to read\nfrom a different fd to allow the other process to drain the pipe\nbuffer so that xwrite() can write again. I suspect pump_io_round()\nmust not use xwrite() and should instead handle EAGAIN by skipping to\nthe next fd.\n\nRené\n\n---\n Makefile | 1 +\n t/helper/test-nonblock.c | 51 ++++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n 4 files changed, 54 insertions(+)\n create mode 100644 t/helper/test-nonblock.c\n\ndiff --git a/Makefile b/Makefile\nindex d9c00cc05d..0bc028ca00 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -751,6 +751,7 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o\n TEST_BUILTINS_OBJS += test-match-trees.o\n TEST_BUILTINS_OBJS += test-mergesort.o\n TEST_BUILTINS_OBJS += test-mktemp.o\n+TEST_BUILTINS_OBJS += test-nonblock.o\n TEST_BUILTINS_OBJS += test-oid-array.o\n TEST_BUILTINS_OBJS += test-oidmap.o\n TEST_BUILTINS_OBJS += test-oidtree.o\ndiff --git a/t/helper/test-nonblock.c b/t/helper/test-nonblock.c\nnew file mode 100644\nindex 0000000000..c9288ea6ac\n--- /dev/null\n+++ b/t/helper/test-nonblock.c\n@@ -0,0 +1,51 @@\n+#include \"test-tool.h\"\n+#include \"compat/nonblock.h\"\n+\n+static void fill_pipe(size_t write_len)\n+{\n+\tvoid *buf = xcalloc(1, write_len);\n+\tint fds[2];\n+\tsize_t total_written = 0;\n+\tint last = 0;\n+\n+\tif (pipe(fds))\n+\t\tdie(\"pipe failed\");\n+\tif (enable_nonblock(fds[1]))\n+\t\tdie(\"enable_nonblock failed\");\n+\n+\tprintf(\"chunk size: %\"PRIuMAX\" bytes\\n\", write_len);\n+\tfor (;;) {\n+\t\tssize_t written = write(fds[1], buf, write_len);\n+\t\tif (written != write_len)\n+\t\t\tprintf(\"%\"PRIuMAX\" total bytes written, then got \",\n+\t\t\t (uintmax_t)total_written);\n+\t\tif (written < 0) {\n+\t\t\tswitch (errno) {\n+\t\t\tcase EAGAIN: printf(\"EAGAIN\\n\"); break;\n+\t\t\tcase ENOSPC: printf(\"ENOSPC\\n\"); break;\n+\t\t\tdefault: printf(\"errno %d\\n\", errno);\n+\t\t\t}\n+\t\t\tbreak;\n+\t\t} else if (written != write_len)\n+\t\t\tprintf(\"a partial write of %\"PRIuMAX\" bytes\\n\",\n+\t\t\t (uintmax_t)written);\n+\t\tif (last)\n+\t\t\tbreak;\n+\t\tif (written > 0)\n+\t\t\ttotal_written += written;\n+\t\tlast = !written;\n+\t};\n+\n+\tclose(fds[1]);\n+\tclose(fds[0]);\n+\tfree(buf);\n+}\n+\n+int cmd__nonblock(int argc, const char **argv)\n+{\n+\tfill_pipe(1);\n+\tfill_pipe(1000);\n+\tfill_pipe(1024);\n+\tfill_pipe(100000);\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 318fdbab0c..562d7a9161 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -45,6 +45,7 @@ static struct test_cmd cmds[] = {\n \t{ \"match-trees\", cmd__match_trees },\n \t{ \"mergesort\", cmd__mergesort },\n \t{ \"mktemp\", cmd__mktemp },\n+\t{ \"nonblock\", cmd__nonblock },\n \t{ \"oid-array\", cmd__oid_array },\n \t{ \"oidmap\", cmd__oidmap },\n \t{ \"oidtree\", cmd__oidtree },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex bb79927163..d9006a5298 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -36,6 +36,7 @@ int cmd__lazy_init_name_hash(int argc, const char **argv);\n int cmd__match_trees(int argc, const char **argv);\n int cmd__mergesort(int argc, const char **argv);\n int cmd__mktemp(int argc, const char **argv);\n+int cmd__nonblock(int argc, const char **argv);\n int cmd__oidmap(int argc, const char **argv);\n int cmd__oidtree(int argc, const char **argv);\n int cmd__online_cpus(int argc, const char **argv);\n--\n2.37.1\n"},{"id":"460967","messageId":"1739sr2q-71p7-q601-2022-ps56r12pos50@tzk.qr","threadId":"58254","inReplyTo":"72d007c5-9429-1612-24d7-af5db906dd63@web.de","subject":"Re: [PATCH] mingw: handle writes to non-blocking pipe","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-10T09:07:26Z","receivedAt":"2022-08-10T09:07:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Wed, 10 Aug 2022, René Scharfe wrote:\n\n> write() on Windows reports ENOSPC when writing to a non-blocking pipe\n> whose buffer is full and rejects writes bigger than the buffer outright.\n> Change the error code to EAGAIN and try a buffer-sized partial write to\n> comply with POSIX and the expections of our Git-internal callers.\n\nExcellent analysis, thank you!\n\nHowever, let's reword this to clarify that the error code is set to EAGAIN\nonly if the buffer-sized partial write fails.\n\n>\n> Helped-by: Jeff King \n> Signed-off-by: René Scharfe \n> ---\n> compat/mingw.c | 19 ++++++++++++++++++-\n> 1 file changed, 18 insertions(+), 1 deletion(-)\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index b5502997e2..c6f244c0fe 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -689,6 +689,8 @@ int mingw_fflush(FILE *stream)\n> \treturn ret;\n> }\n>\n> +#define PIPE_BUFFER_SIZE (8192)\n\nThis constant hails all the way back from 897bb8cb2c2 (Windows: A pipe()\nreplacement whose ends are not inherited to children., 2007-12-07), in\ncase anyone wondered like I did where that number came from (and why it is\nso low).\n\nIt is outside the purview of this patch to change that constant, therefore\nI am fine with leaving this as-is.\n\n> +\n> #undef write\n> ssize_t mingw_write(int fd, const void *buf, size_t len)\n> {\n> @@ -702,6 +704,21 @@ ssize_t mingw_write(int fd, const void *buf, size_t len)\n> \t\telse\n> \t\t\terrno = EINVAL;\n> \t}\n> +\tif (result < 0 && errno == ENOSPC) {\n\nIt might make the code clearer to turn this into an `else if`.\n\n> +\t\t/* check if fd is a non-blocking pipe */\n> +\t\tHANDLE h = (HANDLE) _get_osfhandle(fd);\n> +\t\tDWORD s;\n> +\t\tif (GetFileType(h) == FILE_TYPE_PIPE &&\n> +\t\t GetNamedPipeHandleState(h, &s, NULL, NULL, NULL, NULL, 0) &&\n> +\t\t (s & PIPE_NOWAIT)) {\n> +\t\t\tDWORD obuflen;\n> +\t\t\tif (!GetNamedPipeInfo(h, NULL, &obuflen, NULL, NULL))\n> +\t\t\t\tobuflen = PIPE_BUFFER_SIZE;\n> +\t\t\tif (len > obuflen)\n> +\t\t\t\treturn mingw_write(fd, buf, obuflen);\n\nIt is probably easier to reason about to recurse instead of using a `goto`\nhere.\n\nThank you for this patch!\nDscho\n\n> +\t\t\terrno = EAGAIN;\n> +\t\t}\n> +\t}\n>\n> \treturn result;\n> }\n> @@ -1079,7 +1096,7 @@ int pipe(int filedes[2])\n> \tHANDLE h[2];\n>\n> \t/* this creates non-inheritable handles */\n> -\tif (!CreatePipe(&h[0], &h[1], NULL, 8192)) {\n> +\tif (!CreatePipe(&h[0], &h[1], NULL, PIPE_BUFFER_SIZE)) {\n> \t\terrno = err_win_to_posix(GetLastError());\n> \t\treturn -1;\n> \t}\n> --\n> 2.37.1.windows.1\n>\n"},{"id":"461022","messageId":"YvQMsMv3EhiI/rTb@coredump.intra.peff.net","threadId":"58254","inReplyTo":"0e1b8066-3f67-cec6-675a-05d2cf54c119@web.de","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-10T19:53:20Z","receivedAt":"2022-08-10T19:53:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2022 at 07:39:34AM +0200, René Scharfe wrote:\n\n> > So it's weird that you'd see EAGAIN in this instance. Either the\n> > underlying write() is refusing to do a partial write (and just returning\n> > an error with EAGAIN in the first place), or the poll emulation is wrong\n> > (telling us the descriptor is ready for writing when it isn't).\n> \n> You're right, Windows' write needs two corrections. The helper below\n> reports what happens when we feed a pipe with writes of different sizes.\n> On Debian on WSL 2 (Windows Subsystem for Linux) it says:\n> [...]\n\nThanks for digging into this further. What you found makes sense to me\nand explains what we're seeing.\n\n> The two corrections mentioned above together with the enable_nonblock()\n> implementation for Windows (and the removal of \"false\") suffice to let\n> t3701 pass when started directly, but it still hangs when running the\n> whole test suite using prove.\n\nInteresting. I wish there was an easy way for me to poke at this, too. I\ntried installing the Git for Windows SDK under wine, but unsurprisingly\nit did not get very far.\n\nPossibly I could try connecting to a running CI instance, but the test\ndid not seem to fail there! (Plus I'd have to figure out how to do\nthat... ;) ).\n\n> I don't have time to investigate right now, but I still don't\n> understand how xwrite() can possibly work against a non-blocking pipe.\n> It loops on EAGAIN, which is bad if the only way forward is to read\n> from a different fd to allow the other process to drain the pipe\n> buffer so that xwrite() can write again. I suspect pump_io_round()\n> must not use xwrite() and should instead handle EAGAIN by skipping to\n> the next fd.\n\nRight, it's susceptible to looping forever in such a case. _But_ a\nblocking write is likewise susceptible to blocking forever. In either\ncase, we're relying on the reading side to pull some bytes out of the\npipe so we can make forward progress.\n\nThe key thing is that pump_io() is careful never to initiate a write()\nunless poll() has just told us that the descriptor is ready for writing.\n\nIf something unexpected happens there (i.e., the descriptor is not\nreally ready), a blocking descriptor is going to be stuck. And with\nxwrite(), we're similarly stuck (just looping instead of blocking).\nWithout xwrite(), a non-blocking one _could_ be better off, because that\nEAGAIN would make it up to pump_io(). But what is it supposed to do? I\nguess it could go back into its main loop and hope that whatever bug\ncaused the mismatch between poll() and write() goes away.\n\nBut even that would not have fixed the problem here on Windows. From my\nunderstanding, mingw_write() in this case would never write _any_ bytes.\nSo we'd never make forward progress, and just loop writing 0 bytes and\nreturning EAGAIN over and over.\n\nSo I dunno. We could try to be a bit more defensive about non-blocking\ndescriptors by avoiding xwrite() in this instance, but it only helps for\na particular class of weird OS behavior/bugs. I'd prefer to see a real\ncase that it would help before moving in that direction.\n\n-Peff\n"},{"id":"461024","messageId":"YvQO4ZYI8/fAk0Gj@coredump.intra.peff.net","threadId":"58254","inReplyTo":"72d007c5-9429-1612-24d7-af5db906dd63@web.de","subject":"Re: [PATCH] mingw: handle writes to non-blocking pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-10T20:02:41Z","receivedAt":"2022-08-10T20:02:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2022 at 07:39:40AM +0200, René Scharfe wrote:\n\n> write() on Windows reports ENOSPC when writing to a non-blocking pipe\n> whose buffer is full and rejects writes bigger than the buffer outright.\n> Change the error code to EAGAIN and try a buffer-sized partial write to\n> comply with POSIX and the expections of our Git-internal callers.\n\nBearing in mind that I have no qualifications for reviewing\nWindows-specific patches, this seems like the right thing to be doing\nfrom the behavior you saw.\n\nOne question:\n\n> +\tif (result < 0 && errno == ENOSPC) {\n> +\t\t/* check if fd is a non-blocking pipe */\n> +\t\tHANDLE h = (HANDLE) _get_osfhandle(fd);\n> +\t\tDWORD s;\n> +\t\tif (GetFileType(h) == FILE_TYPE_PIPE &&\n> +\t\t GetNamedPipeHandleState(h, &s, NULL, NULL, NULL, NULL, 0) &&\n> +\t\t (s & PIPE_NOWAIT)) {\n> +\t\t\tDWORD obuflen;\n> +\t\t\tif (!GetNamedPipeInfo(h, NULL, &obuflen, NULL, NULL))\n> +\t\t\t\tobuflen = PIPE_BUFFER_SIZE;\n> +\t\t\tif (len > obuflen)\n> +\t\t\t\treturn mingw_write(fd, buf, obuflen);\n> +\t\t\terrno = EAGAIN;\n> +\t\t}\n\nOK, so we call GetNamedPipeInfo() to find the size of the pipe buffer.\nIt's unclear to me from Microsoft's docs if that is the _total_ size, or\nif it's the remaining available size. Hopefully the latter, since none\nof this works otherwise. ;)\n\nBut two corner cases:\n\n - If we fail to get the size, we guess that it's the maximum. Is this\n dangerous? I'm not sure why the call would fail, but if for some\n reason it did fail and we can't make forward progress, would we\n enter an infinite recursion of mingw_write()? Would it be safer to\n bail with EAGAIN in such a case (through granted, that probably just\n puts us into an infinite loop in xwrite())?\n\n - According to the docs:\n\n If the buffer size is zero, the buffer is allocated as needed.\n\n If we see this case, we'd then call mingw_write() with 0 bytes,\n which I imagine also makes no forward progress (though maybe we\n eventually return a successful 0-byte write?).\n\n-Peff\n"},{"id":"461042","messageId":"77244ffe-41c1-65bd-8984-8ed6909ffe07@web.de","threadId":"58254","inReplyTo":"YvQO4ZYI8/fAk0Gj@coredump.intra.peff.net","subject":"Re: [PATCH] mingw: handle writes to non-blocking pipe","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-10T22:34:46Z","receivedAt":"2022-08-10T22:35:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.08.2022 um 22:02 schrieb Jeff King:\n> On Wed, Aug 10, 2022 at 07:39:40AM +0200, René Scharfe wrote:\n>\n>> write() on Windows reports ENOSPC when writing to a non-blocking pipe\n>> whose buffer is full and rejects writes bigger than the buffer outright.\n>> Change the error code to EAGAIN and try a buffer-sized partial write to\n>> comply with POSIX and the expections of our Git-internal callers.\n>\n> Bearing in mind that I have no qualifications for reviewing\n> Windows-specific patches, this seems like the right thing to be doing\n> from the behavior you saw.\n>\n> One question:\n>\n>> +\tif (result < 0 && errno == ENOSPC) {\n>> +\t\t/* check if fd is a non-blocking pipe */\n>> +\t\tHANDLE h = (HANDLE) _get_osfhandle(fd);\n>> +\t\tDWORD s;\n>> +\t\tif (GetFileType(h) == FILE_TYPE_PIPE &&\n>> +\t\t GetNamedPipeHandleState(h, &s, NULL, NULL, NULL, NULL, 0) &&\n>> +\t\t (s & PIPE_NOWAIT)) {\n>> +\t\t\tDWORD obuflen;\n>> +\t\t\tif (!GetNamedPipeInfo(h, NULL, &obuflen, NULL, NULL))\n>> +\t\t\t\tobuflen = PIPE_BUFFER_SIZE;\n>> +\t\t\tif (len > obuflen)\n>> +\t\t\t\treturn mingw_write(fd, buf, obuflen);\n>> +\t\t\terrno = EAGAIN;\n>> +\t\t}\n>\n> OK, so we call GetNamedPipeInfo() to find the size of the pipe buffer.\n> It's unclear to me from Microsoft's docs if that is the _total_ size, or\n> if it's the remaining available size. Hopefully the latter, since none\n> of this works otherwise. ;)\n>\n> But two corner cases:\n>\n> - If we fail to get the size, we guess that it's the maximum. Is this\n> dangerous? I'm not sure why the call would fail, but if for some\n> reason it did fail and we can't make forward progress, would we\n> enter an infinite recursion of mingw_write()? Would it be safer to\n> bail with EAGAIN in such a case (through granted, that probably just\n> puts us into an infinite loop in xwrite())?\n\nAFAIU it's the total size, not the available space. I think I confused\nit with PIPE_BUF, which we should use instead.\n\nAlternatively we could retry with ever smaller sizes, down to one byte,\nto avoid EAGAIN as much as possible. Sounds costly, though.\n\n>\n> - According to the docs:\n>\n> If the buffer size is zero, the buffer is allocated as needed.\n>\n> If we see this case, we'd then call mingw_write() with 0 bytes,\n> which I imagine also makes no forward progress (though maybe we\n> eventually return a successful 0-byte write?).\n\nAh, yes, forgot that case.\n\nRené\n\n"},{"id":"461043","messageId":"dee6cce5-6f95-ecbe-1dd3-dd54f9746c67@web.de","threadId":"58254","inReplyTo":"YvQMsMv3EhiI/rTb@coredump.intra.peff.net","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-10T22:35:02Z","receivedAt":"2022-08-10T22:35:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.08.2022 um 21:53 schrieb Jeff King:\n> On Wed, Aug 10, 2022 at 07:39:34AM +0200, René Scharfe wrote:\n>\n>>> So it's weird that you'd see EAGAIN in this instance. Either the\n>>> underlying write() is refusing to do a partial write (and just returning\n>>> an error with EAGAIN in the first place), or the poll emulation is wrong\n>>> (telling us the descriptor is ready for writing when it isn't).\n>>\n>> You're right, Windows' write needs two corrections. The helper below\n>> reports what happens when we feed a pipe with writes of different sizes.\n>> On Debian on WSL 2 (Windows Subsystem for Linux) it says:\n>> [...]\n>\n> Thanks for digging into this further. What you found makes sense to me\n> and explains what we're seeing.\n>\n>> The two corrections mentioned above together with the enable_nonblock()\n>> implementation for Windows (and the removal of \"false\") suffice to let\n>> t3701 pass when started directly, but it still hangs when running the\n>> whole test suite using prove.\n>\n> Interesting. I wish there was an easy way for me to poke at this, too. I\n> tried installing the Git for Windows SDK under wine, but unsurprisingly\n> it did not get very far.\n>\n> Possibly I could try connecting to a running CI instance, but the test\n> did not seem to fail there! (Plus I'd have to figure out how to do\n> that... ;) ).\n>\n>> I don't have time to investigate right now, but I still don't\n>> understand how xwrite() can possibly work against a non-blocking pipe.\n>> It loops on EAGAIN, which is bad if the only way forward is to read\n>> from a different fd to allow the other process to drain the pipe\n>> buffer so that xwrite() can write again. I suspect pump_io_round()\n>> must not use xwrite() and should instead handle EAGAIN by skipping to\n>> the next fd.\n>\n> Right, it's susceptible to looping forever in such a case. _But_ a\n> blocking write is likewise susceptible to blocking forever. In either\n> case, we're relying on the reading side to pull some bytes out of the\n> pipe so we can make forward progress.\n>\n> The key thing is that pump_io() is careful never to initiate a write()\n> unless poll() has just told us that the descriptor is ready for writing.\n\nRight, and Windows breaks it by refusing to write data bigger than the\nbuffer even if it's empty.\n\nWhat does \"ready for writing\" mean? PIPE_BUF bytes are free, right?\n\n> If something unexpected happens there (i.e., the descriptor is not\n> really ready), a blocking descriptor is going to be stuck. And with\n> xwrite(), we're similarly stuck (just looping instead of blocking).\n> Without xwrite(), a non-blocking one _could_ be better off, because that\n> EAGAIN would make it up to pump_io(). But what is it supposed to do? I\n> guess it could go back into its main loop and hope that whatever bug\n> caused the mismatch between poll() and write() goes away.\n\nIt should check other fds to let the other side make some progress on\nthem, so that it eventually gets to drain the pipe we want to write to.\n\n> But even that would not have fixed the problem here on Windows. From my\n> understanding, mingw_write() in this case would never write _any_ bytes.\n> So we'd never make forward progress, and just loop writing 0 bytes and\n> returning EAGAIN over and over.\n\nRight, we need to teach it to break up large writes. It must make at\nleast some progress, if possible.\n\n> So I dunno. We could try to be a bit more defensive about non-blocking\n> descriptors by avoiding xwrite() in this instance, but it only helps for\n> a particular class of weird OS behavior/bugs. I'd prefer to see a real\n> case that it would help before moving in that direction.\n\nMakes sense.\n\nRené\n\n"},{"id":"461063","messageId":"YvTCIVN2VBir7WEP@coredump.intra.peff.net","threadId":"58254","inReplyTo":"77244ffe-41c1-65bd-8984-8ed6909ffe07@web.de","subject":"Re: [PATCH] mingw: handle writes to non-blocking pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-11T08:47:29Z","receivedAt":"2022-08-11T08:47:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 11, 2022 at 12:34:46AM +0200, René Scharfe wrote:\n\n> > OK, so we call GetNamedPipeInfo() to find the size of the pipe buffer.\n> > It's unclear to me from Microsoft's docs if that is the _total_ size, or\n> > if it's the remaining available size. Hopefully the latter, since none\n> > of this works otherwise. ;)\n> >\n> > But two corner cases:\n> >\n> > - If we fail to get the size, we guess that it's the maximum. Is this\n> > dangerous? I'm not sure why the call would fail, but if for some\n> > reason it did fail and we can't make forward progress, would we\n> > enter an infinite recursion of mingw_write()? Would it be safer to\n> > bail with EAGAIN in such a case (through granted, that probably just\n> > puts us into an infinite loop in xwrite())?\n> \n> AFAIU it's the total size, not the available space. I think I confused\n> it with PIPE_BUF, which we should use instead.\n\nHmm. If it's giving us the total size, that seems like it may fail in a\ncase like this:\n\n - we write a smaller amount to the pipe, say 7168 bytes. That leaves\n 1024 bytes in the buffer. The reader reads nothing yet.\n\n - now we try to write another 4096 bytes. That's too big, so we get\n ENOSPC. But when we ask for the pipe size, it tells us 8192. So we\n _don't_ try to do a partial write of the remaining size, and return\n EAGAIN. But now we've made no forward progress (i.e., even though\n poll() said we could write, we don't, and we end up in the xwrite\n loop).\n\nSo we really do want to try to get a partial write. Even a single byte\nmeans we are making forward progress.\n\n> Alternatively we could retry with ever smaller sizes, down to one byte,\n> to avoid EAGAIN as much as possible. Sounds costly, though.\n\nIt's definitely not optimal, but it may not be too bad. If we cut the\nsize in half each time, then at worst we make log2(N) extra write\nattempts, and we end up with a partial write within 50% of the optimal\nsize.\n\n-Peff\n"},{"id":"461064","messageId":"YvTDUWjmJKK/6c1s@coredump.intra.peff.net","threadId":"58254","inReplyTo":"dee6cce5-6f95-ecbe-1dd3-dd54f9746c67@web.de","subject":"Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-11T08:52:33Z","receivedAt":"2022-08-11T08:52:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 11, 2022 at 12:35:02AM +0200, René Scharfe wrote:\n\n> > The key thing is that pump_io() is careful never to initiate a write()\n> > unless poll() has just told us that the descriptor is ready for writing.\n> \n> Right, and Windows breaks it by refusing to write data bigger than the\n> buffer even if it's empty.\n> \n> What does \"ready for writing\" mean? PIPE_BUF bytes are free, right?\n\nAccording to POSIX, it is \"a write() will not block\". ;)\n\nI think in practice for Unix systems it means that at least 1 byte is\navailable.\n\n> > If something unexpected happens there (i.e., the descriptor is not\n> > really ready), a blocking descriptor is going to be stuck. And with\n> > xwrite(), we're similarly stuck (just looping instead of blocking).\n> > Without xwrite(), a non-blocking one _could_ be better off, because that\n> > EAGAIN would make it up to pump_io(). But what is it supposed to do? I\n> > guess it could go back into its main loop and hope that whatever bug\n> > caused the mismatch between poll() and write() goes away.\n> \n> It should check other fds to let the other side make some progress on\n> them, so that it eventually gets to drain the pipe we want to write to.\n\nYes, that \"go back into its main loop\" would do that. But that's not\nguaranteed to produce progress any time soon. We may just busy-loop\nbetween poll() and write() returning EAGAIN if another descriptor isn't\nready. And if the thing preventing further writes is something the\nprocess on the other side of the pipe can't fix (and thus we in turn\ncan't help it fix things by doing a read()), then we'll loop forever.\nWhich (if I understand it) is exactly the case here because the\nunderlying code refused to do a partial write.\n\n-Peff\n"},{"id":"461088","messageId":"976ac297-28ec-0a38-c4e1-eb7b94d0eb8c@web.de","threadId":"58254","inReplyTo":"YvTCIVN2VBir7WEP@coredump.intra.peff.net","subject":"Re: [PATCH] mingw: handle writes to non-blocking pipe","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-11T17:35:33Z","receivedAt":"2022-08-11T17:36:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.08.2022 um 10:47 schrieb Jeff King:\n> On Thu, Aug 11, 2022 at 12:34:46AM +0200, René Scharfe wrote:\n>\n>>> OK, so we call GetNamedPipeInfo() to find the size of the pipe buffer.\n>>> It's unclear to me from Microsoft's docs if that is the _total_ size, or\n>>> if it's the remaining available size. Hopefully the latter, since none\n>>> of this works otherwise. ;)\n>>>\n>>> But two corner cases:\n>>>\n>>> - If we fail to get the size, we guess that it's the maximum. Is this\n>>> dangerous? I'm not sure why the call would fail, but if for some\n>>> reason it did fail and we can't make forward progress, would we\n>>> enter an infinite recursion of mingw_write()? Would it be safer to\n>>> bail with EAGAIN in such a case (through granted, that probably just\n>>> puts us into an infinite loop in xwrite())?\n>>\n>> AFAIU it's the total size, not the available space. I think I confused\n>> it with PIPE_BUF, which we should use instead.\n>\n> Hmm. If it's giving us the total size, that seems like it may fail in a\n> case like this:\n>\n> - we write a smaller amount to the pipe, say 7168 bytes. That leaves\n> 1024 bytes in the buffer. The reader reads nothing yet.\n>\n> - now we try to write another 4096 bytes. That's too big, so we get\n> ENOSPC. But when we ask for the pipe size, it tells us 8192. So we\n> _don't_ try to do a partial write of the remaining size, and return\n> EAGAIN. But now we've made no forward progress (i.e., even though\n> poll() said we could write, we don't, and we end up in the xwrite\n> loop).\n>\n> So we really do want to try to get a partial write. Even a single byte\n> means we are making forward progress.\n>\n>> Alternatively we could retry with ever smaller sizes, down to one byte,\n>> to avoid EAGAIN as much as possible. Sounds costly, though.\n>\n> It's definitely not optimal, but it may not be too bad. If we cut the\n> size in half each time, then at worst we make log2(N) extra write\n> attempts, and we end up with a partial write within 50% of the optimal\n> size.\n\nOK, but we can't just split anything up as we like: POSIX wants us to\nkeep writes up to PIPE_BUF atomic. When I read that name I mistakenly\nthought it was the size of the pipe buffer, but it's a different value.\nThe minimum value according to POSIX is 512 bytes; on Linux it's 4KB.\n\nAnd Windows doesn't seem to define it. Bash's ulimit -p returns 8,\nwhich is in units of 512 bytes, so it's 4KB like on Linux. But that's\napparently not respected by Windows' write.\n\nI just realized that we can interprete the situation slightly\ndifferently. Windows has effectively PIPE_BUF = 2^32, which means all\nwrites are atomic. I don't see POSIX specifying a maximum allowed\nvalue, so this must be allowed. Which means you cannot rely on write\nperforming partial writes. Makes sense?\n\nIf we accept that, then we need a special write function for\nnon-blocking pipes that chops the data into small enough chunks.\n\nAnother oddity: t3701 with yesterday's patch finishes fine with -i, but\nhangs without it (not just when running it via prove). O_o\n\nRené\n"},{"id":"461094","messageId":"YvVIYyA8Js0WDAMc@coredump.intra.peff.net","threadId":"58254","inReplyTo":"976ac297-28ec-0a38-c4e1-eb7b94d0eb8c@web.de","subject":"Re: [PATCH] mingw: handle writes to non-blocking pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-11T18:20:19Z","receivedAt":"2022-08-11T18:20:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 11, 2022 at 07:35:33PM +0200, René Scharfe wrote:\n\n> OK, but we can't just split anything up as we like: POSIX wants us to\n> keep writes up to PIPE_BUF atomic. When I read that name I mistakenly\n> thought it was the size of the pipe buffer, but it's a different value.\n> The minimum value according to POSIX is 512 bytes; on Linux it's 4KB.\n> \n> And Windows doesn't seem to define it. Bash's ulimit -p returns 8,\n> which is in units of 512 bytes, so it's 4KB like on Linux. But that's\n> apparently not respected by Windows' write.\n> \n> I just realized that we can interprete the situation slightly\n> differently. Windows has effectively PIPE_BUF = 2^32, which means all\n> writes are atomic. I don't see POSIX specifying a maximum allowed\n> value, so this must be allowed. Which means you cannot rely on write\n> performing partial writes. Makes sense?\n\nHmm, I'm not sure. POSIX does allow writing a single byte if that's all\nthe space there is, but only if the original _request_ was for more than\nPIPE_BUF. Which I guess matches what you're saying.\n\nIf the original request is smaller than PIPE_BUF, it does seem to say\nthat EAGAIN is the correct output. But it also says:\n\n There is no exception regarding partial writes when O_NONBLOCK is set.\n With the exception of writing to an empty pipe, this volume of\n POSIX.1-2017 does not specify exactly when a partial write is\n performed since that would require specifying internal details of the\n implementation. Every application should be prepared to handle partial\n writes when O_NONBLOCK is set and the requested amount is greater than\n {PIPE_BUF}, just as every application should be prepared to handle\n partial writes on other kinds of file descriptors.\n\n The intent of forcing writing at least one byte if any can be written\n is to assure that each write makes progress if there is any room in\n the pipe. If the pipe is empty, {PIPE_BUF} bytes must be written; if\n not, at least some progress must have been made.\n\nSo they are clearly aware of the \"we need to make forward progress\"\nproblem. But how are you supposed to do that if you only have less than\nPIPE_BUF bytes left to write? And likewise, even if it is technically\nlegal, having a PIPE_BUF of 2^32 seems like a quality-of-implementation\nissue.\n\nAnd that doesn't really match what poll() is saying. The response from\npoll() told us we could write without blocking, which implies at least\nPIPE_BUF bytes available. But clearly it isn't available, since the\nwrite fails (with EAGAIN, but that is equivalent to blocking in POSIX's\nview here).\n\nLawyering aside, I think it would be OK to move forward with cutting up\nwrites at least to a size of 512 bytes. Git runs on lots of platforms,\nand none of the code can make an assumption that PIPE_BUF is larger than\nthat. I.e., we are reducing atomicity provided by Windows, but that's\nOK.\n\nI don't think that solves our problem fully, though. We might need to\nwrite 5 bytes, and telling mingw_write() to do so means it's supposed to\nabide by PIPE_BUF conventions. But again, we are in control of the\ncalling code here. I don't think there's any reason that we care about\nPIPE_BUF atomicity. Certainly we don't get those atomicity guarantees on\nthe socket side, which is where much of our writing happens, and our\ncode is prepared to handle partial writes of any size. So we could just\nignore that guarantee here.\n\n> If we accept that, then we need a special write function for\n> non-blocking pipes that chops the data into small enough chunks.\n\nI'm not sure what \"small enough\" we can rely on, though. Really it is\nthe interplay between poll() and write() that we care about here. We\nwould like to know at what point poll() will tell us it's OK to write().\nBut we don't know what the OS thinks of that.\n\nOr maybe we do, since I think poll() is our own compat layer. But it\njust seems to be based on select(). We do seem to use PeekNamedPipe()\nthere to look on the reading side, but I don't know if there's an\nequivalent for writing.\n\n> Another oddity: t3701 with yesterday's patch finishes fine with -i, but\n> hangs without it (not just when running it via prove). O_o\n\nYes, definitely strange. I'd expect weirdness with \"-v\", for example,\nbecause of terminal stuff, but \"-i\" should have no impact on the running\nenvironment.\n\n-Peff\n"},{"id":"461191","messageId":"c7c6524c-4f02-10f6-1a58-738cef5aecf2@web.de","threadId":"58254","inReplyTo":"YvVIYyA8Js0WDAMc@coredump.intra.peff.net","subject":"Re: [PATCH] mingw: handle writes to non-blocking pipe","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-14T15:37:08Z","receivedAt":"2022-08-14T15:57:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.08.2022 um 20:20 schrieb Jeff King:\n> On Thu, Aug 11, 2022 at 07:35:33PM +0200, René Scharfe wrote:\n>\n>> OK, but we can't just split anything up as we like: POSIX wants us to\n>> keep writes up to PIPE_BUF atomic. When I read that name I mistakenly\n>> thought it was the size of the pipe buffer, but it's a different value.\n>> The minimum value according to POSIX is 512 bytes; on Linux it's 4KB.\n>>\n>> And Windows doesn't seem to define it. Bash's ulimit -p returns 8,\n>> which is in units of 512 bytes, so it's 4KB like on Linux. But that's\n>> apparently not respected by Windows' write.\n>>\n>> I just realized that we can interprete the situation slightly\n>> differently. Windows has effectively PIPE_BUF = 2^32, which means all\n>> writes are atomic. I don't see POSIX specifying a maximum allowed\n>> value, so this must be allowed. Which means you cannot rely on write\n>> performing partial writes. Makes sense?\n>\n> Hmm, I'm not sure. POSIX does allow writing a single byte if that's all\n> the space there is, but only if the original _request_ was for more than\n> PIPE_BUF. Which I guess matches what you're saying.\n>\n> If the original request is smaller than PIPE_BUF, it does seem to say\n> that EAGAIN is the correct output. But it also says:\n>\n> There is no exception regarding partial writes when O_NONBLOCK is set.\n> With the exception of writing to an empty pipe, this volume of\n> POSIX.1-2017 does not specify exactly when a partial write is\n> performed since that would require specifying internal details of the\n> implementation. Every application should be prepared to handle partial\n> writes when O_NONBLOCK is set and the requested amount is greater than\n> {PIPE_BUF}, just as every application should be prepared to handle\n> partial writes on other kinds of file descriptors.\n>\n> The intent of forcing writing at least one byte if any can be written\n> is to assure that each write makes progress if there is any room in\n> the pipe. If the pipe is empty, {PIPE_BUF} bytes must be written; if\n> not, at least some progress must have been made.\n>\n> So they are clearly aware of the \"we need to make forward progress\"\n> problem. But how are you supposed to do that if you only have less than\n> PIPE_BUF bytes left to write? And likewise, even if it is technically\n> legal, having a PIPE_BUF of 2^32 seems like a quality-of-implementation\n> issue.\n>\n> And that doesn't really match what poll() is saying. The response from\n> poll() told us we could write without blocking, which implies at least\n> PIPE_BUF bytes available. But clearly it isn't available, since the\n> write fails (with EAGAIN, but that is equivalent to blocking in POSIX's\n> view here).\n\nTurns out that's not the case on Windows: 94f4d01932 (mingw: workaround\nfor hangs when sending STDIN, 2020-02-17) changed the compatibility\nimplementation to 'Make `poll()` always reply \"writable\" for write end\nof the pipe.'.\n\nAn updated version of the test helper confirms it (patch below) by\nreporting on Windows:\n\nchunk size: 1 bytes\n EAGAIN after 8192 bytes\nchunk size: 500 bytes\n EAGAIN after 8000 bytes\nchunk size: 1000 bytes\n EAGAIN after 8000 bytes\nchunk size: 1024 bytes\n EAGAIN after 8192 bytes\nchunk size: 100000 bytes\n partial write of 8192 bytes after 0 bytes\n EAGAIN after 8192 bytes\n\nOn Debian I get this instead:\n\nchunk size: 1 bytes\n POLLOUT gone after 61441 bytes\n EAGAIN after 65536 bytes\nchunk size: 500 bytes\n POLLOUT gone after 60500 bytes\n EAGAIN after 64000 bytes\nchunk size: 1000 bytes\n POLLOUT gone after 61000 bytes\n EAGAIN after 64000 bytes\nchunk size: 1024 bytes\n POLLOUT gone after 62464 bytes\n EAGAIN after 65536 bytes\nchunk size: 100000 bytes\n partial write of 65536 bytes after 0 bytes\n POLLOUT gone after 65536 bytes\n EAGAIN after 65536 bytes\n\nSo on Windows the POLLOUT bit is indeed never unset.\n\n> Lawyering aside, I think it would be OK to move forward with cutting up\n> writes at least to a size of 512 bytes. Git runs on lots of platforms,\n> and none of the code can make an assumption that PIPE_BUF is larger than\n> that. I.e., we are reducing atomicity provided by Windows, but that's\n> OK.\n>\n> I don't think that solves our problem fully, though. We might need to\n> write 5 bytes, and telling mingw_write() to do so means it's supposed to\n> abide by PIPE_BUF conventions. But again, we are in control of the\n> calling code here. I don't think there's any reason that we care about\n> PIPE_BUF atomicity. Certainly we don't get those atomicity guarantees on\n> the socket side, which is where much of our writing happens, and our\n> code is prepared to handle partial writes of any size. So we could just\n> ignore that guarantee here.\n>\n>> If we accept that, then we need a special write function for\n>> non-blocking pipes that chops the data into small enough chunks.\n>\n> I'm not sure what \"small enough\" we can rely on, though. Really it is\n> the interplay between poll() and write() that we care about here. We\n> would like to know at what point poll() will tell us it's OK to write().\n> But we don't know what the OS thinks of that.\n\nBased on the output above I think Linux' poll() won't consider a pipe\nwritable that has less than PIPE_BUF (4096) available bytes.\n\n> Or maybe we do, since I think poll() is our own compat layer. But it\n> just seems to be based on select(). We do seem to use PeekNamedPipe()\n> there to look on the reading side, but I don't know if there's an\n> equivalent for writing.\n\nThis has been elusive so far (see 94f4d01932).\n\nPerhaps we should take the advice about PIPE_NOWAIT in the docs serious\nand use overlapping (asynchronous) writes on Windows instead. This\nwould mean reimplementing the whole pipe_command() with Windows API\ncommands, I imagine.\n\n>> Another oddity: t3701 with yesterday's patch finishes fine with -i, but\n>> hangs without it (not just when running it via prove). O_o\n>\n> Yes, definitely strange. I'd expect weirdness with \"-v\", for example,\n> because of terminal stuff, but \"-i\" should have no impact on the running\n> environment.\nIt's especially grating because test runs also take very looong.\n\nAvoiding xwrite() in pump_io_round() on top lets the test suite\nfinish successfully.\n\nRené\n\n\n---\n Makefile | 1 +\n t/helper/test-nonblock.c | 73 ++++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n 4 files changed, 76 insertions(+)\n create mode 100644 t/helper/test-nonblock.c\n\ndiff --git a/Makefile b/Makefile\nindex d9c00cc05d..0bc028ca00 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -751,6 +751,7 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o\n TEST_BUILTINS_OBJS += test-match-trees.o\n TEST_BUILTINS_OBJS += test-mergesort.o\n TEST_BUILTINS_OBJS += test-mktemp.o\n+TEST_BUILTINS_OBJS += test-nonblock.o\n TEST_BUILTINS_OBJS += test-oid-array.o\n TEST_BUILTINS_OBJS += test-oidmap.o\n TEST_BUILTINS_OBJS += test-oidtree.o\ndiff --git a/t/helper/test-nonblock.c b/t/helper/test-nonblock.c\nnew file mode 100644\nindex 0000000000..c9350ce894\n--- /dev/null\n+++ b/t/helper/test-nonblock.c\n@@ -0,0 +1,73 @@\n+#include \"test-tool.h\"\n+#include \"compat/nonblock.h\"\n+\n+static void fill_pipe(size_t write_len)\n+{\n+\tvoid *buf = xcalloc(1, write_len);\n+\tint fds[2];\n+\tsize_t total_written = 0;\n+\tint last = 0;\n+\tstruct pollfd pfd;\n+\tint writable = 1;\n+\n+\tif (pipe(fds))\n+\t\tdie_errno(\"pipe failed\");\n+\tif (enable_nonblock(fds[1]))\n+\t\tdie_errno(\"enable_nonblock failed\");\n+\tpfd.fd = fds[1];\n+\tpfd.events = POLLOUT;\n+\n+\tprintf(\"chunk size: %\"PRIuMAX\" bytes\\n\", write_len);\n+\tfor (;;) {\n+\t\tssize_t written;\n+\t\tif (poll(&pfd, 1, 0) < 0) {\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tdie_errno(\"poll failed\");\n+\t\t}\n+\t\tif (writable && !(pfd.revents & POLLOUT)) {\n+\t\t\twritable = 0;\n+\t\t\tprintf(\" POLLOUT gone after %\"PRIuMAX\" bytes\\n\",\n+\t\t\t total_written);\n+\t\t}\n+\t\twritten = write(fds[1], buf, write_len);\n+\t\tif (written < 0) {\n+\t\t\tswitch (errno) {\n+\t\t\tcase EAGAIN:\n+\t\t\t\tprintf(\" EAGAIN after %\"PRIuMAX\" bytes\\n\",\n+\t\t\t\t total_written);\n+\t\t\t\tbreak;\n+\t\t\tcase ENOSPC:\n+\t\t\t\tprintf(\" ENOSPC after %\"PRIuMAX\" bytes\\n\",\n+\t\t\t\t total_written);\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tprintf(\" errno %d after %\"PRIuMAX\" bytes\\n\",\n+\t\t\t\t errno, total_written);\n+\t\t\t}\n+\t\t\tbreak;\n+\t\t} else if (written != write_len)\n+\t\t\tprintf(\" partial write of %\"PRIuMAX\" bytes\"\n+\t\t\t \" after %\"PRIuMAX\" bytes\\n\",\n+\t\t\t (uintmax_t)written, total_written);\n+\t\tif (last)\n+\t\t\tbreak;\n+\t\tif (written > 0)\n+\t\t\ttotal_written += written;\n+\t\tlast = !written;\n+\t};\n+\n+\tclose(fds[1]);\n+\tclose(fds[0]);\n+\tfree(buf);\n+}\n+\n+int cmd__nonblock(int argc, const char **argv)\n+{\n+\tfill_pipe(1);\n+\tfill_pipe(500);\n+\tfill_pipe(1000);\n+\tfill_pipe(1024);\n+\tfill_pipe(100000);\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 318fdbab0c..562d7a9161 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -45,6 +45,7 @@ static struct test_cmd cmds[] = {\n \t{ \"match-trees\", cmd__match_trees },\n \t{ \"mergesort\", cmd__mergesort },\n \t{ \"mktemp\", cmd__mktemp },\n+\t{ \"nonblock\", cmd__nonblock },\n \t{ \"oid-array\", cmd__oid_array },\n \t{ \"oidmap\", cmd__oidmap },\n \t{ \"oidtree\", cmd__oidtree },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex bb79927163..d9006a5298 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -36,6 +36,7 @@ int cmd__lazy_init_name_hash(int argc, const char **argv);\n int cmd__match_trees(int argc, const char **argv);\n int cmd__mergesort(int argc, const char **argv);\n int cmd__mktemp(int argc, const char **argv);\n+int cmd__nonblock(int argc, const char **argv);\n int cmd__oidmap(int argc, const char **argv);\n int cmd__oidtree(int argc, const char **argv);\n int cmd__online_cpus(int argc, const char **argv);\n--\n2.37.1.windows.1\n"},{"id":"461339","messageId":"Yvx/FNlTx+DCiCOj@coredump.intra.peff.net","threadId":"58254","inReplyTo":"c7c6524c-4f02-10f6-1a58-738cef5aecf2@web.de","subject":"Re: [PATCH] mingw: handle writes to non-blocking pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-17T05:39:32Z","receivedAt":"2022-08-17T05:39:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 14, 2022 at 05:37:08PM +0200, René Scharfe wrote:\n\n> Turns out that's not the case on Windows: 94f4d01932 (mingw: workaround\n> for hangs when sending STDIN, 2020-02-17) changed the compatibility\n> implementation to 'Make `poll()` always reply \"writable\" for write end\n> of the pipe.'.\n\nAh, good find. That kind of explains everything, then, I think. ;)\n\n> > I'm not sure what \"small enough\" we can rely on, though. Really it is\n> > the interplay between poll() and write() that we care about here. We\n> > would like to know at what point poll() will tell us it's OK to write().\n> > But we don't know what the OS thinks of that.\n> \n> Based on the output above I think Linux' poll() won't consider a pipe\n> writable that has less than PIPE_BUF (4096) available bytes.\n\nRight, that makes sense. It would have to in order to meet the atomicity\nrequirement for write(), but still always make forward progress for each\nwrite().\n\n> Perhaps we should take the advice about PIPE_NOWAIT in the docs serious\n> and use overlapping (asynchronous) writes on Windows instead. This\n> would mean reimplementing the whole pipe_command() with Windows API\n> commands, I imagine.\n\nI wouldn't be opposed to that, in the sense that it's supposed to be a\nblack box to the caller, and it's relatively small in size. But I think\nwe're pretty close to having something usable without that, so I'd like\nto pursue a smaller fix in the interim.\n\n> Avoiding xwrite() in pump_io_round() on top lets the test suite\n> finish successfully.\n\nThat makes sense. We end up busy-looping between poll() and write()\nwhile we wait for our read descriptor to become available. But if poll()\ndoesn't block, that's the best we can do.\n\n-Peff\n"},{"id":"461340","messageId":"YvyE1RDFVAg4uQo1@coredump.intra.peff.net","threadId":"58254","inReplyTo":"Yvx/FNlTx+DCiCOj@coredump.intra.peff.net","subject":"[PATCH v2 0/6] fix pipe_command() deadlock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-17T06:04:05Z","receivedAt":"2022-08-17T06:04:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 17, 2022 at 01:39:32AM -0400, Jeff King wrote:\n\n> I wouldn't be opposed to that, in the sense that it's supposed to be a\n> black box to the caller, and it's relatively small in size. But I think\n> we're pretty close to having something usable without that, so I'd like\n> to pursue a smaller fix in the interim.\n\nSo here's what I came up with. Most of the changes are done as\npreparatory steps, so I believe the final commit should just work out of\nthe box for you, as well as on Windows CI (which I just kicked off).\n\nHere's a breakdown of the patches:\n\n [1/6]: compat: add function to enable nonblocking pipes\n [2/6]: nonblock: support Windows\n\nThis is my compat wrapper from v1 pulled into a separate function, and\nthen your Windows implementation on top.\n\n [3/6]: git-compat-util: make MAX_IO_SIZE define globally available\n [4/6]: pipe_command(): avoid xwrite() for writing to pipe\n\nThis fixes the \"loop forever\" problem of using xwrite(), under the\nassumption that fixing our Windows poll() emulation is too hard. If we\ndid fix it, then we could revert these (though keeping them can be\nthought of as a defensive measure against future bugs; busy-looping is\nless bad than deadlocking).\n\nI _think_ the amount of busy-looping on Windows will be OK here. Any\ncase that wouldn't deadlock currently is unaffected, and the cases that\nare will probably not be too wasteful. More discussion in the commit\nmessage.\n\n [5/6]: pipe_command(): handle ENOSPC when writing to a pipe\n\nA hack around the ENOSPC problem you found. We could replace this with\nyour mingw_write() patch, but it looked like that needed some corner\ncases to be smoothed a bit.\n\n [6/6]: pipe_command(): mark stdin descriptor as non-blocking\n\nThe actual fix, largely as before.\n\nI don't love patches 3-5, but it seems like a practical path forward.\nI'd prefer to go with a minimal fix that removes the existing deadlock,\nand then we can do more cleanups on top. Thanks so much for all of your\ninvestigation and testing so far!\n\n Makefile | 1 +\n compat/nonblock.c | 50 ++++++++++++++++++++++++++++++++++++++\n compat/nonblock.h | 9 +++++++\n git-compat-util.h | 22 +++++++++++++++++\n run-command.c | 33 +++++++++++++++++++++----\n t/t3701-add-interactive.sh | 13 ++++++++++\n wrapper.c | 22 -----------------\n 7 files changed, 123 insertions(+), 27 deletions(-)\n create mode 100644 compat/nonblock.c\n create mode 100644 compat/nonblock.h\n\n-Peff\n"},{"id":"461341","messageId":"YvyFBzdO8PN7Ou0W@coredump.intra.peff.net","threadId":"58254","inReplyTo":"YvyE1RDFVAg4uQo1@coredump.intra.peff.net","subject":"[PATCH v2 1/6] compat: add function to enable nonblocking pipes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-17T06:04:55Z","receivedAt":"2022-08-17T06:05:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We'd like to be able to make some of our pipes nonblocking so that\npoll() can be used effectively, but O_NONBLOCK isn't portable. Let's\nintroduce a compat wrapper so this can be abstracted for each platform.\n\nThe interface is as narrow as possible to let platforms do what's\nnatural there (rather than having to implement fcntl() and a fake\nO_NONBLOCK for example, or having to handle other types of descriptors).\n\nThe next commit will add Windows support, at which point we should be\ncovering all platforms in practice. But if we do find some other\nplatform without O_NONBLOCK, we'll return ENOSYS. Arguably we could just\ntrigger a build-time #error in this case, which would catch the problem\nearlier. But since we're not planning to use this compat wrapper in many\ncode paths, a seldom-seen runtime error may be friendlier for such a\nplatform than blocking compilation completely. Our test suite would\nstill notice it.\n\nSigned-off-by: Jeff King \n---\n Makefile | 1 +\n compat/nonblock.c | 23 +++++++++++++++++++++++\n compat/nonblock.h | 9 +++++++++\n 3 files changed, 33 insertions(+)\n create mode 100644 compat/nonblock.c\n create mode 100644 compat/nonblock.h\n\ndiff --git a/Makefile b/Makefile\nindex e8adeb09f1..224e193b66 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -918,6 +918,7 @@ LIB_OBJS += combine-diff.o\n LIB_OBJS += commit-graph.o\n LIB_OBJS += commit-reach.o\n LIB_OBJS += commit.o\n+LIB_OBJS += compat/nonblock.o\n LIB_OBJS += compat/obstack.o\n LIB_OBJS += compat/terminal.o\n LIB_OBJS += compat/zlib-uncompress2.o\ndiff --git a/compat/nonblock.c b/compat/nonblock.c\nnew file mode 100644\nindex 0000000000..b08105a21d\n--- /dev/null\n+++ b/compat/nonblock.c\n@@ -0,0 +1,23 @@\n+#include \"git-compat-util.h\"\n+#include \"nonblock.h\"\n+\n+#ifdef O_NONBLOCK\n+\n+int enable_pipe_nonblock(int fd)\n+{\n+\tint flags = fcntl(fd, F_GETFL);\n+\tif (flags < 0)\n+\t\treturn -1;\n+\tflags |= O_NONBLOCK;\n+\treturn fcntl(fd, F_SETFL, flags);\n+}\n+\n+#else\n+\n+int enable_pipe_nonblock(int fd)\n+{\n+\terrno = ENOSYS;\n+\treturn -1;\n+}\n+\n+#endif\ndiff --git a/compat/nonblock.h b/compat/nonblock.h\nnew file mode 100644\nindex 0000000000..af1a331301\n--- /dev/null\n+++ b/compat/nonblock.h\n@@ -0,0 +1,9 @@\n+#ifndef COMPAT_NONBLOCK_H\n+#define COMPAT_NONBLOCK_H\n+\n+/*\n+ * Enable non-blocking I/O for the pipe specified by the passed-in descriptor.\n+ */\n+int enable_pipe_nonblock(int fd);\n+\n+#endif\n-- \n2.37.2.881.gb57357660c\n\n"},{"id":"461342","messageId":"YvyFJcqyBGXMtoxZ@coredump.intra.peff.net","threadId":"58254","inReplyTo":"YvyE1RDFVAg4uQo1@coredump.intra.peff.net","subject":"[PATCH v2 2/6] nonblock: support Windows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-17T06:05:25Z","receivedAt":"2022-08-17T06:05:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: René Scharfe \n\nImplement enable_pipe_nonblock() using the Windows API. This works only\nfor pipes, but that is sufficient for this limited interface. Despite\nthe API calls used, it handles both \"named\" and anonymous pipes from our\npipe() emulation.\n\nSigned-off-by: René Scharfe \nSigned-off-by: Jeff King \n---\n compat/nonblock.c | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/compat/nonblock.c b/compat/nonblock.c\nindex b08105a21d..9694ebdb1d 100644\n--- a/compat/nonblock.c\n+++ b/compat/nonblock.c\n@@ -12,6 +12,33 @@ int enable_pipe_nonblock(int fd)\n \treturn fcntl(fd, F_SETFL, flags);\n }\n \n+#elif defined(GIT_WINDOWS_NATIVE)\n+\n+#include \"win32.h\"\n+\n+int enable_pipe_nonblock(int fd)\n+{\n+\tHANDLE h = (HANDLE)_get_osfhandle(fd);\n+\tDWORD mode;\n+\tDWORD type = GetFileType(h);\n+\tif (type == FILE_TYPE_UNKNOWN && GetLastError() != NO_ERROR) {\n+\t\terrno = EBADF;\n+\t\treturn -1;\n+\t}\n+\tif (type != FILE_TYPE_PIPE)\n+\t\tBUG(\"unsupported file type: %lu\", type);\n+\tif (!GetNamedPipeHandleState(h, &mode, NULL, NULL, NULL, NULL, 0)) {\n+\t\terrno = err_win_to_posix(GetLastError());\n+\t\treturn -1;\n+\t}\n+\tmode |= PIPE_NOWAIT;\n+\tif (!SetNamedPipeHandleState(h, &mode, NULL, NULL)) {\n+\t\terrno = err_win_to_posix(GetLastError());\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n+}\n+\n #else\n \n int enable_pipe_nonblock(int fd)\n-- \n2.37.2.881.gb57357660c\n\n"},{"id":"461343","messageId":"YvyFggmzXAGf3jlL@coredump.intra.peff.net","threadId":"58254","inReplyTo":"YvyE1RDFVAg4uQo1@coredump.intra.peff.net","subject":"[PATCH v2 3/6] git-compat-util: make MAX_IO_SIZE define globally available","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-17T06:06:58Z","receivedAt":"2022-08-17T06:07:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We define MAX_IO_SIZE within wrapper.c, but it's useful for any code\nthat wants to do a raw write() for whatever reason (say, because they\nwant different EAGAIN handling). Let's make it available everywhere.\n\nThe alternative would be adding xwrite_foo() variants to give callers\nmore options. But there's really no reason MAX_IO_SIZE needs to be\nabstracted away, so this give callers the most flexibility.\n\nSigned-off-by: Jeff King \n---\nI did actually write xwrite_nonblock() at first, and I could be\npersuaded to go in that direction if we prefer. But given that using it\nsanely requires the caller to have a poll() loop, I feel like\npipe_command() is already the right level of abstraction.\n\n git-compat-util.h | 22 ++++++++++++++++++++++\n wrapper.c | 22 ----------------------\n 2 files changed, 22 insertions(+), 22 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 36a25ae252..6aee4d92e7 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -998,6 +998,28 @@ static inline unsigned long cast_size_t_to_ulong(size_t a)\n \treturn (unsigned long)a;\n }\n \n+/*\n+ * Limit size of IO chunks, because huge chunks only cause pain. OS X\n+ * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in\n+ * the absence of bugs, large chunks can result in bad latencies when\n+ * you decide to kill the process.\n+ *\n+ * We pick 8 MiB as our default, but if the platform defines SSIZE_MAX\n+ * that is smaller than that, clip it to SSIZE_MAX, as a call to\n+ * read(2) or write(2) larger than that is allowed to fail. As the last\n+ * resort, we allow a port to pass via CFLAGS e.g. \"-DMAX_IO_SIZE=value\"\n+ * to override this, if the definition of SSIZE_MAX given by the platform\n+ * is broken.\n+ */\n+#ifndef MAX_IO_SIZE\n+# define MAX_IO_SIZE_DEFAULT (8*1024*1024)\n+# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)\n+# define MAX_IO_SIZE SSIZE_MAX\n+# else\n+# define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT\n+# endif\n+#endif\n+\n #ifdef HAVE_ALLOCA_H\n # include \n # define xalloca(size) (alloca(size))\ndiff --git a/wrapper.c b/wrapper.c\nindex cfe79bd081..299d6489a6 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -161,28 +161,6 @@ void xsetenv(const char *name, const char *value, int overwrite)\n \t\tdie_errno(_(\"could not setenv '%s'\"), name ? name : \"(null)\");\n }\n \n-/*\n- * Limit size of IO chunks, because huge chunks only cause pain. OS X\n- * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in\n- * the absence of bugs, large chunks can result in bad latencies when\n- * you decide to kill the process.\n- *\n- * We pick 8 MiB as our default, but if the platform defines SSIZE_MAX\n- * that is smaller than that, clip it to SSIZE_MAX, as a call to\n- * read(2) or write(2) larger than that is allowed to fail. As the last\n- * resort, we allow a port to pass via CFLAGS e.g. \"-DMAX_IO_SIZE=value\"\n- * to override this, if the definition of SSIZE_MAX given by the platform\n- * is broken.\n- */\n-#ifndef MAX_IO_SIZE\n-# define MAX_IO_SIZE_DEFAULT (8*1024*1024)\n-# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)\n-# define MAX_IO_SIZE SSIZE_MAX\n-# else\n-# define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT\n-# endif\n-#endif\n-\n /**\n * xopen() is the same as open(), but it die()s if the open() fails.\n */\n-- \n2.37.2.881.gb57357660c\n\n"},{"id":"461344","messageId":"YvyFxry9+2x05Di3@coredump.intra.peff.net","threadId":"58254","inReplyTo":"YvyE1RDFVAg4uQo1@coredump.intra.peff.net","subject":"[PATCH v2 4/6] pipe_command(): avoid xwrite() for writing to pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-17T06:08:06Z","receivedAt":"2022-08-17T06:08:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If xwrite() sees an EAGAIN response, it will loop forever until the\nwrite succeeds (or encounters a real error). This is due to ef1cf0167a\n(xwrite: poll on non-blocking FDs, 2016-06-26), with the idea that we\nwon't be surprised by a descriptor unexpectedly set as non-blocking.\n\nBut that will make things awkward when we do want a non-blocking\ndescriptor, and a future patch will switch pipe_command() to using one.\nIn that case, looping on EAGAIN is bad, because the process on the other\nend of the pipe may be waiting on us before doing another read() on the\npipe, which would mean we deadlock.\n\nIn practice we're not supposed to ever see EAGAIN here, since poll()\nwill have just told us the descriptor is ready for writing. But our\nWindows emulation of poll() will always return \"ready\" for writing to a\npipe descriptor! This is due to 94f4d01932 (mingw: workaround for hangs\nwhen sending STDIN, 2020-02-17).\n\nOur best bet in that case is to keep handling other descriptors, as any\nread() we do may allow the child command to make forward progress (i.e.,\nits write() finishes, and then it read()s from its stdin, freeing up\nspace in the pipe buffer). This means we might busy-loop between poll()\nand write() on Windows if the child command is slow to read our input,\nbut it's much better than the alternative of deadlocking.\n\nIn practice, this busy-looping should be rare:\n\n - for small inputs, we'll just write the whole thing in a single\n write() anyway, non-blocking or not\n\n - for larger inputs where the child reads input and then processes it\n before writing (e.g., gpg verifying a signature), we may make a few\n extra write() calls that get EAGAIN during the initial write, but\n once it has taken in the whole input, we'll correctly block waiting\n to read back the data.\n\n - for larger inputs where the child process is streaming output back\n (like a diff filter), we'll likewise see some extra EAGAINs, but\n most of them will be followed immediately by a read(), which will\n let the child command make forward progress.\n\nOf course it won't happen at all for now, since we don't yet use a\nnon-blocking pipe. This is just preparation for when we do.\n\nHelped-by: René Scharfe \nSigned-off-by: Jeff King \n---\n run-command.c | 22 +++++++++++++++++-----\n 1 file changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 14f17830f5..e078c3046f 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1364,12 +1364,24 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)\n \t\t\tcontinue;\n \n \t\tif (io->type == POLLOUT) {\n-\t\t\tssize_t len = xwrite(io->fd,\n-\t\t\t\t\t io->u.out.buf, io->u.out.len);\n+\t\t\tssize_t len;\n+\n+\t\t\t/*\n+\t\t\t * Don't use xwrite() here. It loops forever on EAGAIN,\n+\t\t\t * and we're in our own poll() loop here.\n+\t\t\t *\n+\t\t\t * Note that we lose xwrite()'s handling of MAX_IO_SIZE\n+\t\t\t * and EINTR, so we have to implement those ourselves.\n+\t\t\t */\n+\t\t\tlen = write(io->fd, io->u.out.buf,\n+\t\t\t\t io->u.out.len <= MAX_IO_SIZE ?\n+\t\t\t\t io->u.out.len : MAX_IO_SIZE);\n \t\t\tif (len < 0) {\n-\t\t\t\tio->error = errno;\n-\t\t\t\tclose(io->fd);\n-\t\t\t\tio->fd = -1;\n+\t\t\t\tif (errno != EINTR && errno != EAGAIN) {\n+\t\t\t\t\tio->error = errno;\n+\t\t\t\t\tclose(io->fd);\n+\t\t\t\t\tio->fd = -1;\n+\t\t\t\t}\n \t\t\t} else {\n \t\t\t\tio->u.out.buf += len;\n \t\t\t\tio->u.out.len -= len;\n-- \n2.37.2.881.gb57357660c\n\n"},{"id":"461345","messageId":"YvyGJiz+CFPcgpML@coredump.intra.peff.net","threadId":"58254","inReplyTo":"YvyE1RDFVAg4uQo1@coredump.intra.peff.net","subject":"[PATCH v2 5/6] pipe_command(): handle ENOSPC when writing to a pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-17T06:09:42Z","receivedAt":"2022-08-17T06:09:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When write() to a non-blocking pipe fails because the buffer is full,\nPOSIX says we should see EAGAIN. But our mingw_write() compat layer on\nWindows actually returns ENOSPC for this case. This is probably\nsomething we want to correct, but given that we don't plan to use\nnon-blocking descriptors in a lot of places, we can work around it by\njust catching ENOSPC alongside EAGAIN. If we ever do fix mingw_write(),\nthen this patch can be reverted.\n\nWe don't actually use a non-blocking pipe yet, so this is still just\npreparation.\n\nHelped-by: René Scharfe \nSigned-off-by: Jeff King \n---\nIronically, this ENOSPC bug means that switching away from xwrite() in\nthe previous patch wasn't necessary (because it's not clever enough to\nknow that ENOSPC on a pipe means EAGAIN!). But I think handling both\nshows the intent, and sets us up better for fixing mingw_write().\n\n run-command.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex e078c3046f..5fbaa8b5ac 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1377,7 +1377,8 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)\n \t\t\t\t io->u.out.len <= MAX_IO_SIZE ?\n \t\t\t\t io->u.out.len : MAX_IO_SIZE);\n \t\t\tif (len < 0) {\n-\t\t\t\tif (errno != EINTR && errno != EAGAIN) {\n+\t\t\t\tif (errno != EINTR && errno != EAGAIN &&\n+\t\t\t\t errno != ENOSPC) {\n \t\t\t\t\tio->error = errno;\n \t\t\t\t\tclose(io->fd);\n \t\t\t\t\tio->fd = -1;\n-- \n2.37.2.881.gb57357660c\n\n"},{"id":"461346","messageId":"YvyGTmlVZlXwV4kF@coredump.intra.peff.net","threadId":"58254","inReplyTo":"YvyE1RDFVAg4uQo1@coredump.intra.peff.net","subject":"[PATCH v2 6/6] pipe_command(): mark stdin descriptor as non-blocking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-17T06:10:22Z","receivedAt":"2022-08-17T06:10:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our pipe_command() helper lets you both write to and read from a child\nprocess on its stdin/stdout. It's supposed to work without deadlocks\nbecause we use poll() to check when descriptors are ready for reading or\nwriting. But there's a bug: if both the data to be written and the data\nto be read back exceed the pipe buffer, we'll deadlock.\n\nThe issue is that the code assumes that if you have, say, a 2MB buffer\nto write and poll() tells you that the pipe descriptor is ready for\nwriting, that calling:\n\n write(cmd->in, buf, 2*1024*1024);\n\nwill do a partial write, filling the pipe buffer and then returning what\nit did write. And that is what it would do on a socket, but not for a\npipe. When writing to a pipe, at least on Linux, it will block waiting\nfor the child process to read() more. And now we have a potential\ndeadlock, because the child may be writing back to us, waiting for us to\nread() ourselves.\n\nAn easy way to trigger this is:\n\n git -c add.interactive.useBuiltin=true \\\n -c interactive.diffFilter=cat \\\n checkout -p HEAD~200\n\nThe diff against HEAD~200 will be big, and the filter wants to write all\nof it back to us (obviously this is a dummy filter, but in the real\nworld something like diff-highlight would similarly stream back a big\noutput).\n\nIf you set add.interactive.useBuiltin to false, the problem goes away,\nbecause now we're not using pipe_command() anymore (instead, that part\nhappens in perl). But this isn't a bug in the interactive code at all.\nIt's the underlying pipe_command() code which is broken, and has been\nall along.\n\nWe presumably didn't notice because most calls only do input _or_\noutput, not both. And the few that do both, like gpg calls, may have\nlarge inputs or outputs, but never both at the same time (e.g., consider\nsigning, which has a large payload but a small signature comes back).\n\nThe obvious fix is to put the descriptor into non-blocking mode, and\nindeed, that makes the problem go away. Callers shouldn't need to\ncare, because they never see the descriptor (they hand us a buffer to\nfeed into it).\n\nThe included test fails reliably on Linux without this patch. Curiously,\nit doesn't fail in our Windows CI environment, but has been reported to\ndo so for individual developers. It should pass in any environment after\nthis patch (courtesy of the compat/ layers added in the last few\ncommits).\n\nSigned-off-by: Jeff King \n---\nSame as before, minus the compat/ bits which were already done, and we\ncan now claim victory on Windows, too.\n\n run-command.c | 10 ++++++++++\n t/t3701-add-interactive.sh | 13 +++++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 5fbaa8b5ac..5ec3a46dcc 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -10,6 +10,7 @@\n #include \"config.h\"\n #include \"packfile.h\"\n #include \"hook.h\"\n+#include \"compat/nonblock.h\"\n \n void child_process_init(struct child_process *child)\n {\n@@ -1451,6 +1452,15 @@ int pipe_command(struct child_process *cmd,\n \t\treturn -1;\n \n \tif (in) {\n+\t\tif (enable_pipe_nonblock(cmd->in) < 0) {\n+\t\t\terror_errno(\"unable to make pipe non-blocking\");\n+\t\t\tclose(cmd->in);\n+\t\t\tif (out)\n+\t\t\t\tclose(cmd->out);\n+\t\t\tif (err)\n+\t\t\t\tclose(cmd->err);\n+\t\t\treturn -1;\n+\t\t}\n \t\tio[nr].fd = cmd->in;\n \t\tio[nr].type = POLLOUT;\n \t\tio[nr].u.out.buf = in;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex b354fb39de..3b7df9bed5 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -766,6 +766,19 @@ test_expect_success 'detect bogus diffFilter output' '\n \tforce_color test_must_fail git add -p test &&\n+\ttest_config interactive.diffFilter cat &&\n+\tprintf y >y &&\n+\tforce_color git add -p >output 2>&1 On Wed, Aug 17, 2022 at 01:39:32AM -0400, Jeff King wrote:\n> \n> > I wouldn't be opposed to that, in the sense that it's supposed to be a\n> > black box to the caller, and it's relatively small in size. But I think\n> > we're pretty close to having something usable without that, so I'd like\n> > to pursue a smaller fix in the interim.\n> \n> So here's what I came up with. Most of the changes are done as\n> preparatory steps, so I believe the final commit should just work out of\n> the box for you, as well as on Windows CI (which I just kicked off).\n\nThe Windows CI did indeed complete successfully, though I'm not too\nsurprised, since it never hit the blocking/deadlock case in the first\nplace. I _think_ from our conversation that it should work for you, too,\nbut please let me know. Fingers crossed. :)\n\n-Peff\n"},{"id":"461379","messageId":"xmqqo7wivgua.fsf@gitster.g","threadId":"58254","inReplyTo":"YvyGJiz+CFPcgpML@coredump.intra.peff.net","subject":"Re: [PATCH v2 5/6] pipe_command(): handle ENOSPC when writing to a pipe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-17T18:57:01Z","receivedAt":"2022-08-17T18:57:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When write() to a non-blocking pipe fails because the buffer is full,\n> POSIX says we should see EAGAIN. But our mingw_write() compat layer on\n> Windows actually returns ENOSPC for this case. This is probably\n> something we want to correct, but given that we don't plan to use\n> non-blocking descriptors in a lot of places, we can work around it by\n> just catching ENOSPC alongside EAGAIN. If we ever do fix mingw_write(),\n> then this patch can be reverted.\n>\n> We don't actually use a non-blocking pipe yet, so this is still just\n> preparation.\n>\n> Helped-by: René Scharfe \n> Signed-off-by: Jeff King \n> ---\n> Ironically, this ENOSPC bug means that switching away from xwrite() in\n> the previous patch wasn't necessary (because it's not clever enough to\n> know that ENOSPC on a pipe means EAGAIN!). But I think handling both\n> shows the intent, and sets us up better for fixing mingw_write().\n\nYeah, I am impressed by the attention of small details by you two\nshown here to split the steps 4/6 and 5/6. If we consider that this\nstep is a band-aid we'd be happier if we can remove, perhaps in-code\ncomment to explain why we deal with ENOSPC here, instead of burying\nit only in the log message, would help remind people of the issue\n(but of course the patch is good with or without such a tweak, which\nis only relevant in the longer term).\n\n> run-command.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index e078c3046f..5fbaa8b5ac 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1377,7 +1377,8 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)\n> \t\t\t\t io->u.out.len <= MAX_IO_SIZE ?\n> \t\t\t\t io->u.out.len : MAX_IO_SIZE);\n> \t\t\tif (len < 0) {\n> -\t\t\t\tif (errno != EINTR && errno != EAGAIN) {\n> +\t\t\t\tif (errno != EINTR && errno != EAGAIN &&\n> +\t\t\t\t errno != ENOSPC) {\n> \t\t\t\t\tio->error = errno;\n> \t\t\t\t\tclose(io->fd);\n> \t\t\t\t\tio->fd = -1;\n"},{"id":"461386","messageId":"xmqqwnb6ty9u.fsf@gitster.g","threadId":"58254","inReplyTo":"YvyFBzdO8PN7Ou0W@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/6] compat: add function to enable nonblocking pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-17T20:23:25Z","receivedAt":"2022-08-17T20:23:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We'd like to be able to make some of our pipes nonblocking so that\n> poll() can be used effectively, but O_NONBLOCK isn't portable. Let's\n> introduce a compat wrapper so this can be abstracted for each platform.\n>\n> The interface is as narrow as possible to let platforms do what's\n> natural there (rather than having to implement fcntl() and a fake\n> O_NONBLOCK for example, or having to handle other types of descriptors).\n>\n> The next commit will add Windows support, at which point we should be\n> covering all platforms in practice. But if we do find some other\n> platform without O_NONBLOCK, we'll return ENOSYS. Arguably we could just\n> trigger a build-time #error in this case, which would catch the problem\n> earlier. But since we're not planning to use this compat wrapper in many\n> code paths, a seldom-seen runtime error may be friendlier for such a\n> platform than blocking compilation completely. Our test suite would\n> still notice it.\n\nVery well reasoned.\n\nThe only small \"huh?\" factor was that the name of the helper is not\nenable_nonblock(), but singles out pipe as valid thing to work on.\nI think it is perfectly fine, given that the current plan we have is\nto use this on the pipe with the command we spawn with\npipe_command(), and it probably is even preferrable to explicitly\ndeclare that this is designed to only work with pipes and future\ndevelopers who try to abuse it on other kinds of file descriptors\nare on their own. And \"pipe\" in the name of this helper may be such\na declaration that is strong enough.\n\nThanks.\n"},{"id":"461395","messageId":"Yv3QcK4PPP9yJkKR@coredump.intra.peff.net","threadId":"58254","inReplyTo":"xmqqo7wivgua.fsf@gitster.g","subject":"Re: [PATCH v2 5/6] pipe_command(): handle ENOSPC when writing to a pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-18T05:38:56Z","receivedAt":"2022-08-18T05:39:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 17, 2022 at 11:57:01AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > When write() to a non-blocking pipe fails because the buffer is full,\n> > POSIX says we should see EAGAIN. But our mingw_write() compat layer on\n> > Windows actually returns ENOSPC for this case. This is probably\n> > something we want to correct, but given that we don't plan to use\n> > non-blocking descriptors in a lot of places, we can work around it by\n> > just catching ENOSPC alongside EAGAIN. If we ever do fix mingw_write(),\n> > then this patch can be reverted.\n> >\n> > We don't actually use a non-blocking pipe yet, so this is still just\n> > preparation.\n> >\n> > Helped-by: René Scharfe \n> > Signed-off-by: Jeff King \n> > ---\n> > Ironically, this ENOSPC bug means that switching away from xwrite() in\n> > the previous patch wasn't necessary (because it's not clever enough to\n> > know that ENOSPC on a pipe means EAGAIN!). But I think handling both\n> > shows the intent, and sets us up better for fixing mingw_write().\n> \n> Yeah, I am impressed by the attention of small details by you two\n> shown here to split the steps 4/6 and 5/6. If we consider that this\n> step is a band-aid we'd be happier if we can remove, perhaps in-code\n> comment to explain why we deal with ENOSPC here, instead of burying\n> it only in the log message, would help remind people of the issue\n> (but of course the patch is good with or without such a tweak, which\n> is only relevant in the longer term).\n\nYeah, you may be right. I had originally written a quite long comment\nhere (before I split things up so much), but found in the splitting that\nit made more sense to put most of the details into the commit message.\n\nBut maybe it's worth squashing this in?\n\ndiff --git a/run-command.c b/run-command.c\nindex 5fbaa8b5ac..065883672b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1372,6 +1372,10 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)\n \t\t\t *\n \t\t\t * Note that we lose xwrite()'s handling of MAX_IO_SIZE\n \t\t\t * and EINTR, so we have to implement those ourselves.\n+\t\t\t *\n+\t\t\t * We also check for ENOSPC to handle a quirk of\n+\t\t\t * mingw_write(), which uses that for a full pipe\n+\t\t\t * instead of EAGAIN.\n \t\t\t */\n \t\t\tlen = write(io->fd, io->u.out.buf,\n \t\t\t\t io->u.out.len <= MAX_IO_SIZE ?\n\n-Peff\n"},{"id":"461396","messageId":"Yv3REyubPXQJVboY@coredump.intra.peff.net","threadId":"58254","inReplyTo":"xmqqwnb6ty9u.fsf@gitster.g","subject":"Re: [PATCH v2 1/6] compat: add function to enable nonblocking pipes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-18T05:41:39Z","receivedAt":"2022-08-18T05:41:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 17, 2022 at 01:23:25PM -0700, Junio C Hamano wrote:\n\n> The only small \"huh?\" factor was that the name of the helper is not\n> enable_nonblock(), but singles out pipe as valid thing to work on.\n> I think it is perfectly fine, given that the current plan we have is\n> to use this on the pipe with the command we spawn with\n> pipe_command(), and it probably is even preferrable to explicitly\n> declare that this is designed to only work with pipes and future\n> developers who try to abuse it on other kinds of file descriptors\n> are on their own. And \"pipe\" in the name of this helper may be such\n> a declaration that is strong enough.\n\nMy goal was to explain that \"huh\" with this bit in the commit message:\n\n> > The interface is as narrow as possible to let platforms do what's\n> > natural there (rather than having to implement fcntl() and a fake\n> > O_NONBLOCK for example, or having to handle other types of descriptors).\n\nwithout going into too many details. I do think it would be easier to\nexplain if the Windows implementation was added in the same commit\n(since it is explicitly just about pipes), but I wanted to credit René\nas the author there. I'm not sure if it's worth folding them together\nand adding a co-author credit instead. Or maybe I should state outright\nin this commit message that we're doing it for Windows. ;)\n\n-Peff\n"},{"id":"461621","messageId":"3e102aa4-ff44-0b7b-8164-f79bb00a8d3b@web.de","threadId":"58254","inReplyTo":"YvyE1RDFVAg4uQo1@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/6] fix pipe_command() deadlock","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-19T21:19:08Z","receivedAt":"2022-08-19T21:19:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.08.2022 um 08:04 schrieb Jeff King:\n> On Wed, Aug 17, 2022 at 01:39:32AM -0400, Jeff King wrote:\n>\n>> I wouldn't be opposed to that, in the sense that it's supposed to be a\n>> black box to the caller, and it's relatively small in size. But I think\n>> we're pretty close to having something usable without that, so I'd like\n>> to pursue a smaller fix in the interim.\n>\n> So here's what I came up with. Most of the changes are done as\n> preparatory steps, so I believe the final commit should just work out of\n> the box for you, as well as on Windows CI (which I just kicked off).\n\nIndeed: This series passes the test suite for me on Windows 11.\n\nRené\n"},{"id":"461647","messageId":"YwCHkLd0lTxWzDJ9@coredump.intra.peff.net","threadId":"58254","inReplyTo":"3e102aa4-ff44-0b7b-8164-f79bb00a8d3b@web.de","subject":"Re: [PATCH v2 0/6] fix pipe_command() deadlock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-20T07:04:48Z","receivedAt":"2022-08-20T07:04:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 19, 2022 at 11:19:08PM +0200, René Scharfe wrote:\n\n> Am 17.08.2022 um 08:04 schrieb Jeff King:\n> > On Wed, Aug 17, 2022 at 01:39:32AM -0400, Jeff King wrote:\n> >\n> >> I wouldn't be opposed to that, in the sense that it's supposed to be a\n> >> black box to the caller, and it's relatively small in size. But I think\n> >> we're pretty close to having something usable without that, so I'd like\n> >> to pursue a smaller fix in the interim.\n> >\n> > So here's what I came up with. Most of the changes are done as\n> > preparatory steps, so I believe the final commit should just work out of\n> > the box for you, as well as on Windows CI (which I just kicked off).\n> \n> Indeed: This series passes the test suite for me on Windows 11.\n\nYay. :)\n\n-Peff\n"}]}