[RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking

[RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King]

Our pipe_command() helper lets you both write to and read from a child
process on its stdin/stdout. It's supposed to work without deadlocks
because we use poll() to check when descriptors are ready for reading or
writing. But there's a bug: if both the data to be written and the data
to be read back exceed the pipe buffer, we'll deadlock.

The issue is that the code assumes that if you have, say, a 2MB buffer
to write and poll() tells you that the pipe descriptor is ready for
writing, that calling:

  write(cmd->in, buf, 2*1024*1024);

will do a partial write, filling the pipe buffer and then returning what
it did write. And that is what it would do on a socket, but not for a
pipe. When writing to a pipe, at least on Linux, it will block waiting
for the child process to read() more. And now we have a potential
deadlock, because the child may be writing back to us, waiting for us to
read() ourselves.

An easy way to trigger this is:

  git -c add.interactive.useBuiltin=true \
      -c interactive.diffFilter=cat \
      checkout -p HEAD~200

The diff against HEAD~200 will be big, and the filter wants to write all
of it back to us (obviously this is a dummy filter, but in the real
world something like diff-highlight would similarly stream back a big
output).

If you set add.interactive.useBuiltin to false, the problem goes away,
because now we're not using pipe_command() anymore (instead, that part
happens in perl). But this isn't a bug in the interactive code at all.
It's the underlying pipe_command() code which is broken, and has been
all along.

We presumably didn't notice because most calls only do input _or_
output, not both. And the few that do both, like gpg calls, may have
large inputs or outputs, but never both at the same time (e.g., consider
signing, which has a large payload but a small signature comes back).

The obvious fix is to put the descriptor into non-blocking mode, and
indeed, that makes the problem go away. Callers shouldn't need to
care, because they never see the descriptor (they hand us a buffer to
feed into it).

Signed-off-by: Jeff King <peff@peff.net>
---
+cc Dscho for two reasons:

  - this is a fallout of the builtin add-interactive, though I again
    emphasize that it's just triggering a lurking bug (which is mine, no
    less!). But I thought you'd want to know.

  - more importantly, I'm not sure of the portability implications of
    the fix. This is our first use of O_NONBLOCK outside of the
    compat/simple-ipc unix-socket code. Do we need to abstract this
    behind a compat/ layer for Windows?

 run-command.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/run-command.c b/run-command.c
index 14f17830f5..45bffb4b11 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1418,6 +1418,14 @@ static int pump_io(struct io_pump *slots, int nr)
 	return 0;
 }
 
+static int make_nonblock(int fd)
+{
+	int flags = fcntl(fd, F_GETFL);
+	if (flags < 0)
+		return -1;
+	flags |= O_NONBLOCK;
+	return fcntl(fd, F_SETFL, flags);
+}
 
 int pipe_command(struct child_process *cmd,
 		 const char *in, size_t in_len,
@@ -1438,6 +1446,15 @@ int pipe_command(struct child_process *cmd,
 		return -1;
 
 	if (in) {
+		if (make_nonblock(cmd->in) < 0) {
+			error_errno("unable to make pipe non-blocking");
+			close(cmd->in);
+			if (out)
+				close(cmd->out);
+			if (err)
+				close(cmd->err);
+			return -1;
+		}
 		io[nr].fd = cmd->in;
 		io[nr].type = POLLOUT;
 		io[nr].u.out.buf = in;
-- 
2.37.1.804.g1775fa20e0

Re: [RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Our pipe_command() helper lets you both write to and read from a child
> process on its stdin/stdout. It's supposed to work without deadlocks
> because we use poll() to check when descriptors are ready for reading or
> writing. But there's a bug: if both the data to be written and the data
> to be read back exceed the pipe buffer, we'll deadlock.
> ...
> If you set add.interactive.useBuiltin to false, the problem goes away,
> because now we're not using pipe_command() anymore (instead, that part
> happens in perl). But this isn't a bug in the interactive code at all.
> It's the underlying pipe_command() code which is broken, and has been
> all along.
> ...
> The obvious fix is to put the descriptor into non-blocking mode, and
> indeed, that makes the problem go away. Callers shouldn't need to
> care, because they never see the descriptor (they hand us a buffer to
> feed into it).

Thanks for a very well reasoned and explained patch.

>   - more importantly, I'm not sure of the portability implications of
>     the fix. This is our first use of O_NONBLOCK outside of the
>     compat/simple-ipc unix-socket code. Do we need to abstract this
>     behind a compat/ layer for Windows?

Yup.  A very good question to ask for the platform maintainer.

>  run-command.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 14f17830f5..45bffb4b11 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1418,6 +1418,14 @@ static int pump_io(struct io_pump *slots, int nr)
>  	return 0;
>  }
>  
> +static int make_nonblock(int fd)
> +{
> +	int flags = fcntl(fd, F_GETFL);
> +	if (flags < 0)
> +		return -1;
> +	flags |= O_NONBLOCK;
> +	return fcntl(fd, F_SETFL, flags);
> +}
>  
>  int pipe_command(struct child_process *cmd,
>  		 const char *in, size_t in_len,
> @@ -1438,6 +1446,15 @@ int pipe_command(struct child_process *cmd,
>  		return -1;
>  
>  	if (in) {
> +		if (make_nonblock(cmd->in) < 0) {
> +			error_errno("unable to make pipe non-blocking");
> +			close(cmd->in);
> +			if (out)
> +				close(cmd->out);
> +			if (err)
> +				close(cmd->err);
> +			return -1;
> +		}
>  		io[nr].fd = cmd->in;
>  		io[nr].type = POLLOUT;
>  		io[nr].u.out.buf = in;

Re: [RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King]

On Tue, Aug 02, 2022 at 12:13:07AM -0400, Jeff King wrote:

>   - more importantly, I'm not sure of the portability implications of
>     the fix. This is our first use of O_NONBLOCK outside of the
>     compat/simple-ipc unix-socket code. Do we need to abstract this
>     behind a compat/ layer for Windows?

So I think the answer is pretty clearly "yes", from the Windows CI
results:

  run-command.c:1429:18: 'O_NONBLOCK' undeclared (first use in this function)
         flags |= O_NONBLOCK;
                  ^~~~~~~~~~

It looks like we'd have the option of either adding F_GETFL/F_SETFL
support to compat/mingw.c's fake fcntl() function, or adding a
compat/nonblock.c that abstracts the whole thing. I'd prefer the latter,
as it makes the interface a bit narrower.

But I'm not sure what should go on the Windows side of that #ifdef.
Unlike some other spots, I don't think we can just make it a noop, or
Windows will be subject to the same deadlock (unless for some reason its
write() does behave differently?).

-Peff

Re: [RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It looks like we'd have the option of either adding F_GETFL/F_SETFL
> support to compat/mingw.c's fake fcntl() function, or adding a
> compat/nonblock.c that abstracts the whole thing. I'd prefer the latter,
> as it makes the interface a bit narrower.

I agree that "make this non-blocking" is the preferred interface
over "emuilate fcntl".

> But I'm not sure what should go on the Windows side of that #ifdef.
> Unlike some other spots, I don't think we can just make it a noop, or
> Windows will be subject to the same deadlock (unless for some reason its
> write() does behave differently?).

Let them deadlock so that they can fix it, and until then leave it a
noop?  That may break the CI tests for them so we could hide the
known-to-be-broken test behind prerequisite to buy them time,
perhaps?

[PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King]

On Tue, Aug 02, 2022 at 09:16:17AM -0700, Junio C Hamano wrote:

> > But I'm not sure what should go on the Windows side of that #ifdef.
> > Unlike some other spots, I don't think we can just make it a noop, or
> > Windows will be subject to the same deadlock (unless for some reason its
> > write() does behave differently?).
> 
> Let them deadlock so that they can fix it, and until then leave it a
> noop?  That may break the CI tests for them so we could hide the
> known-to-be-broken test behind prerequisite to buy them time,
> perhaps?

I was skeptical of including a test, but I was able to come up with a
reproduction that triggers for me on Linux, but isn't too expensive to
keep around.

Much to my surprise (and delight), it passes on the Windows CI with the
noop! So I guess their pipes behave in the way we want by default.

It does feel like a bit of a landmine to have an enable_nonblock()
function which might do nothing. I'm not sure if other descriptor types
might behave differently on Windows. But hopefully the comment above the
function is sufficient to make people think twice.

-- >8 --
Subject: [PATCH] pipe_command(): mark stdin descriptor as non-blocking

Our pipe_command() helper lets you both write to and read from a child
process on its stdin/stdout. It's supposed to work without deadlocks
because we use poll() to check when descriptors are ready for reading or
writing. But there's a bug: if both the data to be written and the data
to be read back exceed the pipe buffer, we'll deadlock.

The issue is that the code assumes that if you have, say, a 2MB buffer
to write and poll() tells you that the pipe descriptor is ready for
writing, that calling:

  write(cmd->in, buf, 2*1024*1024);

will do a partial write, filling the pipe buffer and then returning what
it did write. And that is what it would do on a socket, but not for a
pipe. When writing to a pipe, at least on Linux, it will block waiting
for the child process to read() more. And now we have a potential
deadlock, because the child may be writing back to us, waiting for us to
read() ourselves.

An easy way to trigger this is:

  git -c add.interactive.useBuiltin=true \
      -c interactive.diffFilter=cat \
      checkout -p HEAD~200

The diff against HEAD~200 will be big, and the filter wants to write all
of it back to us (obviously this is a dummy filter, but in the real
world something like diff-highlight would similarly stream back a big
output).

If you set add.interactive.useBuiltin to false, the problem goes away,
because now we're not using pipe_command() anymore (instead, that part
happens in perl). But this isn't a bug in the interactive code at all.
It's the underlying pipe_command() code which is broken, and has been
all along.

We presumably didn't notice because most calls only do input _or_
output, not both. And the few that do both, like gpg calls, may have
large inputs or outputs, but never both at the same time (e.g., consider
signing, which has a large payload but a small signature comes back).

The obvious fix is to put the descriptor into non-blocking mode, and
indeed, that makes the problem go away. Callers shouldn't need to
care, because they never see the descriptor (they hand us a buffer to
feed into it).

Unfortunately O_NONBLOCK isn't available everywhere, especially on
Windows. However, the included test seems to work fine there, which
implies that pipes there do not behave in the same way (they will do the
partial write by default, which is what we want). This is true even if I
size up the diff for a larger pipe buffer (the value chosen here
triggers the deadlock on Linux, but isn't too expensive to keep as a
regression test).

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile                   |  1 +
 compat/nonblock.c          | 22 ++++++++++++++++++++++
 compat/nonblock.h          | 10 ++++++++++
 run-command.c              | 10 ++++++++++
 t/t3701-add-interactive.sh | 14 ++++++++++++++
 5 files changed, 57 insertions(+)
 create mode 100644 compat/nonblock.c
 create mode 100644 compat/nonblock.h

diff --git a/Makefile b/Makefile
index 1624471bad..78bedf26e0 100644
--- a/Makefile
+++ b/Makefile
@@ -918,6 +918,7 @@ LIB_OBJS += combine-diff.o
 LIB_OBJS += commit-graph.o
 LIB_OBJS += commit-reach.o
 LIB_OBJS += commit.o
+LIB_OBJS += compat/nonblock.o
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += compat/zlib-uncompress2.o
diff --git a/compat/nonblock.c b/compat/nonblock.c
new file mode 100644
index 0000000000..897c099010
--- /dev/null
+++ b/compat/nonblock.c
@@ -0,0 +1,22 @@
+#include "git-compat-util.h"
+#include "nonblock.h"
+
+#ifdef O_NONBLOCK
+
+int enable_nonblock(int fd)
+{
+	int flags = fcntl(fd, F_GETFL);
+	if (flags < 0)
+		return -1;
+	flags |= O_NONBLOCK;
+	return fcntl(fd, F_SETFL, flags);
+}
+
+#else
+
+int enable_nonblock(int fd)
+{
+	return 0;
+}
+
+#endif
diff --git a/compat/nonblock.h b/compat/nonblock.h
new file mode 100644
index 0000000000..2721f72187
--- /dev/null
+++ b/compat/nonblock.h
@@ -0,0 +1,10 @@
+#ifndef COMPAT_NONBLOCK_H
+#define COMPAT_NONBLOCK_H
+
+/*
+ * Enable non-blocking I/O for the passed-in descriptor. Note that this is a
+ * noop on systems without O_NONBLOCK, like Windows! Use with caution.
+ */
+int enable_nonblock(int fd);
+
+#endif
diff --git a/run-command.c b/run-command.c
index 14f17830f5..ed99503b22 100644
--- a/run-command.c
+++ b/run-command.c
@@ -10,6 +10,7 @@
 #include "config.h"
 #include "packfile.h"
 #include "hook.h"
+#include "compat/nonblock.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -1438,6 +1439,15 @@ int pipe_command(struct child_process *cmd,
 		return -1;
 
 	if (in) {
+		if (enable_nonblock(cmd->in) < 0) {
+			error_errno("unable to make pipe non-blocking");
+			close(cmd->in);
+			if (out)
+				close(cmd->out);
+			if (err)
+				close(cmd->err);
+			return -1;
+		}
 		io[nr].fd = cmd->in;
 		io[nr].type = POLLOUT;
 		io[nr].u.out.buf = in;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index b354fb39de..01d6ce9c83 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -766,6 +766,20 @@ test_expect_success 'detect bogus diffFilter output' '
 	force_color test_must_fail git add -p <y
 '
 
+test_expect_success 'handle very large filtered diff' '
+	git reset --hard &&
+	# The specific number here is not important, but it must
+	# be large enough that the output of "git diff --color"
+	# fills up the pipe buffer. 10,000 results in ~200k of
+	# colored output.
+	test_seq 10000 >test &&
+	false &&
+	test_config interactive.diffFilter cat &&
+	printf y >y &&
+	force_color git add -p >output 2>&1 <y &&
+	git diff-files --exit-code -- test
+'
+
 test_expect_success 'diff.algorithm is passed to `git diff-files`' '
 	git reset --hard &&
 
-- 
2.37.1.810.g8c5f98b46b

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[René Scharfe]

Am 03.08.22 um 05:53 schrieb Jeff King:
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index b354fb39de..01d6ce9c83 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -766,6 +766,20 @@ test_expect_success 'detect bogus diffFilter output' '
>  	force_color test_must_fail git add -p <y
>  '
>
> +test_expect_success 'handle very large filtered diff' '
> +	git reset --hard &&
> +	# The specific number here is not important, but it must
> +	# be large enough that the output of "git diff --color"
> +	# fills up the pipe buffer. 10,000 results in ~200k of
> +	# colored output.
> +	test_seq 10000 >test &&
> +	false &&

Isn't this test going to end here, reporting failure before it even gets
to the interesting part?

> +	test_config interactive.diffFilter cat &&
> +	printf y >y &&
> +	force_color git add -p >output 2>&1 <y &&
> +	git diff-files --exit-code -- test
> +'
> +
>  test_expect_success 'diff.algorithm is passed to `git diff-files`' '
>  	git reset --hard &&
>

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King]

On Wed, Aug 03, 2022 at 06:45:23PM +0200, René Scharfe wrote:

> > +test_expect_success 'handle very large filtered diff' '
> > +	git reset --hard &&
> > +	# The specific number here is not important, but it must
> > +	# be large enough that the output of "git diff --color"
> > +	# fills up the pipe buffer. 10,000 results in ~200k of
> > +	# colored output.
> > +	test_seq 10000 >test &&
> > +	false &&
> 
> Isn't this test going to end here, reporting failure before it even gets
> to the interesting part?

Urgh, whoops. That was from some last-minute tweaking of the comment.
There was also a line:

  git diff --color | wc -c

before it so I could measure how big the output was for a few values.

It snuck into the emailed patch, but the actual test runs (including the
Windows CI) didn't include that (since obviously they'd have failed the
test).

Thanks for catching.

-Peff

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[René Scharfe]

Am 03.08.2022 um 19:20 schrieb Jeff King:
> On Wed, Aug 03, 2022 at 06:45:23PM +0200, René Scharfe wrote:
>
>>> +test_expect_success 'handle very large filtered diff' '
>>> +	git reset --hard &&
>>> +	# The specific number here is not important, but it must
>>> +	# be large enough that the output of "git diff --color"
>>> +	# fills up the pipe buffer. 10,000 results in ~200k of
>>> +	# colored output.
>>> +	test_seq 10000 >test &&
>>> +	false &&
>>
>> Isn't this test going to end here, reporting failure before it even gets
>> to the interesting part?
>
> Urgh, whoops. That was from some last-minute tweaking of the comment.
> There was also a line:
>
>   git diff --color | wc -c
>
> before it so I could measure how big the output was for a few values.
>
> It snuck into the emailed patch, but the actual test runs (including the
> Windows CI) didn't include that (since obviously they'd have failed the
> test).

Without that line the added test hangs for me on the Git for Windows
SDK on Windows 11.

With the patch below it fails and reports basically nothing:

   expecting success of 3701.57 'handle very large filtered diff':
           git reset --hard &&
           # The specific number here is not important, but it must
           # be large enough that the output of "git diff --color"
           # fills up the pipe buffer. 10,000 results in ~200k of
           # colored output.
           test_seq 10000 >test &&
           test_config interactive.diffFilter cat &&
           printf y >y &&
           force_color git add -p >output 2>&1 <y &&
           git diff-files --exit-code -- test

   HEAD is now at 095e8c6 main
   not ok 57 - handle very large filtered diff
   #
   #               git reset --hard &&
   #               # The specific number here is not important, but it must
   #               # be large enough that the output of "git diff --color"
   #               # fills up the pipe buffer. 10,000 results in ~200k of
   #               # colored output.
   #               test_seq 10000 >test &&
   #               test_config interactive.diffFilter cat &&
   #               printf y >y &&
   #               force_color git add -p >output 2>&1 <y &&
   #               git diff-files --exit-code -- test
   #
   1..57

The file "output" contains "error: failed to run 'cat'".  This is
add-patch.c::parse_diff() reporting that pipe_command() failed.  So
that's not it, yet.  (I don't actually know what I'm doing here.)

---
 compat/nonblock.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/compat/nonblock.c b/compat/nonblock.c
index 897c099010..030cf92af2 100644
--- a/compat/nonblock.c
+++ b/compat/nonblock.c
@@ -14,8 +14,21 @@ int enable_nonblock(int fd)

 #else

+#include "win32.h"
+
 int enable_nonblock(int fd)
 {
+	DWORD mode;
+	HANDLE handle = winansi_get_osfhandle(fd);
+	if (!handle)
+		return -1;
+	if (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))
+		return -1;
+	if (mode & PIPE_NOWAIT)
+		return 0;
+	mode |= PIPE_NOWAIT;
+	if (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))
+		return -1;
 	return 0;
 }

--
2.37.1.windows.1

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King]

On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:

> Without that line the added test hangs for me on the Git for Windows
> SDK on Windows 11.

Hmph. Interesting that it passes in CI, but not on your local setup.
I wonder why pipes would behave differently. Or perhaps there is even
some configuration different that means we are still running the perl
add--interactive there, though I kind of doubt it (and couldn't find
anything pointing there).

Still, if it fails in at least one spot, it's something we need to deal
with. On the plus side, once we figure out how to fix it, the
hand-grenade of "enable_nonblock() does nothing on Windows" will not be
present anymore. ;)

> With the patch below it fails and reports basically nothing:
> [...]
>    not ok 57 - handle very large filtered diff
>    #
>    #               git reset --hard &&
>    #               # The specific number here is not important, but it must
>    #               # be large enough that the output of "git diff --color"
>    #               # fills up the pipe buffer. 10,000 results in ~200k of
>    #               # colored output.
>    #               test_seq 10000 >test &&
>    #               test_config interactive.diffFilter cat &&
>    #               printf y >y &&
>    #               force_color git add -p >output 2>&1 <y &&
>    #               git diff-files --exit-code -- test
>    #
>    1..57
> 
> The file "output" contains "error: failed to run 'cat'".  This is
> add-patch.c::parse_diff() reporting that pipe_command() failed.  So
> that's not it, yet.  (I don't actually know what I'm doing here.)

That implies that your call to enable_nonblock() succeeded, or
pipe_command() itself would have complained, too. Maybe instrument
it like this:

diff --git a/run-command.c b/run-command.c
index 8ea609d4ae..27e79c928a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1473,11 +1473,17 @@ int pipe_command(struct child_process *cmd,
 	}
 
 	if (pump_io(io, nr) < 0) {
+		error_errno("pumping io failed");
 		finish_command(cmd); /* throw away exit code */
 		return -1;
 	}
 
-	return finish_command(cmd);
+	{
+		int ret = finish_command(cmd);
+		if (ret)
+			error("child returned failure %d", ret);
+		return ret;
+	}
 }
 
 enum child_state {

Normally we stay pretty quiet there and let the caller report any
problems, but it lacks enough context to make a more specific error
report.

>  int enable_nonblock(int fd)
>  {
> +	DWORD mode;
> +	HANDLE handle = winansi_get_osfhandle(fd);
> +	if (!handle)
> +		return -1;
> +	if (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))
> +		return -1;
> +	if (mode & PIPE_NOWAIT)
> +		return 0;
> +	mode |= PIPE_NOWAIT;
> +	if (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))
> +		return -1;
>  	return 0;
>  }

This looks plausibly correct to me. ;) We might want to change the name
of the compat layer to enable_pipe_nonblock(), since one assumes from
the function names this only works for pipes.

Thanks for poking at this.

-Peff

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[René Scharfe]

Am 05.08.2022 um 17:36 schrieb Jeff King:
> On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:
>
>> Without that line the added test hangs for me on the Git for Windows
>> SDK on Windows 11.
>
> Hmph. Interesting that it passes in CI, but not on your local setup.
> I wonder why pipes would behave differently. Or perhaps there is even
> some configuration different that means we are still running the perl
> add--interactive there, though I kind of doubt it (and couldn't find
> anything pointing there).
>
> Still, if it fails in at least one spot, it's something we need to deal
> with. On the plus side, once we figure out how to fix it, the
> hand-grenade of "enable_nonblock() does nothing on Windows" will not be
> present anymore. ;)
>
>> With the patch below it fails and reports basically nothing:
>> [...]
>>    not ok 57 - handle very large filtered diff
>>    #
>>    #               git reset --hard &&
>>    #               # The specific number here is not important, but it must
>>    #               # be large enough that the output of "git diff --color"
>>    #               # fills up the pipe buffer. 10,000 results in ~200k of
>>    #               # colored output.
>>    #               test_seq 10000 >test &&
>>    #               test_config interactive.diffFilter cat &&
>>    #               printf y >y &&
>>    #               force_color git add -p >output 2>&1 <y &&
>>    #               git diff-files --exit-code -- test
>>    #
>>    1..57
>>
>> The file "output" contains "error: failed to run 'cat'".  This is
>> add-patch.c::parse_diff() reporting that pipe_command() failed.  So
>> that's not it, yet.  (I don't actually know what I'm doing here.)
>
> That implies that your call to enable_nonblock() succeeded, or
> pipe_command() itself would have complained, too. Maybe instrument
> it like this:
>
> diff --git a/run-command.c b/run-command.c
> index 8ea609d4ae..27e79c928a 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1473,11 +1473,17 @@ int pipe_command(struct child_process *cmd,
>  	}
>
>  	if (pump_io(io, nr) < 0) {
> +		error_errno("pumping io failed");
>  		finish_command(cmd); /* throw away exit code */
>  		return -1;
>  	}
>
> -	return finish_command(cmd);
> +	{
> +		int ret = finish_command(cmd);
> +		if (ret)
> +			error("child returned failure %d", ret);
> +		return ret;
> +	}
>  }
>
>  enum child_state {
>
> Normally we stay pretty quiet there and let the caller report any
> problems, but it lacks enough context to make a more specific error
> report.

This adds "error: pumping io failed: No space left on device" to output.
Which kinda makes sense: With the pipe no longer blocking, there can be
a moment when the buffer is full and writes have to be rejected.  This
condition should be reported with EAGAIN, though.

Adding "if (len < 0 && errno == ENOSPC) continue;" after the xwrite()
call in pump_io_round() lets the test pass.

Perhaps the translation from Windows error code to POSIX is wrong here?

>
>>  int enable_nonblock(int fd)
>>  {
>> +	DWORD mode;
>> +	HANDLE handle = winansi_get_osfhandle(fd);
>> +	if (!handle)
>> +		return -1;
>> +	if (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))
>> +		return -1;
>> +	if (mode & PIPE_NOWAIT)
>> +		return 0;
>> +	mode |= PIPE_NOWAIT;
>> +	if (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))
>> +		return -1;
>>  	return 0;
>>  }
>
> This looks plausibly correct to me. ;) We might want to change the name
> of the compat layer to enable_pipe_nonblock(), since one assumes from
> the function names this only works for pipes.

Right, and the "Named" part should be explained: As [1] says: "[...] you
can often pass a handle to an anonymous pipe to a function that requires
a handle to a named pipe.".

And perhaps that's not the right thing to do after all, as [2] says:
"Note that nonblocking mode is supported for compatibility with
Microsoft LAN Manager version 2.0 and should not be used to achieve
asynchronous input and output (I/O) [...]".

[1] https://docs.microsoft.com/en-us/windows/win32/ipc/anonymous-pipe-operations
[2] https://docs.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-setnamedpipehandlestate

>
> Thanks for poking at this.
>
> -Peff

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[René Scharfe]

Am 05.08.2022 um 23:13 schrieb René Scharfe:
> Am 05.08.2022 um 17:36 schrieb Jeff King:
>> On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:
>>
>>> Without that line the added test hangs for me on the Git for Windows
>>> SDK on Windows 11.
>>
>> Hmph. Interesting that it passes in CI, but not on your local setup.
>> I wonder why pipes would behave differently. Or perhaps there is even
>> some configuration different that means we are still running the perl
>> add--interactive there, though I kind of doubt it (and couldn't find
>> anything pointing there).
>>
>> Still, if it fails in at least one spot, it's something we need to deal
>> with. On the plus side, once we figure out how to fix it, the
>> hand-grenade of "enable_nonblock() does nothing on Windows" will not be
>> present anymore. ;)
>>
>>> With the patch below it fails and reports basically nothing:
>>> [...]
>>>    not ok 57 - handle very large filtered diff
>>>    #
>>>    #               git reset --hard &&
>>>    #               # The specific number here is not important, but it must
>>>    #               # be large enough that the output of "git diff --color"
>>>    #               # fills up the pipe buffer. 10,000 results in ~200k of
>>>    #               # colored output.
>>>    #               test_seq 10000 >test &&
>>>    #               test_config interactive.diffFilter cat &&
>>>    #               printf y >y &&
>>>    #               force_color git add -p >output 2>&1 <y &&
>>>    #               git diff-files --exit-code -- test
>>>    #
>>>    1..57
>>>
>>> The file "output" contains "error: failed to run 'cat'".  This is
>>> add-patch.c::parse_diff() reporting that pipe_command() failed.  So
>>> that's not it, yet.  (I don't actually know what I'm doing here.)
>>
>> That implies that your call to enable_nonblock() succeeded, or
>> pipe_command() itself would have complained, too. Maybe instrument
>> it like this:
>>
>> diff --git a/run-command.c b/run-command.c
>> index 8ea609d4ae..27e79c928a 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -1473,11 +1473,17 @@ int pipe_command(struct child_process *cmd,
>>  	}
>>
>>  	if (pump_io(io, nr) < 0) {
>> +		error_errno("pumping io failed");
>>  		finish_command(cmd); /* throw away exit code */
>>  		return -1;
>>  	}
>>
>> -	return finish_command(cmd);
>> +	{
>> +		int ret = finish_command(cmd);
>> +		if (ret)
>> +			error("child returned failure %d", ret);
>> +		return ret;
>> +	}
>>  }
>>
>>  enum child_state {
>>
>> Normally we stay pretty quiet there and let the caller report any
>> problems, but it lacks enough context to make a more specific error
>> report.

Side note: The above patch gives better insight on error, but also
breaks t4205.105 (%(describe) vs git describe), t7400.16 (submodule add
to .gitignored path fails) and t7406.59 (submodule update --quiet passes
quietness to fetch with a shallow clone).

>
> This adds "error: pumping io failed: No space left on device" to output.
> Which kinda makes sense: With the pipe no longer blocking, there can be
> a moment when the buffer is full and writes have to be rejected.  This
> condition should be reported with EAGAIN, though.
>
> Adding "if (len < 0 && errno == ENOSPC) continue;" after the xwrite()
> call in pump_io_round() lets the test pass.
>
> Perhaps the translation from Windows error code to POSIX is wrong here?

So if we fix that with the patch below, t3701.57 still hangs, but this
time it goes through wrapper.c::handle_nonblock() again and again.
Replacing the "errno = EAGAIN" with a "return 0" to fake report a
successful write of nothing instead lets the test pass.

This seems to make sense -- looping in xwrite() won't help, as we need
to read from the other fd first, to allow the process on the other end
of the pipe to make some progress first, as otherwise the pipe buffer
will stay full in this scenario.  Shouldn't that be a problem on other
systems as well?

René

---
 compat/mingw.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index b5502997e2..e34cceb151 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -702,6 +702,17 @@ ssize_t mingw_write(int fd, const void *buf, size_t len)
 		else
 			errno = EINVAL;
 	}
+	if (result < 0 && errno == ENOSPC) {
+		/* check if fd is a non-blocking pipe */
+		HANDLE h = (HANDLE) _get_osfhandle(fd);
+		DWORD m;
+		if (GetFileType(h) == FILE_TYPE_PIPE &&
+		    GetNamedPipeHandleState(h, &m, NULL, NULL, NULL, NULL, 0) &&
+		    (m & PIPE_NOWAIT))
+			errno = EAGAIN;
+		else
+			errno = ENOSPC;
+	}

 	return result;
 }
--
2.37.1.windows.1

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King]

On Sun, Aug 07, 2022 at 12:15:06PM +0200, René Scharfe wrote:

> > This adds "error: pumping io failed: No space left on device" to output.
> > Which kinda makes sense: With the pipe no longer blocking, there can be
> > a moment when the buffer is full and writes have to be rejected.  This
> > condition should be reported with EAGAIN, though.
> >
> > Adding "if (len < 0 && errno == ENOSPC) continue;" after the xwrite()
> > call in pump_io_round() lets the test pass.
> >
> > Perhaps the translation from Windows error code to POSIX is wrong here?
> 
> So if we fix that with the patch below, t3701.57 still hangs, but this
> time it goes through wrapper.c::handle_nonblock() again and again.
> Replacing the "errno = EAGAIN" with a "return 0" to fake report a
> successful write of nothing instead lets the test pass.
> 
> This seems to make sense -- looping in xwrite() won't help, as we need
> to read from the other fd first, to allow the process on the other end
> of the pipe to make some progress first, as otherwise the pipe buffer
> will stay full in this scenario.  Shouldn't that be a problem on other
> systems as well?

It doesn't happen on Linux; I suspect there's something funny either
about partial writes, or about poll() on Windows. What's supposed to
happen is:

  1. pump_io() calls poll(), which tells us the descriptor is ready to
     write

  2. we call xwrite(), and our actual write() call returns a partial
     write (i.e., reports "ret" bytes < "len" we passed in)

  3. we return back to pump_io() do another round of poll(). If the
     other side consumed some bytes from the pipe, then we may get
     triggered to do another (possibly partial) write. If it didn't, and
     we'd get EAGAIN writing, then poll shouldn't trigger at all!

So it's weird that you'd see EAGAIN in this instance. Either the
underlying write() is refusing to do a partial write (and just returning
an error with EAGAIN in the first place), or the poll emulation is wrong
(telling us the descriptor is ready for writing when it isn't).

Can you instrument pump_io_round() (or use some strace equivalent, if
there is one) to see if we do a successful partial write first (which
implies poll() is wrong in telling us we can write more for the second
round), or if the very first write() is failing (which implies write()
is wrong for returning EAGAIN when it could do a partial write).

-Peff

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[René Scharfe]

Am 07.08.22 um 19:41 schrieb Jeff King:
> On Sun, Aug 07, 2022 at 12:15:06PM +0200, René Scharfe wrote:
>
>>> This adds "error: pumping io failed: No space left on device" to output.
>>> Which kinda makes sense: With the pipe no longer blocking, there can be
>>> a moment when the buffer is full and writes have to be rejected.  This
>>> condition should be reported with EAGAIN, though.
>>>
>>> Adding "if (len < 0 && errno == ENOSPC) continue;" after the xwrite()
>>> call in pump_io_round() lets the test pass.
>>>
>>> Perhaps the translation from Windows error code to POSIX is wrong here?
>>
>> So if we fix that with the patch below, t3701.57 still hangs, but this
>> time it goes through wrapper.c::handle_nonblock() again and again.
>> Replacing the "errno = EAGAIN" with a "return 0" to fake report a
>> successful write of nothing instead lets the test pass.
>>
>> This seems to make sense -- looping in xwrite() won't help, as we need
>> to read from the other fd first, to allow the process on the other end
>> of the pipe to make some progress first, as otherwise the pipe buffer
>> will stay full in this scenario.  Shouldn't that be a problem on other
>> systems as well?
>
> It doesn't happen on Linux; I suspect there's something funny either
> about partial writes, or about poll() on Windows. What's supposed to
> happen is:
>
>   1. pump_io() calls poll(), which tells us the descriptor is ready to
>      write
>
>   2. we call xwrite(), and our actual write() call returns a partial
>      write (i.e., reports "ret" bytes < "len" we passed in)
>
>   3. we return back to pump_io() do another round of poll(). If the
>      other side consumed some bytes from the pipe, then we may get
>      triggered to do another (possibly partial) write. If it didn't, and
>      we'd get EAGAIN writing, then poll shouldn't trigger at all!
>
> So it's weird that you'd see EAGAIN in this instance. Either the
> underlying write() is refusing to do a partial write (and just returning
> an error with EAGAIN in the first place), or the poll emulation is wrong
> (telling us the descriptor is ready for writing when it isn't).

You're right, Windows' write needs two corrections.  The helper below
reports what happens when we feed a pipe with writes of different sizes.
On Debian on WSL 2 (Windows Subsystem for Linux) it says:

   chunk size: 1 bytes
   65536 total bytes written, then got EAGAIN
   chunk size: 1000 bytes
   64000 total bytes written, then got EAGAIN
   chunk size: 1024 bytes
   65536 total bytes written, then got EAGAIN
   chunk size: 100000 bytes
   0 total bytes written, then got a partial write of 65536 bytes
   65536 total bytes written, then got EAGAIN

On Windows directly I get:

   chunk size: 1 bytes
   8192 total bytes written, then got ENOSPC
   chunk size: 1000 bytes
   8000 total bytes written, then got ENOSPC
   chunk size: 1024 bytes
   8192 total bytes written, then got ENOSPC
   chunk size: 100000 bytes
   0 total bytes written, then got ENOSPC

https://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
documents what we should get: Writes smaller than the buffer should
be atomic, bigger writes bigger should be broken up, and the error code
for a full buffer should be EAGAIN.  I.e. the first example is right.
So mingw_write() needs to translate ENOSPC to EAGAIN and break up huge
writes instead of giving up outright.

> Can you instrument pump_io_round() (or use some strace equivalent, if
> there is one) to see if we do a successful partial write first (which
> implies poll() is wrong in telling us we can write more for the second
> round), or if the very first write() is failing (which implies write()
> is wrong for returning EAGAIN when it could do a partial write).

The two corrections mentioned above together with the enable_nonblock()
implementation for Windows (and the removal of "false") suffice to let
t3701 pass when started directly, but it still hangs when running the
whole test suite using prove.

I don't have time to investigate right now, but I still don't
understand how xwrite() can possibly work against a non-blocking pipe.
It loops on EAGAIN, which is bad if the only way forward is to read
from a different fd to allow the other process to drain the pipe
buffer so that xwrite() can write again.  I suspect pump_io_round()
must not use xwrite() and should instead handle EAGAIN by skipping to
the next fd.

René

---
 Makefile                 |  1 +
 t/helper/test-nonblock.c | 51 ++++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c     |  1 +
 t/helper/test-tool.h     |  1 +
 4 files changed, 54 insertions(+)
 create mode 100644 t/helper/test-nonblock.c

diff --git a/Makefile b/Makefile
index d9c00cc05d..0bc028ca00 100644
--- a/Makefile
+++ b/Makefile
@@ -751,6 +751,7 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
 TEST_BUILTINS_OBJS += test-match-trees.o
 TEST_BUILTINS_OBJS += test-mergesort.o
 TEST_BUILTINS_OBJS += test-mktemp.o
+TEST_BUILTINS_OBJS += test-nonblock.o
 TEST_BUILTINS_OBJS += test-oid-array.o
 TEST_BUILTINS_OBJS += test-oidmap.o
 TEST_BUILTINS_OBJS += test-oidtree.o
diff --git a/t/helper/test-nonblock.c b/t/helper/test-nonblock.c
new file mode 100644
index 0000000000..c9288ea6ac
--- /dev/null
+++ b/t/helper/test-nonblock.c
@@ -0,0 +1,51 @@
+#include "test-tool.h"
+#include "compat/nonblock.h"
+
+static void fill_pipe(size_t write_len)
+{
+	void *buf = xcalloc(1, write_len);
+	int fds[2];
+	size_t total_written = 0;
+	int last = 0;
+
+	if (pipe(fds))
+		die("pipe failed");
+	if (enable_nonblock(fds[1]))
+		die("enable_nonblock failed");
+
+	printf("chunk size: %"PRIuMAX" bytes\n", write_len);
+	for (;;) {
+		ssize_t written = write(fds[1], buf, write_len);
+		if (written != write_len)
+			printf("%"PRIuMAX" total bytes written, then got ",
+			       (uintmax_t)total_written);
+		if (written < 0) {
+			switch (errno) {
+			case EAGAIN: printf("EAGAIN\n"); break;
+			case ENOSPC: printf("ENOSPC\n"); break;
+			default: printf("errno %d\n", errno);
+			}
+			break;
+		} else if (written != write_len)
+			printf("a partial write of %"PRIuMAX" bytes\n",
+			       (uintmax_t)written);
+		if (last)
+			break;
+		if (written > 0)
+			total_written += written;
+		last = !written;
+	};
+
+	close(fds[1]);
+	close(fds[0]);
+	free(buf);
+}
+
+int cmd__nonblock(int argc, const char **argv)
+{
+	fill_pipe(1);
+	fill_pipe(1000);
+	fill_pipe(1024);
+	fill_pipe(100000);
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 318fdbab0c..562d7a9161 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -45,6 +45,7 @@ static struct test_cmd cmds[] = {
 	{ "match-trees", cmd__match_trees },
 	{ "mergesort", cmd__mergesort },
 	{ "mktemp", cmd__mktemp },
+	{ "nonblock", cmd__nonblock },
 	{ "oid-array", cmd__oid_array },
 	{ "oidmap", cmd__oidmap },
 	{ "oidtree", cmd__oidtree },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index bb79927163..d9006a5298 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -36,6 +36,7 @@ int cmd__lazy_init_name_hash(int argc, const char **argv);
 int cmd__match_trees(int argc, const char **argv);
 int cmd__mergesort(int argc, const char **argv);
 int cmd__mktemp(int argc, const char **argv);
+int cmd__nonblock(int argc, const char **argv);
 int cmd__oidmap(int argc, const char **argv);
 int cmd__oidtree(int argc, const char **argv);
 int cmd__online_cpus(int argc, const char **argv);
--
2.37.1

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King]

On Wed, Aug 10, 2022 at 07:39:34AM +0200, René Scharfe wrote:

> > So it's weird that you'd see EAGAIN in this instance. Either the
> > underlying write() is refusing to do a partial write (and just returning
> > an error with EAGAIN in the first place), or the poll emulation is wrong
> > (telling us the descriptor is ready for writing when it isn't).
> 
> You're right, Windows' write needs two corrections.  The helper below
> reports what happens when we feed a pipe with writes of different sizes.
> On Debian on WSL 2 (Windows Subsystem for Linux) it says:
> [...]

Thanks for digging into this further. What you found makes sense to me
and explains what we're seeing.

> The two corrections mentioned above together with the enable_nonblock()
> implementation for Windows (and the removal of "false") suffice to let
> t3701 pass when started directly, but it still hangs when running the
> whole test suite using prove.

Interesting. I wish there was an easy way for me to poke at this, too. I
tried installing the Git for Windows SDK under wine, but unsurprisingly
it did not get very far.

Possibly I could try connecting to a running CI instance, but the test
did not seem to fail there! (Plus I'd have to figure out how to do
that... ;) ).

> I don't have time to investigate right now, but I still don't
> understand how xwrite() can possibly work against a non-blocking pipe.
> It loops on EAGAIN, which is bad if the only way forward is to read
> from a different fd to allow the other process to drain the pipe
> buffer so that xwrite() can write again.  I suspect pump_io_round()
> must not use xwrite() and should instead handle EAGAIN by skipping to
> the next fd.

Right, it's susceptible to looping forever in such a case. _But_ a
blocking write is likewise susceptible to blocking forever. In either
case, we're relying on the reading side to pull some bytes out of the
pipe so we can make forward progress.

The key thing is that pump_io() is careful never to initiate a write()
unless poll() has just told us that the descriptor is ready for writing.

If something unexpected happens there (i.e., the descriptor is not
really ready), a blocking descriptor is going to be stuck. And with
xwrite(), we're similarly stuck (just looping instead of blocking).
Without xwrite(), a non-blocking one _could_ be better off, because that
EAGAIN would make it up to pump_io(). But what is it supposed to do? I
guess it could go back into its main loop and hope that whatever bug
caused the mismatch between poll() and write() goes away.

But even that would not have fixed the problem here on Windows. From my
understanding, mingw_write() in this case would never write _any_ bytes.
So we'd never make forward progress, and just loop writing 0 bytes and
returning EAGAIN over and over.

So I dunno. We could try to be a bit more defensive about non-blocking
descriptors by avoiding xwrite() in this instance, but it only helps for
a particular class of weird OS behavior/bugs. I'd prefer to see a real
case that it would help before moving in that direction.

-Peff

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[René Scharfe]

Am 10.08.2022 um 21:53 schrieb Jeff King:
> On Wed, Aug 10, 2022 at 07:39:34AM +0200, René Scharfe wrote:
>
>>> So it's weird that you'd see EAGAIN in this instance. Either the
>>> underlying write() is refusing to do a partial write (and just returning
>>> an error with EAGAIN in the first place), or the poll emulation is wrong
>>> (telling us the descriptor is ready for writing when it isn't).
>>
>> You're right, Windows' write needs two corrections.  The helper below
>> reports what happens when we feed a pipe with writes of different sizes.
>> On Debian on WSL 2 (Windows Subsystem for Linux) it says:
>> [...]
>
> Thanks for digging into this further. What you found makes sense to me
> and explains what we're seeing.
>
>> The two corrections mentioned above together with the enable_nonblock()
>> implementation for Windows (and the removal of "false") suffice to let
>> t3701 pass when started directly, but it still hangs when running the
>> whole test suite using prove.
>
> Interesting. I wish there was an easy way for me to poke at this, too. I
> tried installing the Git for Windows SDK under wine, but unsurprisingly
> it did not get very far.
>
> Possibly I could try connecting to a running CI instance, but the test
> did not seem to fail there! (Plus I'd have to figure out how to do
> that... ;) ).
>
>> I don't have time to investigate right now, but I still don't
>> understand how xwrite() can possibly work against a non-blocking pipe.
>> It loops on EAGAIN, which is bad if the only way forward is to read
>> from a different fd to allow the other process to drain the pipe
>> buffer so that xwrite() can write again.  I suspect pump_io_round()
>> must not use xwrite() and should instead handle EAGAIN by skipping to
>> the next fd.
>
> Right, it's susceptible to looping forever in such a case. _But_ a
> blocking write is likewise susceptible to blocking forever. In either
> case, we're relying on the reading side to pull some bytes out of the
> pipe so we can make forward progress.
>
> The key thing is that pump_io() is careful never to initiate a write()
> unless poll() has just told us that the descriptor is ready for writing.

Right, and Windows breaks it by refusing to write data bigger than the
buffer even if it's empty.

What does "ready for writing" mean?  PIPE_BUF bytes are free, right?

> If something unexpected happens there (i.e., the descriptor is not
> really ready), a blocking descriptor is going to be stuck. And with
> xwrite(), we're similarly stuck (just looping instead of blocking).
> Without xwrite(), a non-blocking one _could_ be better off, because that
> EAGAIN would make it up to pump_io(). But what is it supposed to do? I
> guess it could go back into its main loop and hope that whatever bug
> caused the mismatch between poll() and write() goes away.

It should check other fds to let the other side make some progress on
them, so that it eventually gets to drain the pipe we want to write to.

> But even that would not have fixed the problem here on Windows. From my
> understanding, mingw_write() in this case would never write _any_ bytes.
> So we'd never make forward progress, and just loop writing 0 bytes and
> returning EAGAIN over and over.

Right, we need to teach it to break up large writes.  It must make at
least some progress, if possible.

> So I dunno. We could try to be a bit more defensive about non-blocking
> descriptors by avoiding xwrite() in this instance, but it only helps for
> a particular class of weird OS behavior/bugs. I'd prefer to see a real
> case that it would help before moving in that direction.

Makes sense.

René

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King]

On Thu, Aug 11, 2022 at 12:35:02AM +0200, René Scharfe wrote:

> > The key thing is that pump_io() is careful never to initiate a write()
> > unless poll() has just told us that the descriptor is ready for writing.
> 
> Right, and Windows breaks it by refusing to write data bigger than the
> buffer even if it's empty.
> 
> What does "ready for writing" mean?  PIPE_BUF bytes are free, right?

According to POSIX, it is "a write() will not block". ;)

I think in practice for Unix systems it means that at least 1 byte is
available.

> > If something unexpected happens there (i.e., the descriptor is not
> > really ready), a blocking descriptor is going to be stuck. And with
> > xwrite(), we're similarly stuck (just looping instead of blocking).
> > Without xwrite(), a non-blocking one _could_ be better off, because that
> > EAGAIN would make it up to pump_io(). But what is it supposed to do? I
> > guess it could go back into its main loop and hope that whatever bug
> > caused the mismatch between poll() and write() goes away.
> 
> It should check other fds to let the other side make some progress on
> them, so that it eventually gets to drain the pipe we want to write to.

Yes, that "go back into its main loop" would do that. But that's not
guaranteed to produce progress any time soon. We may just busy-loop
between poll() and write() returning EAGAIN if another descriptor isn't
ready. And if the thing preventing further writes is something the
process on the other side of the pipe can't fix (and thus we in turn
can't help it fix things by doing a read()), then we'll loop forever.
Which (if I understand it) is exactly the case here because the
underlying code refused to do a partial write.

-Peff

[PATCH] mingw: handle writes to non-blocking pipe

[René Scharfe]

write() on Windows reports ENOSPC when writing to a non-blocking pipe
whose buffer is full and rejects writes bigger than the buffer outright.
Change the error code to EAGAIN and try a buffer-sized partial write to
comply with POSIX and the expections of our Git-internal callers.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/mingw.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index b5502997e2..c6f244c0fe 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -689,6 +689,8 @@ int mingw_fflush(FILE *stream)
 	return ret;
 }

+#define PIPE_BUFFER_SIZE (8192)
+
 #undef write
 ssize_t mingw_write(int fd, const void *buf, size_t len)
 {
@@ -702,6 +704,21 @@ ssize_t mingw_write(int fd, const void *buf, size_t len)
 		else
 			errno = EINVAL;
 	}
+	if (result < 0 && errno == ENOSPC) {
+		/* check if fd is a non-blocking pipe */
+		HANDLE h = (HANDLE) _get_osfhandle(fd);
+		DWORD s;
+		if (GetFileType(h) == FILE_TYPE_PIPE &&
+		    GetNamedPipeHandleState(h, &s, NULL, NULL, NULL, NULL, 0) &&
+		    (s & PIPE_NOWAIT)) {
+			DWORD obuflen;
+			if (!GetNamedPipeInfo(h, NULL, &obuflen, NULL, NULL))
+				obuflen = PIPE_BUFFER_SIZE;
+			if (len > obuflen)
+				return mingw_write(fd, buf, obuflen);
+			errno = EAGAIN;
+		}
+	}

 	return result;
 }
@@ -1079,7 +1096,7 @@ int pipe(int filedes[2])
 	HANDLE h[2];

 	/* this creates non-inheritable handles */
-	if (!CreatePipe(&h[0], &h[1], NULL, 8192)) {
+	if (!CreatePipe(&h[0], &h[1], NULL, PIPE_BUFFER_SIZE)) {
 		errno = err_win_to_posix(GetLastError());
 		return -1;
 	}
--
2.37.1.windows.1

Re: [PATCH] mingw: handle writes to non-blocking pipe

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2599 bytes --]

Hi René,

On Wed, 10 Aug 2022, René Scharfe wrote:

> write() on Windows reports ENOSPC when writing to a non-blocking pipe
> whose buffer is full and rejects writes bigger than the buffer outright.
> Change the error code to EAGAIN and try a buffer-sized partial write to
> comply with POSIX and the expections of our Git-internal callers.

Excellent analysis, thank you!

However, let's reword this to clarify that the error code is set to EAGAIN
only if the buffer-sized partial write fails.

>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  compat/mingw.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index b5502997e2..c6f244c0fe 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -689,6 +689,8 @@ int mingw_fflush(FILE *stream)
>  	return ret;
>  }
>
> +#define PIPE_BUFFER_SIZE (8192)

This constant hails all the way back from 897bb8cb2c2 (Windows: A pipe()
replacement whose ends are not inherited to children., 2007-12-07), in
case anyone wondered like I did where that number came from (and why it is
so low).

It is outside the purview of this patch to change that constant, therefore
I am fine with leaving this as-is.

> +
>  #undef write
>  ssize_t mingw_write(int fd, const void *buf, size_t len)
>  {
> @@ -702,6 +704,21 @@ ssize_t mingw_write(int fd, const void *buf, size_t len)
>  		else
>  			errno = EINVAL;
>  	}
> +	if (result < 0 && errno == ENOSPC) {

It might make the code clearer to turn this into an `else if`.

> +		/* check if fd is a non-blocking pipe */
> +		HANDLE h = (HANDLE) _get_osfhandle(fd);
> +		DWORD s;
> +		if (GetFileType(h) == FILE_TYPE_PIPE &&
> +		    GetNamedPipeHandleState(h, &s, NULL, NULL, NULL, NULL, 0) &&
> +		    (s & PIPE_NOWAIT)) {
> +			DWORD obuflen;
> +			if (!GetNamedPipeInfo(h, NULL, &obuflen, NULL, NULL))
> +				obuflen = PIPE_BUFFER_SIZE;
> +			if (len > obuflen)
> +				return mingw_write(fd, buf, obuflen);

It is probably easier to reason about to recurse instead of using a `goto`
here.

Thank you for this patch!
Dscho

> +			errno = EAGAIN;
> +		}
> +	}
>
>  	return result;
>  }
> @@ -1079,7 +1096,7 @@ int pipe(int filedes[2])
>  	HANDLE h[2];
>
>  	/* this creates non-inheritable handles */
> -	if (!CreatePipe(&h[0], &h[1], NULL, 8192)) {
> +	if (!CreatePipe(&h[0], &h[1], NULL, PIPE_BUFFER_SIZE)) {
>  		errno = err_win_to_posix(GetLastError());
>  		return -1;
>  	}
> --
> 2.37.1.windows.1
>

Re: [PATCH] mingw: handle writes to non-blocking pipe

[Jeff King]

On Wed, Aug 10, 2022 at 07:39:40AM +0200, René Scharfe wrote:

> write() on Windows reports ENOSPC when writing to a non-blocking pipe
> whose buffer is full and rejects writes bigger than the buffer outright.
> Change the error code to EAGAIN and try a buffer-sized partial write to
> comply with POSIX and the expections of our Git-internal callers.

Bearing in mind that I have no qualifications for reviewing
Windows-specific patches, this seems like the right thing to be doing
from the behavior you saw.

One question:

> +	if (result < 0 && errno == ENOSPC) {
> +		/* check if fd is a non-blocking pipe */
> +		HANDLE h = (HANDLE) _get_osfhandle(fd);
> +		DWORD s;
> +		if (GetFileType(h) == FILE_TYPE_PIPE &&
> +		    GetNamedPipeHandleState(h, &s, NULL, NULL, NULL, NULL, 0) &&
> +		    (s & PIPE_NOWAIT)) {
> +			DWORD obuflen;
> +			if (!GetNamedPipeInfo(h, NULL, &obuflen, NULL, NULL))
> +				obuflen = PIPE_BUFFER_SIZE;
> +			if (len > obuflen)
> +				return mingw_write(fd, buf, obuflen);
> +			errno = EAGAIN;
> +		}

OK, so we call GetNamedPipeInfo() to find the size of the pipe buffer.
It's unclear to me from Microsoft's docs if that is the _total_ size, or
if it's the remaining available size. Hopefully the latter, since none
of this works otherwise. ;)

But two corner cases:

  - If we fail to get the size, we guess that it's the maximum. Is this
    dangerous? I'm not sure why the call would fail, but if for some
    reason it did fail and we can't make forward progress, would we
    enter an infinite recursion of mingw_write()? Would it be safer to
    bail with EAGAIN in such a case (through granted, that probably just
    puts us into an infinite loop in xwrite())?

  - According to the docs:

      If the buffer size is zero, the buffer is allocated as needed.

    If we see this case, we'd then call mingw_write() with 0 bytes,
    which I imagine also makes no forward progress (though maybe we
    eventually return a successful 0-byte write?).

-Peff

Re: [PATCH] mingw: handle writes to non-blocking pipe

[René Scharfe]

Am 10.08.2022 um 22:02 schrieb Jeff King:
> On Wed, Aug 10, 2022 at 07:39:40AM +0200, René Scharfe wrote:
>
>> write() on Windows reports ENOSPC when writing to a non-blocking pipe
>> whose buffer is full and rejects writes bigger than the buffer outright.
>> Change the error code to EAGAIN and try a buffer-sized partial write to
>> comply with POSIX and the expections of our Git-internal callers.
>
> Bearing in mind that I have no qualifications for reviewing
> Windows-specific patches, this seems like the right thing to be doing
> from the behavior you saw.
>
> One question:
>
>> +	if (result < 0 && errno == ENOSPC) {
>> +		/* check if fd is a non-blocking pipe */
>> +		HANDLE h = (HANDLE) _get_osfhandle(fd);
>> +		DWORD s;
>> +		if (GetFileType(h) == FILE_TYPE_PIPE &&
>> +		    GetNamedPipeHandleState(h, &s, NULL, NULL, NULL, NULL, 0) &&
>> +		    (s & PIPE_NOWAIT)) {
>> +			DWORD obuflen;
>> +			if (!GetNamedPipeInfo(h, NULL, &obuflen, NULL, NULL))
>> +				obuflen = PIPE_BUFFER_SIZE;
>> +			if (len > obuflen)
>> +				return mingw_write(fd, buf, obuflen);
>> +			errno = EAGAIN;
>> +		}
>
> OK, so we call GetNamedPipeInfo() to find the size of the pipe buffer.
> It's unclear to me from Microsoft's docs if that is the _total_ size, or
> if it's the remaining available size. Hopefully the latter, since none
> of this works otherwise. ;)
>
> But two corner cases:
>
>   - If we fail to get the size, we guess that it's the maximum. Is this
>     dangerous? I'm not sure why the call would fail, but if for some
>     reason it did fail and we can't make forward progress, would we
>     enter an infinite recursion of mingw_write()? Would it be safer to
>     bail with EAGAIN in such a case (through granted, that probably just
>     puts us into an infinite loop in xwrite())?

AFAIU it's the total size, not the available space.  I think I confused
it with PIPE_BUF, which we should use instead.

Alternatively we could retry with ever smaller sizes, down to one byte,
to avoid EAGAIN as much as possible.  Sounds costly, though.

>
>   - According to the docs:
>
>       If the buffer size is zero, the buffer is allocated as needed.
>
>     If we see this case, we'd then call mingw_write() with 0 bytes,
>     which I imagine also makes no forward progress (though maybe we
>     eventually return a successful 0-byte write?).

Ah, yes, forgot that case.

René

Re: [PATCH] mingw: handle writes to non-blocking pipe

[Jeff King]

On Thu, Aug 11, 2022 at 12:34:46AM +0200, René Scharfe wrote:

> > OK, so we call GetNamedPipeInfo() to find the size of the pipe buffer.
> > It's unclear to me from Microsoft's docs if that is the _total_ size, or
> > if it's the remaining available size. Hopefully the latter, since none
> > of this works otherwise. ;)
> >
> > But two corner cases:
> >
> >   - If we fail to get the size, we guess that it's the maximum. Is this
> >     dangerous? I'm not sure why the call would fail, but if for some
> >     reason it did fail and we can't make forward progress, would we
> >     enter an infinite recursion of mingw_write()? Would it be safer to
> >     bail with EAGAIN in such a case (through granted, that probably just
> >     puts us into an infinite loop in xwrite())?
> 
> AFAIU it's the total size, not the available space.  I think I confused
> it with PIPE_BUF, which we should use instead.

Hmm. If it's giving us the total size, that seems like it may fail in a
case like this:

  - we write a smaller amount to the pipe, say 7168 bytes. That leaves
    1024 bytes in the buffer. The reader reads nothing yet.

  - now we try to write another 4096 bytes. That's too big, so we get
    ENOSPC. But when we ask for the pipe size, it tells us 8192. So we
    _don't_ try to do a partial write of the remaining size, and return
    EAGAIN. But now we've made no forward progress (i.e., even though
    poll() said we could write, we don't, and we end up in the xwrite
    loop).

So we really do want to try to get a partial write. Even a single byte
means we are making forward progress.

> Alternatively we could retry with ever smaller sizes, down to one byte,
> to avoid EAGAIN as much as possible.  Sounds costly, though.

It's definitely not optimal, but it may not be too bad. If we cut the
size in half each time, then at worst we make log2(N) extra write
attempts, and we end up with a partial write within 50% of the optimal
size.

-Peff

Re: [PATCH] mingw: handle writes to non-blocking pipe

[René Scharfe]

Am 11.08.2022 um 10:47 schrieb Jeff King:
> On Thu, Aug 11, 2022 at 12:34:46AM +0200, René Scharfe wrote:
>
>>> OK, so we call GetNamedPipeInfo() to find the size of the pipe buffer.
>>> It's unclear to me from Microsoft's docs if that is the _total_ size, or
>>> if it's the remaining available size. Hopefully the latter, since none
>>> of this works otherwise. ;)
>>>
>>> But two corner cases:
>>>
>>>   - If we fail to get the size, we guess that it's the maximum. Is this
>>>     dangerous? I'm not sure why the call would fail, but if for some
>>>     reason it did fail and we can't make forward progress, would we
>>>     enter an infinite recursion of mingw_write()? Would it be safer to
>>>     bail with EAGAIN in such a case (through granted, that probably just
>>>     puts us into an infinite loop in xwrite())?
>>
>> AFAIU it's the total size, not the available space.  I think I confused
>> it with PIPE_BUF, which we should use instead.
>
> Hmm. If it's giving us the total size, that seems like it may fail in a
> case like this:
>
>   - we write a smaller amount to the pipe, say 7168 bytes. That leaves
>     1024 bytes in the buffer. The reader reads nothing yet.
>
>   - now we try to write another 4096 bytes. That's too big, so we get
>     ENOSPC. But when we ask for the pipe size, it tells us 8192. So we
>     _don't_ try to do a partial write of the remaining size, and return
>     EAGAIN. But now we've made no forward progress (i.e., even though
>     poll() said we could write, we don't, and we end up in the xwrite
>     loop).
>
> So we really do want to try to get a partial write. Even a single byte
> means we are making forward progress.
>
>> Alternatively we could retry with ever smaller sizes, down to one byte,
>> to avoid EAGAIN as much as possible.  Sounds costly, though.
>
> It's definitely not optimal, but it may not be too bad. If we cut the
> size in half each time, then at worst we make log2(N) extra write
> attempts, and we end up with a partial write within 50% of the optimal
> size.

OK, but we can't just split anything up as we like: POSIX wants us to
keep writes up to PIPE_BUF atomic.  When I read that name I mistakenly
thought it was the size of the pipe buffer, but it's a different value.
The minimum value according to POSIX is 512 bytes; on Linux it's 4KB.

And Windows doesn't seem to define it.  Bash's ulimit -p returns 8,
which is in units of 512 bytes, so it's 4KB like on Linux.  But that's
apparently not respected by Windows' write.

I just realized that we can interprete the situation slightly
differently.  Windows has effectively PIPE_BUF = 2^32, which means all
writes are atomic.  I don't see POSIX specifying a maximum allowed
value, so this must be allowed.  Which means you cannot rely on write
performing partial writes.  Makes sense?

If we accept that, then we need a special write function for
non-blocking pipes that chops the data into small enough chunks.

Another oddity: t3701 with yesterday's patch finishes fine with -i, but
hangs without it (not just when running it via prove).  O_o

René

Re: [PATCH] mingw: handle writes to non-blocking pipe

[Jeff King]

On Thu, Aug 11, 2022 at 07:35:33PM +0200, René Scharfe wrote:

> OK, but we can't just split anything up as we like: POSIX wants us to
> keep writes up to PIPE_BUF atomic.  When I read that name I mistakenly
> thought it was the size of the pipe buffer, but it's a different value.
> The minimum value according to POSIX is 512 bytes; on Linux it's 4KB.
> 
> And Windows doesn't seem to define it.  Bash's ulimit -p returns 8,
> which is in units of 512 bytes, so it's 4KB like on Linux.  But that's
> apparently not respected by Windows' write.
> 
> I just realized that we can interprete the situation slightly
> differently.  Windows has effectively PIPE_BUF = 2^32, which means all
> writes are atomic.  I don't see POSIX specifying a maximum allowed
> value, so this must be allowed.  Which means you cannot rely on write
> performing partial writes.  Makes sense?

Hmm, I'm not sure. POSIX does allow writing a single byte if that's all
the space there is, but only if the original _request_ was for more than
PIPE_BUF. Which I guess matches what you're saying.

If the original request is smaller than PIPE_BUF, it does seem to say
that EAGAIN is the correct output. But it also says:

  There is no exception regarding partial writes when O_NONBLOCK is set.
  With the exception of writing to an empty pipe, this volume of
  POSIX.1-2017 does not specify exactly when a partial write is
  performed since that would require specifying internal details of the
  implementation. Every application should be prepared to handle partial
  writes when O_NONBLOCK is set and the requested amount is greater than
  {PIPE_BUF}, just as every application should be prepared to handle
  partial writes on other kinds of file descriptors.

  The intent of forcing writing at least one byte if any can be written
  is to assure that each write makes progress if there is any room in
  the pipe. If the pipe is empty, {PIPE_BUF} bytes must be written; if
  not, at least some progress must have been made.

So they are clearly aware of the "we need to make forward progress"
problem. But how are you supposed to do that if you only have less than
PIPE_BUF bytes left to write? And likewise, even if it is technically
legal, having a PIPE_BUF of 2^32 seems like a quality-of-implementation
issue.

And that doesn't really match what poll() is saying. The response from
poll() told us we could write without blocking, which implies at least
PIPE_BUF bytes available. But clearly it isn't available, since the
write fails (with EAGAIN, but that is equivalent to blocking in POSIX's
view here).

Lawyering aside, I think it would be OK to move forward with cutting up
writes at least to a size of 512 bytes. Git runs on lots of platforms,
and none of the code can make an assumption that PIPE_BUF is larger than
that. I.e., we are reducing atomicity provided by Windows, but that's
OK.

I don't think that solves our problem fully, though. We might need to
write 5 bytes, and telling mingw_write() to do so means it's supposed to
abide by PIPE_BUF conventions. But again, we are in control of the
calling code here. I don't think there's any reason that we care about
PIPE_BUF atomicity. Certainly we don't get those atomicity guarantees on
the socket side, which is where much of our writing happens, and our
code is prepared to handle partial writes of any size. So we could just
ignore that guarantee here.

> If we accept that, then we need a special write function for
> non-blocking pipes that chops the data into small enough chunks.

I'm not sure what "small enough" we can rely on, though. Really it is
the interplay between poll() and write() that we care about here. We
would like to know at what point poll() will tell us it's OK to write().
But we don't know what the OS thinks of that.

Or maybe we do, since I think poll() is our own compat layer. But it
just seems to be based on select(). We do seem to use PeekNamedPipe()
there to look on the reading side, but I don't know if there's an
equivalent for writing.

> Another oddity: t3701 with yesterday's patch finishes fine with -i, but
> hangs without it (not just when running it via prove).  O_o

Yes, definitely strange. I'd expect weirdness with "-v", for example,
because of terminal stuff, but "-i" should have no impact on the running
environment.

-Peff

Re: [PATCH] mingw: handle writes to non-blocking pipe

[René Scharfe]

Am 11.08.2022 um 20:20 schrieb Jeff King:
> On Thu, Aug 11, 2022 at 07:35:33PM +0200, René Scharfe wrote:
>
>> OK, but we can't just split anything up as we like: POSIX wants us to
>> keep writes up to PIPE_BUF atomic.  When I read that name I mistakenly
>> thought it was the size of the pipe buffer, but it's a different value.
>> The minimum value according to POSIX is 512 bytes; on Linux it's 4KB.
>>
>> And Windows doesn't seem to define it.  Bash's ulimit -p returns 8,
>> which is in units of 512 bytes, so it's 4KB like on Linux.  But that's
>> apparently not respected by Windows' write.
>>
>> I just realized that we can interprete the situation slightly
>> differently.  Windows has effectively PIPE_BUF = 2^32, which means all
>> writes are atomic.  I don't see POSIX specifying a maximum allowed
>> value, so this must be allowed.  Which means you cannot rely on write
>> performing partial writes.  Makes sense?
>
> Hmm, I'm not sure. POSIX does allow writing a single byte if that's all
> the space there is, but only if the original _request_ was for more than
> PIPE_BUF. Which I guess matches what you're saying.
>
> If the original request is smaller than PIPE_BUF, it does seem to say
> that EAGAIN is the correct output. But it also says:
>
>   There is no exception regarding partial writes when O_NONBLOCK is set.
>   With the exception of writing to an empty pipe, this volume of
>   POSIX.1-2017 does not specify exactly when a partial write is
>   performed since that would require specifying internal details of the
>   implementation. Every application should be prepared to handle partial
>   writes when O_NONBLOCK is set and the requested amount is greater than
>   {PIPE_BUF}, just as every application should be prepared to handle
>   partial writes on other kinds of file descriptors.
>
>   The intent of forcing writing at least one byte if any can be written
>   is to assure that each write makes progress if there is any room in
>   the pipe. If the pipe is empty, {PIPE_BUF} bytes must be written; if
>   not, at least some progress must have been made.
>
> So they are clearly aware of the "we need to make forward progress"
> problem. But how are you supposed to do that if you only have less than
> PIPE_BUF bytes left to write? And likewise, even if it is technically
> legal, having a PIPE_BUF of 2^32 seems like a quality-of-implementation
> issue.
>
> And that doesn't really match what poll() is saying. The response from
> poll() told us we could write without blocking, which implies at least
> PIPE_BUF bytes available. But clearly it isn't available, since the
> write fails (with EAGAIN, but that is equivalent to blocking in POSIX's
> view here).

Turns out that's not the case on Windows: 94f4d01932 (mingw: workaround
for hangs when sending STDIN, 2020-02-17) changed the compatibility
implementation to 'Make `poll()` always reply "writable" for write end
of the pipe.'.

An updated version of the test helper confirms it (patch below) by
reporting on Windows:

chunk size: 1 bytes
 EAGAIN after 8192 bytes
chunk size: 500 bytes
 EAGAIN after 8000 bytes
chunk size: 1000 bytes
 EAGAIN after 8000 bytes
chunk size: 1024 bytes
 EAGAIN after 8192 bytes
chunk size: 100000 bytes
 partial write of 8192 bytes after 0 bytes
 EAGAIN after 8192 bytes

On Debian I get this instead:

chunk size: 1 bytes
 POLLOUT gone after 61441 bytes
 EAGAIN after 65536 bytes
chunk size: 500 bytes
 POLLOUT gone after 60500 bytes
 EAGAIN after 64000 bytes
chunk size: 1000 bytes
 POLLOUT gone after 61000 bytes
 EAGAIN after 64000 bytes
chunk size: 1024 bytes
 POLLOUT gone after 62464 bytes
 EAGAIN after 65536 bytes
chunk size: 100000 bytes
 partial write of 65536 bytes after 0 bytes
 POLLOUT gone after 65536 bytes
 EAGAIN after 65536 bytes

So on Windows the POLLOUT bit is indeed never unset.

> Lawyering aside, I think it would be OK to move forward with cutting up
> writes at least to a size of 512 bytes. Git runs on lots of platforms,
> and none of the code can make an assumption that PIPE_BUF is larger than
> that. I.e., we are reducing atomicity provided by Windows, but that's
> OK.
>
> I don't think that solves our problem fully, though. We might need to
> write 5 bytes, and telling mingw_write() to do so means it's supposed to
> abide by PIPE_BUF conventions. But again, we are in control of the
> calling code here. I don't think there's any reason that we care about
> PIPE_BUF atomicity. Certainly we don't get those atomicity guarantees on
> the socket side, which is where much of our writing happens, and our
> code is prepared to handle partial writes of any size. So we could just
> ignore that guarantee here.
>
>> If we accept that, then we need a special write function for
>> non-blocking pipes that chops the data into small enough chunks.
>
> I'm not sure what "small enough" we can rely on, though. Really it is
> the interplay between poll() and write() that we care about here. We
> would like to know at what point poll() will tell us it's OK to write().
> But we don't know what the OS thinks of that.

Based on the output above I think Linux' poll() won't consider a pipe
writable that has less than PIPE_BUF (4096) available bytes.

> Or maybe we do, since I think poll() is our own compat layer. But it
> just seems to be based on select(). We do seem to use PeekNamedPipe()
> there to look on the reading side, but I don't know if there's an
> equivalent for writing.

This has been elusive so far (see 94f4d01932).

Perhaps we should take the advice about PIPE_NOWAIT in the docs serious
and use overlapping (asynchronous) writes on Windows instead.  This
would mean reimplementing the whole pipe_command() with Windows API
commands, I imagine.

>> Another oddity: t3701 with yesterday's patch finishes fine with -i, but
>> hangs without it (not just when running it via prove).  O_o
>
> Yes, definitely strange. I'd expect weirdness with "-v", for example,
> because of terminal stuff, but "-i" should have no impact on the running
> environment.
It's especially grating because test runs also take very looong.

Avoiding xwrite() in pump_io_round() on top lets the test suite
finish successfully.

René


---
 Makefile                 |  1 +
 t/helper/test-nonblock.c | 73 ++++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c     |  1 +
 t/helper/test-tool.h     |  1 +
 4 files changed, 76 insertions(+)
 create mode 100644 t/helper/test-nonblock.c

diff --git a/Makefile b/Makefile
index d9c00cc05d..0bc028ca00 100644
--- a/Makefile
+++ b/Makefile
@@ -751,6 +751,7 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
 TEST_BUILTINS_OBJS += test-match-trees.o
 TEST_BUILTINS_OBJS += test-mergesort.o
 TEST_BUILTINS_OBJS += test-mktemp.o
+TEST_BUILTINS_OBJS += test-nonblock.o
 TEST_BUILTINS_OBJS += test-oid-array.o
 TEST_BUILTINS_OBJS += test-oidmap.o
 TEST_BUILTINS_OBJS += test-oidtree.o
diff --git a/t/helper/test-nonblock.c b/t/helper/test-nonblock.c
new file mode 100644
index 0000000000..c9350ce894
--- /dev/null
+++ b/t/helper/test-nonblock.c
@@ -0,0 +1,73 @@
+#include "test-tool.h"
+#include "compat/nonblock.h"
+
+static void fill_pipe(size_t write_len)
+{
+	void *buf = xcalloc(1, write_len);
+	int fds[2];
+	size_t total_written = 0;
+	int last = 0;
+	struct pollfd pfd;
+	int writable = 1;
+
+	if (pipe(fds))
+		die_errno("pipe failed");
+	if (enable_nonblock(fds[1]))
+		die_errno("enable_nonblock failed");
+	pfd.fd = fds[1];
+	pfd.events = POLLOUT;
+
+	printf("chunk size: %"PRIuMAX" bytes\n", write_len);
+	for (;;) {
+		ssize_t written;
+		if (poll(&pfd, 1, 0) < 0) {
+			if (errno == EINTR)
+				continue;
+			die_errno("poll failed");
+		}
+		if (writable && !(pfd.revents & POLLOUT)) {
+			writable = 0;
+			printf(" POLLOUT gone after %"PRIuMAX" bytes\n",
+			       total_written);
+		}
+		written = write(fds[1], buf, write_len);
+		if (written < 0) {
+			switch (errno) {
+			case EAGAIN:
+				printf(" EAGAIN after %"PRIuMAX" bytes\n",
+				       total_written);
+				break;
+			case ENOSPC:
+				printf(" ENOSPC after %"PRIuMAX" bytes\n",
+				       total_written);
+				break;
+			default:
+				printf(" errno %d after %"PRIuMAX" bytes\n",
+				       errno, total_written);
+			}
+			break;
+		} else if (written != write_len)
+			printf(" partial write of %"PRIuMAX" bytes"
+			       " after %"PRIuMAX" bytes\n",
+			       (uintmax_t)written, total_written);
+		if (last)
+			break;
+		if (written > 0)
+			total_written += written;
+		last = !written;
+	};
+
+	close(fds[1]);
+	close(fds[0]);
+	free(buf);
+}
+
+int cmd__nonblock(int argc, const char **argv)
+{
+	fill_pipe(1);
+	fill_pipe(500);
+	fill_pipe(1000);
+	fill_pipe(1024);
+	fill_pipe(100000);
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 318fdbab0c..562d7a9161 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -45,6 +45,7 @@ static struct test_cmd cmds[] = {
 	{ "match-trees", cmd__match_trees },
 	{ "mergesort", cmd__mergesort },
 	{ "mktemp", cmd__mktemp },
+	{ "nonblock", cmd__nonblock },
 	{ "oid-array", cmd__oid_array },
 	{ "oidmap", cmd__oidmap },
 	{ "oidtree", cmd__oidtree },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index bb79927163..d9006a5298 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -36,6 +36,7 @@ int cmd__lazy_init_name_hash(int argc, const char **argv);
 int cmd__match_trees(int argc, const char **argv);
 int cmd__mergesort(int argc, const char **argv);
 int cmd__mktemp(int argc, const char **argv);
+int cmd__nonblock(int argc, const char **argv);
 int cmd__oidmap(int argc, const char **argv);
 int cmd__oidtree(int argc, const char **argv);
 int cmd__online_cpus(int argc, const char **argv);
--
2.37.1.windows.1

Re: [PATCH] mingw: handle writes to non-blocking pipe

[Jeff King]

On Sun, Aug 14, 2022 at 05:37:08PM +0200, René Scharfe wrote:

> Turns out that's not the case on Windows: 94f4d01932 (mingw: workaround
> for hangs when sending STDIN, 2020-02-17) changed the compatibility
> implementation to 'Make `poll()` always reply "writable" for write end
> of the pipe.'.

Ah, good find. That kind of explains everything, then, I think. ;)

> > I'm not sure what "small enough" we can rely on, though. Really it is
> > the interplay between poll() and write() that we care about here. We
> > would like to know at what point poll() will tell us it's OK to write().
> > But we don't know what the OS thinks of that.
> 
> Based on the output above I think Linux' poll() won't consider a pipe
> writable that has less than PIPE_BUF (4096) available bytes.

Right, that makes sense. It would have to in order to meet the atomicity
requirement for write(), but still always make forward progress for each
write().

> Perhaps we should take the advice about PIPE_NOWAIT in the docs serious
> and use overlapping (asynchronous) writes on Windows instead.  This
> would mean reimplementing the whole pipe_command() with Windows API
> commands, I imagine.

I wouldn't be opposed to that, in the sense that it's supposed to be a
black box to the caller, and it's relatively small in size. But I think
we're pretty close to having something usable without that, so I'd like
to pursue a smaller fix in the interim.

> Avoiding xwrite() in pump_io_round() on top lets the test suite
> finish successfully.

That makes sense. We end up busy-looping between poll() and write()
while we wait for our read descriptor to become available. But if poll()
doesn't block, that's the best we can do.

-Peff

[PATCH v2 0/6] fix pipe_command() deadlock

[Jeff King]

On Wed, Aug 17, 2022 at 01:39:32AM -0400, Jeff King wrote:

> I wouldn't be opposed to that, in the sense that it's supposed to be a
> black box to the caller, and it's relatively small in size. But I think
> we're pretty close to having something usable without that, so I'd like
> to pursue a smaller fix in the interim.

So here's what I came up with. Most of the changes are done as
preparatory steps, so I believe the final commit should just work out of
the box for you, as well as on Windows CI (which I just kicked off).

Here's a breakdown of the patches:

  [1/6]: compat: add function to enable nonblocking pipes
  [2/6]: nonblock: support Windows

This is my compat wrapper from v1 pulled into a separate function, and
then your Windows implementation on top.

  [3/6]: git-compat-util: make MAX_IO_SIZE define globally available
  [4/6]: pipe_command(): avoid xwrite() for writing to pipe

This fixes the "loop forever" problem of using xwrite(), under the
assumption that fixing our Windows poll() emulation is too hard. If we
did fix it, then we could revert these (though keeping them can be
thought of as a defensive measure against future bugs; busy-looping is
less bad than deadlocking).

I _think_ the amount of busy-looping on Windows will be OK here. Any
case that wouldn't deadlock currently is unaffected, and the cases that
are will probably not be too wasteful. More discussion in the commit
message.

  [5/6]: pipe_command(): handle ENOSPC when writing to a pipe

A hack around the ENOSPC problem you found. We could replace this with
your mingw_write() patch, but it looked like that needed some corner
cases to be smoothed a bit.

  [6/6]: pipe_command(): mark stdin descriptor as non-blocking

The actual fix, largely as before.

I don't love patches 3-5, but it seems like a practical path forward.
I'd prefer to go with a minimal fix that removes the existing deadlock,
and then we can do more cleanups on top. Thanks so much for all of your
investigation and testing so far!

 Makefile                   |  1 +
 compat/nonblock.c          | 50 ++++++++++++++++++++++++++++++++++++++
 compat/nonblock.h          |  9 +++++++
 git-compat-util.h          | 22 +++++++++++++++++
 run-command.c              | 33 +++++++++++++++++++++----
 t/t3701-add-interactive.sh | 13 ++++++++++
 wrapper.c                  | 22 -----------------
 7 files changed, 123 insertions(+), 27 deletions(-)
 create mode 100644 compat/nonblock.c
 create mode 100644 compat/nonblock.h

-Peff

[PATCH v2 1/6] compat: add function to enable nonblocking pipes

[Jeff King]

We'd like to be able to make some of our pipes nonblocking so that
poll() can be used effectively, but O_NONBLOCK isn't portable. Let's
introduce a compat wrapper so this can be abstracted for each platform.

The interface is as narrow as possible to let platforms do what's
natural there (rather than having to implement fcntl() and a fake
O_NONBLOCK for example, or having to handle other types of descriptors).

The next commit will add Windows support, at which point we should be
covering all platforms in practice. But if we do find some other
platform without O_NONBLOCK, we'll return ENOSYS. Arguably we could just
trigger a build-time #error in this case, which would catch the problem
earlier. But since we're not planning to use this compat wrapper in many
code paths, a seldom-seen runtime error may be friendlier for such a
platform than blocking compilation completely. Our test suite would
still notice it.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile          |  1 +
 compat/nonblock.c | 23 +++++++++++++++++++++++
 compat/nonblock.h |  9 +++++++++
 3 files changed, 33 insertions(+)
 create mode 100644 compat/nonblock.c
 create mode 100644 compat/nonblock.h

diff --git a/Makefile b/Makefile
index e8adeb09f1..224e193b66 100644
--- a/Makefile
+++ b/Makefile
@@ -918,6 +918,7 @@ LIB_OBJS += combine-diff.o
 LIB_OBJS += commit-graph.o
 LIB_OBJS += commit-reach.o
 LIB_OBJS += commit.o
+LIB_OBJS += compat/nonblock.o
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += compat/zlib-uncompress2.o
diff --git a/compat/nonblock.c b/compat/nonblock.c
new file mode 100644
index 0000000000..b08105a21d
--- /dev/null
+++ b/compat/nonblock.c
@@ -0,0 +1,23 @@
+#include "git-compat-util.h"
+#include "nonblock.h"
+
+#ifdef O_NONBLOCK
+
+int enable_pipe_nonblock(int fd)
+{
+	int flags = fcntl(fd, F_GETFL);
+	if (flags < 0)
+		return -1;
+	flags |= O_NONBLOCK;
+	return fcntl(fd, F_SETFL, flags);
+}
+
+#else
+
+int enable_pipe_nonblock(int fd)
+{
+	errno = ENOSYS;
+	return -1;
+}
+
+#endif
diff --git a/compat/nonblock.h b/compat/nonblock.h
new file mode 100644
index 0000000000..af1a331301
--- /dev/null
+++ b/compat/nonblock.h
@@ -0,0 +1,9 @@
+#ifndef COMPAT_NONBLOCK_H
+#define COMPAT_NONBLOCK_H
+
+/*
+ * Enable non-blocking I/O for the pipe specified by the passed-in descriptor.
+ */
+int enable_pipe_nonblock(int fd);
+
+#endif
-- 
2.37.2.881.gb57357660c

Re: [PATCH v2 1/6] compat: add function to enable nonblocking pipes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We'd like to be able to make some of our pipes nonblocking so that
> poll() can be used effectively, but O_NONBLOCK isn't portable. Let's
> introduce a compat wrapper so this can be abstracted for each platform.
>
> The interface is as narrow as possible to let platforms do what's
> natural there (rather than having to implement fcntl() and a fake
> O_NONBLOCK for example, or having to handle other types of descriptors).
>
> The next commit will add Windows support, at which point we should be
> covering all platforms in practice. But if we do find some other
> platform without O_NONBLOCK, we'll return ENOSYS. Arguably we could just
> trigger a build-time #error in this case, which would catch the problem
> earlier. But since we're not planning to use this compat wrapper in many
> code paths, a seldom-seen runtime error may be friendlier for such a
> platform than blocking compilation completely. Our test suite would
> still notice it.

Very well reasoned.

The only small "huh?" factor was that the name of the helper is not
enable_nonblock(), but singles out pipe as valid thing to work on.
I think it is perfectly fine, given that the current plan we have is
to use this on the pipe with the command we spawn with
pipe_command(), and it probably is even preferrable to explicitly
declare that this is designed to only work with pipes and future
developers who try to abuse it on other kinds of file descriptors
are on their own.  And "pipe" in the name of this helper may be such
a declaration that is strong enough.

Thanks.

Re: [PATCH v2 1/6] compat: add function to enable nonblocking pipes

[Jeff King]

On Wed, Aug 17, 2022 at 01:23:25PM -0700, Junio C Hamano wrote:

> The only small "huh?" factor was that the name of the helper is not
> enable_nonblock(), but singles out pipe as valid thing to work on.
> I think it is perfectly fine, given that the current plan we have is
> to use this on the pipe with the command we spawn with
> pipe_command(), and it probably is even preferrable to explicitly
> declare that this is designed to only work with pipes and future
> developers who try to abuse it on other kinds of file descriptors
> are on their own.  And "pipe" in the name of this helper may be such
> a declaration that is strong enough.

My goal was to explain that "huh" with this bit in the commit message:

> > The interface is as narrow as possible to let platforms do what's
> > natural there (rather than having to implement fcntl() and a fake
> > O_NONBLOCK for example, or having to handle other types of descriptors).

without going into too many details. I do think it would be easier to
explain if the Windows implementation was added in the same commit
(since it is explicitly just about pipes), but I wanted to credit René
as the author there. I'm not sure if it's worth folding them together
and adding a co-author credit instead. Or maybe I should state outright
in this commit message that we're doing it for Windows. ;)

-Peff

[PATCH v2 2/6] nonblock: support Windows

[Jeff King]

From: René Scharfe <l.s.r@web.de>

Implement enable_pipe_nonblock() using the Windows API. This works only
for pipes, but that is sufficient for this limited interface. Despite
the API calls used, it handles both "named" and anonymous pipes from our
pipe() emulation.

Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 compat/nonblock.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/compat/nonblock.c b/compat/nonblock.c
index b08105a21d..9694ebdb1d 100644
--- a/compat/nonblock.c
+++ b/compat/nonblock.c
@@ -12,6 +12,33 @@ int enable_pipe_nonblock(int fd)
 	return fcntl(fd, F_SETFL, flags);
 }
 
+#elif defined(GIT_WINDOWS_NATIVE)
+
+#include "win32.h"
+
+int enable_pipe_nonblock(int fd)
+{
+	HANDLE h = (HANDLE)_get_osfhandle(fd);
+	DWORD mode;
+	DWORD type = GetFileType(h);
+	if (type == FILE_TYPE_UNKNOWN && GetLastError() != NO_ERROR) {
+		errno = EBADF;
+		return -1;
+	}
+	if (type != FILE_TYPE_PIPE)
+		BUG("unsupported file type: %lu", type);
+	if (!GetNamedPipeHandleState(h, &mode, NULL, NULL, NULL, NULL, 0)) {
+		errno = err_win_to_posix(GetLastError());
+		return -1;
+	}
+	mode |= PIPE_NOWAIT;
+	if (!SetNamedPipeHandleState(h, &mode, NULL, NULL)) {
+		errno = err_win_to_posix(GetLastError());
+		return -1;
+	}
+	return 0;
+}
+
 #else
 
 int enable_pipe_nonblock(int fd)
-- 
2.37.2.881.gb57357660c

[PATCH v2 3/6] git-compat-util: make MAX_IO_SIZE define globally available

[Jeff King]

We define MAX_IO_SIZE within wrapper.c, but it's useful for any code
that wants to do a raw write() for whatever reason (say, because they
want different EAGAIN handling). Let's make it available everywhere.

The alternative would be adding xwrite_foo() variants to give callers
more options. But there's really no reason MAX_IO_SIZE needs to be
abstracted away, so this give callers the most flexibility.

Signed-off-by: Jeff King <peff@peff.net>
---
I did actually write xwrite_nonblock() at first, and I could be
persuaded to go in that direction if we prefer. But given that using it
sanely requires the caller to have a poll() loop, I feel like
pipe_command() is already the right level of abstraction.

 git-compat-util.h | 22 ++++++++++++++++++++++
 wrapper.c         | 22 ----------------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 36a25ae252..6aee4d92e7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -998,6 +998,28 @@ static inline unsigned long cast_size_t_to_ulong(size_t a)
 	return (unsigned long)a;
 }
 
+/*
+ * Limit size of IO chunks, because huge chunks only cause pain.  OS X
+ * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in
+ * the absence of bugs, large chunks can result in bad latencies when
+ * you decide to kill the process.
+ *
+ * We pick 8 MiB as our default, but if the platform defines SSIZE_MAX
+ * that is smaller than that, clip it to SSIZE_MAX, as a call to
+ * read(2) or write(2) larger than that is allowed to fail.  As the last
+ * resort, we allow a port to pass via CFLAGS e.g. "-DMAX_IO_SIZE=value"
+ * to override this, if the definition of SSIZE_MAX given by the platform
+ * is broken.
+ */
+#ifndef MAX_IO_SIZE
+# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
+# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
+#  define MAX_IO_SIZE SSIZE_MAX
+# else
+#  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
+# endif
+#endif
+
 #ifdef HAVE_ALLOCA_H
 # include <alloca.h>
 # define xalloca(size)      (alloca(size))
diff --git a/wrapper.c b/wrapper.c
index cfe79bd081..299d6489a6 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -161,28 +161,6 @@ void xsetenv(const char *name, const char *value, int overwrite)
 		die_errno(_("could not setenv '%s'"), name ? name : "(null)");
 }
 
-/*
- * Limit size of IO chunks, because huge chunks only cause pain.  OS X
- * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in
- * the absence of bugs, large chunks can result in bad latencies when
- * you decide to kill the process.
- *
- * We pick 8 MiB as our default, but if the platform defines SSIZE_MAX
- * that is smaller than that, clip it to SSIZE_MAX, as a call to
- * read(2) or write(2) larger than that is allowed to fail.  As the last
- * resort, we allow a port to pass via CFLAGS e.g. "-DMAX_IO_SIZE=value"
- * to override this, if the definition of SSIZE_MAX given by the platform
- * is broken.
- */
-#ifndef MAX_IO_SIZE
-# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
-# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
-#  define MAX_IO_SIZE SSIZE_MAX
-# else
-#  define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
-# endif
-#endif
-
 /**
  * xopen() is the same as open(), but it die()s if the open() fails.
  */
-- 
2.37.2.881.gb57357660c

[PATCH v2 4/6] pipe_command(): avoid xwrite() for writing to pipe

[Jeff King]

If xwrite() sees an EAGAIN response, it will loop forever until the
write succeeds (or encounters a real error). This is due to ef1cf0167a
(xwrite: poll on non-blocking FDs, 2016-06-26), with the idea that we
won't be surprised by a descriptor unexpectedly set as non-blocking.

But that will make things awkward when we do want a non-blocking
descriptor, and a future patch will switch pipe_command() to using one.
In that case, looping on EAGAIN is bad, because the process on the other
end of the pipe may be waiting on us before doing another read() on the
pipe, which would mean we deadlock.

In practice we're not supposed to ever see EAGAIN here, since poll()
will have just told us the descriptor is ready for writing. But our
Windows emulation of poll() will always return "ready" for writing to a
pipe descriptor! This is due to 94f4d01932 (mingw: workaround for hangs
when sending STDIN, 2020-02-17).

Our best bet in that case is to keep handling other descriptors, as any
read() we do may allow the child command to make forward progress (i.e.,
its write() finishes, and then it read()s from its stdin, freeing up
space in the pipe buffer). This means we might busy-loop between poll()
and write() on Windows if the child command is slow to read our input,
but it's much better than the alternative of deadlocking.

In practice, this busy-looping should be rare:

  - for small inputs, we'll just write the whole thing in a single
    write() anyway, non-blocking or not

  - for larger inputs where the child reads input and then processes it
    before writing (e.g., gpg verifying a signature), we may make a few
    extra write() calls that get EAGAIN during the initial write, but
    once it has taken in the whole input, we'll correctly block waiting
    to read back the data.

  - for larger inputs where the child process is streaming output back
    (like a diff filter), we'll likewise see some extra EAGAINs, but
    most of them will be followed immediately by a read(), which will
    let the child command make forward progress.

Of course it won't happen at all for now, since we don't yet use a
non-blocking pipe. This is just preparation for when we do.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/run-command.c b/run-command.c
index 14f17830f5..e078c3046f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1364,12 +1364,24 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
 			continue;
 
 		if (io->type == POLLOUT) {
-			ssize_t len = xwrite(io->fd,
-					     io->u.out.buf, io->u.out.len);
+			ssize_t len;
+
+			/*
+			 * Don't use xwrite() here. It loops forever on EAGAIN,
+			 * and we're in our own poll() loop here.
+			 *
+			 * Note that we lose xwrite()'s handling of MAX_IO_SIZE
+			 * and EINTR, so we have to implement those ourselves.
+			 */
+			len = write(io->fd, io->u.out.buf,
+				    io->u.out.len <= MAX_IO_SIZE ?
+				    io->u.out.len : MAX_IO_SIZE);
 			if (len < 0) {
-				io->error = errno;
-				close(io->fd);
-				io->fd = -1;
+				if (errno != EINTR && errno != EAGAIN) {
+					io->error = errno;
+					close(io->fd);
+					io->fd = -1;
+				}
 			} else {
 				io->u.out.buf += len;
 				io->u.out.len -= len;
-- 
2.37.2.881.gb57357660c

[PATCH v2 5/6] pipe_command(): handle ENOSPC when writing to a pipe

[Jeff King]

When write() to a non-blocking pipe fails because the buffer is full,
POSIX says we should see EAGAIN. But our mingw_write() compat layer on
Windows actually returns ENOSPC for this case. This is probably
something we want to correct, but given that we don't plan to use
non-blocking descriptors in a lot of places, we can work around it by
just catching ENOSPC alongside EAGAIN. If we ever do fix mingw_write(),
then this patch can be reverted.

We don't actually use a non-blocking pipe yet, so this is still just
preparation.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Jeff King <peff@peff.net>
---
Ironically, this ENOSPC bug means that switching away from xwrite() in
the previous patch wasn't necessary (because it's not clever enough to
know that ENOSPC on a pipe means EAGAIN!). But I think handling both
shows the intent, and sets us up better for fixing mingw_write().

 run-command.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index e078c3046f..5fbaa8b5ac 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1377,7 +1377,8 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
 				    io->u.out.len <= MAX_IO_SIZE ?
 				    io->u.out.len : MAX_IO_SIZE);
 			if (len < 0) {
-				if (errno != EINTR && errno != EAGAIN) {
+				if (errno != EINTR && errno != EAGAIN &&
+				    errno != ENOSPC) {
 					io->error = errno;
 					close(io->fd);
 					io->fd = -1;
-- 
2.37.2.881.gb57357660c

Re: [PATCH v2 5/6] pipe_command(): handle ENOSPC when writing to a pipe

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When write() to a non-blocking pipe fails because the buffer is full,
> POSIX says we should see EAGAIN. But our mingw_write() compat layer on
> Windows actually returns ENOSPC for this case. This is probably
> something we want to correct, but given that we don't plan to use
> non-blocking descriptors in a lot of places, we can work around it by
> just catching ENOSPC alongside EAGAIN. If we ever do fix mingw_write(),
> then this patch can be reverted.
>
> We don't actually use a non-blocking pipe yet, so this is still just
> preparation.
>
> Helped-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Ironically, this ENOSPC bug means that switching away from xwrite() in
> the previous patch wasn't necessary (because it's not clever enough to
> know that ENOSPC on a pipe means EAGAIN!). But I think handling both
> shows the intent, and sets us up better for fixing mingw_write().

Yeah, I am impressed by the attention of small details by you two
shown here to split the steps 4/6 and 5/6.  If we consider that this
step is a band-aid we'd be happier if we can remove, perhaps in-code
comment to explain why we deal with ENOSPC here, instead of burying
it only in the log message, would help remind people of the issue
(but of course the patch is good with or without such a tweak, which
is only relevant in the longer term).

>  run-command.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index e078c3046f..5fbaa8b5ac 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1377,7 +1377,8 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
>  				    io->u.out.len <= MAX_IO_SIZE ?
>  				    io->u.out.len : MAX_IO_SIZE);
>  			if (len < 0) {
> -				if (errno != EINTR && errno != EAGAIN) {
> +				if (errno != EINTR && errno != EAGAIN &&
> +				    errno != ENOSPC) {
>  					io->error = errno;
>  					close(io->fd);
>  					io->fd = -1;

Re: [PATCH v2 5/6] pipe_command(): handle ENOSPC when writing to a pipe

[Jeff King]

On Wed, Aug 17, 2022 at 11:57:01AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > When write() to a non-blocking pipe fails because the buffer is full,
> > POSIX says we should see EAGAIN. But our mingw_write() compat layer on
> > Windows actually returns ENOSPC for this case. This is probably
> > something we want to correct, but given that we don't plan to use
> > non-blocking descriptors in a lot of places, we can work around it by
> > just catching ENOSPC alongside EAGAIN. If we ever do fix mingw_write(),
> > then this patch can be reverted.
> >
> > We don't actually use a non-blocking pipe yet, so this is still just
> > preparation.
> >
> > Helped-by: René Scharfe <l.s.r@web.de>
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > Ironically, this ENOSPC bug means that switching away from xwrite() in
> > the previous patch wasn't necessary (because it's not clever enough to
> > know that ENOSPC on a pipe means EAGAIN!). But I think handling both
> > shows the intent, and sets us up better for fixing mingw_write().
> 
> Yeah, I am impressed by the attention of small details by you two
> shown here to split the steps 4/6 and 5/6.  If we consider that this
> step is a band-aid we'd be happier if we can remove, perhaps in-code
> comment to explain why we deal with ENOSPC here, instead of burying
> it only in the log message, would help remind people of the issue
> (but of course the patch is good with or without such a tweak, which
> is only relevant in the longer term).

Yeah, you may be right. I had originally written a quite long comment
here (before I split things up so much), but found in the splitting that
it made more sense to put most of the details into the commit message.

But maybe it's worth squashing this in?

diff --git a/run-command.c b/run-command.c
index 5fbaa8b5ac..065883672b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1372,6 +1372,10 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
 			 *
 			 * Note that we lose xwrite()'s handling of MAX_IO_SIZE
 			 * and EINTR, so we have to implement those ourselves.
+			 *
+			 * We also check for ENOSPC to handle a quirk of
+			 * mingw_write(), which uses that for a full pipe
+			 * instead of EAGAIN.
 			 */
 			len = write(io->fd, io->u.out.buf,
 				    io->u.out.len <= MAX_IO_SIZE ?

-Peff

[PATCH v2 6/6] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King]

Our pipe_command() helper lets you both write to and read from a child
process on its stdin/stdout. It's supposed to work without deadlocks
because we use poll() to check when descriptors are ready for reading or
writing. But there's a bug: if both the data to be written and the data
to be read back exceed the pipe buffer, we'll deadlock.

The issue is that the code assumes that if you have, say, a 2MB buffer
to write and poll() tells you that the pipe descriptor is ready for
writing, that calling:

  write(cmd->in, buf, 2*1024*1024);

will do a partial write, filling the pipe buffer and then returning what
it did write. And that is what it would do on a socket, but not for a
pipe. When writing to a pipe, at least on Linux, it will block waiting
for the child process to read() more. And now we have a potential
deadlock, because the child may be writing back to us, waiting for us to
read() ourselves.

An easy way to trigger this is:

  git -c add.interactive.useBuiltin=true \
      -c interactive.diffFilter=cat \
      checkout -p HEAD~200

The diff against HEAD~200 will be big, and the filter wants to write all
of it back to us (obviously this is a dummy filter, but in the real
world something like diff-highlight would similarly stream back a big
output).

If you set add.interactive.useBuiltin to false, the problem goes away,
because now we're not using pipe_command() anymore (instead, that part
happens in perl). But this isn't a bug in the interactive code at all.
It's the underlying pipe_command() code which is broken, and has been
all along.

We presumably didn't notice because most calls only do input _or_
output, not both. And the few that do both, like gpg calls, may have
large inputs or outputs, but never both at the same time (e.g., consider
signing, which has a large payload but a small signature comes back).

The obvious fix is to put the descriptor into non-blocking mode, and
indeed, that makes the problem go away. Callers shouldn't need to
care, because they never see the descriptor (they hand us a buffer to
feed into it).

The included test fails reliably on Linux without this patch. Curiously,
it doesn't fail in our Windows CI environment, but has been reported to
do so for individual developers. It should pass in any environment after
this patch (courtesy of the compat/ layers added in the last few
commits).

Signed-off-by: Jeff King <peff@peff.net>
---
Same as before, minus the compat/ bits which were already done, and we
can now claim victory on Windows, too.

 run-command.c              | 10 ++++++++++
 t/t3701-add-interactive.sh | 13 +++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/run-command.c b/run-command.c
index 5fbaa8b5ac..5ec3a46dcc 100644
--- a/run-command.c
+++ b/run-command.c
@@ -10,6 +10,7 @@
 #include "config.h"
 #include "packfile.h"
 #include "hook.h"
+#include "compat/nonblock.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -1451,6 +1452,15 @@ int pipe_command(struct child_process *cmd,
 		return -1;
 
 	if (in) {
+		if (enable_pipe_nonblock(cmd->in) < 0) {
+			error_errno("unable to make pipe non-blocking");
+			close(cmd->in);
+			if (out)
+				close(cmd->out);
+			if (err)
+				close(cmd->err);
+			return -1;
+		}
 		io[nr].fd = cmd->in;
 		io[nr].type = POLLOUT;
 		io[nr].u.out.buf = in;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index b354fb39de..3b7df9bed5 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -766,6 +766,19 @@ test_expect_success 'detect bogus diffFilter output' '
 	force_color test_must_fail git add -p <y
 '
 
+test_expect_success 'handle very large filtered diff' '
+	git reset --hard &&
+	# The specific number here is not important, but it must
+	# be large enough that the output of "git diff --color"
+	# fills up the pipe buffer. 10,000 results in ~200k of
+	# colored output.
+	test_seq 10000 >test &&
+	test_config interactive.diffFilter cat &&
+	printf y >y &&
+	force_color git add -p >output 2>&1 <y &&
+	git diff-files --exit-code -- test
+'
+
 test_expect_success 'diff.algorithm is passed to `git diff-files`' '
 	git reset --hard &&
 
-- 
2.37.2.881.gb57357660c

Re: [PATCH v2 0/6] fix pipe_command() deadlock

[Jeff King]

On Wed, Aug 17, 2022 at 02:04:05AM -0400, Jeff King wrote:

> On Wed, Aug 17, 2022 at 01:39:32AM -0400, Jeff King wrote:
> 
> > I wouldn't be opposed to that, in the sense that it's supposed to be a
> > black box to the caller, and it's relatively small in size. But I think
> > we're pretty close to having something usable without that, so I'd like
> > to pursue a smaller fix in the interim.
> 
> So here's what I came up with. Most of the changes are done as
> preparatory steps, so I believe the final commit should just work out of
> the box for you, as well as on Windows CI (which I just kicked off).

The Windows CI did indeed complete successfully, though I'm not too
surprised, since it never hit the blocking/deadlock case in the first
place. I _think_ from our conversation that it should work for you, too,
but please let me know. Fingers crossed. :)

-Peff

Re: [PATCH v2 0/6] fix pipe_command() deadlock

[René Scharfe]

Am 17.08.2022 um 08:04 schrieb Jeff King:
> On Wed, Aug 17, 2022 at 01:39:32AM -0400, Jeff King wrote:
>
>> I wouldn't be opposed to that, in the sense that it's supposed to be a
>> black box to the caller, and it's relatively small in size. But I think
>> we're pretty close to having something usable without that, so I'd like
>> to pursue a smaller fix in the interim.
>
> So here's what I came up with. Most of the changes are done as
> preparatory steps, so I believe the final commit should just work out of
> the box for you, as well as on Windows CI (which I just kicked off).

Indeed: This series passes the test suite for me on Windows 11.

René

Re: [PATCH v2 0/6] fix pipe_command() deadlock

[Jeff King]

On Fri, Aug 19, 2022 at 11:19:08PM +0200, René Scharfe wrote:

> Am 17.08.2022 um 08:04 schrieb Jeff King:
> > On Wed, Aug 17, 2022 at 01:39:32AM -0400, Jeff King wrote:
> >
> >> I wouldn't be opposed to that, in the sense that it's supposed to be a
> >> black box to the caller, and it's relatively small in size. But I think
> >> we're pretty close to having something usable without that, so I'd like
> >> to pursue a smaller fix in the interim.
> >
> > So here's what I came up with. Most of the changes are done as
> > preparatory steps, so I believe the final commit should just work out of
> > the box for you, as well as on Windows CI (which I just kicked off).
> 
> Indeed: This series passes the test suite for me on Windows 11.

Yay. :)

-Peff

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[René Scharfe]

Am 05.08.2022 um 17:36 schrieb Jeff King:
> On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:
> >>  int enable_nonblock(int fd)
>>  {
>> +	DWORD mode;
>> +	HANDLE handle = winansi_get_osfhandle(fd);
>> +	if (!handle)
>> +		return -1;
>> +	if (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))
>> +		return -1;
>> +	if (mode & PIPE_NOWAIT)
>> +		return 0;
>> +	mode |= PIPE_NOWAIT;
>> +	if (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))
>> +		return -1;
>>  	return 0;
>>  }
>
> This looks plausibly correct to me. ;) We might want to change the name
> of the compat layer to enable_pipe_nonblock(), since one assumes from
> the function names this only works for pipes.

Or how about this?  Squashable.  Needs testing.

--- >8 ---
Subject: [PATCH] nonblock: support Windows

Implement enable_nonblock() using the Windows API.  Supports only named
and anonymous pipes for now, which suffices for the current caller.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/nonblock.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/compat/nonblock.c b/compat/nonblock.c
index 897c099010..78923cd2c3 100644
--- a/compat/nonblock.c
+++ b/compat/nonblock.c
@@ -14,9 +14,40 @@ int enable_nonblock(int fd)

 #else

+#ifdef GIT_WINDOWS_NATIVE
+
+#include "win32.h"
+
+int enable_nonblock(int fd)
+{
+	HANDLE h = (HANDLE)_get_osfhandle(fd);
+	DWORD mode;
+	DWORD type = GetFileType(h);
+	if (type == FILE_TYPE_UNKNOWN && GetLastError() != NO_ERROR) {
+		errno = EBADF;
+		return -1;
+	}
+	if (type != FILE_TYPE_PIPE)
+		BUG("unsupported file type: %lu", type);
+	if (!GetNamedPipeHandleState(h, &mode, NULL, NULL, NULL, NULL, 0)) {
+		errno = err_win_to_posix(GetLastError());
+		return -1;
+	}
+	mode |= PIPE_NOWAIT;
+	if (!SetNamedPipeHandleState(h, &mode, NULL, NULL)) {
+		errno = err_win_to_posix(GetLastError());
+		return -1;
+	}
+	return 0;
+}
+
+#else
+
 int enable_nonblock(int fd)
 {
 	return 0;
 }

 #endif
+
+#endif
--
2.37.1.windows.1

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3132 bytes --]

Hi René,

On Sun, 7 Aug 2022, René Scharfe wrote:

> Am 05.08.2022 um 17:36 schrieb Jeff King:
> > On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:
> > >>  int enable_nonblock(int fd)
> >>  {
> >> +	DWORD mode;
> >> +	HANDLE handle = winansi_get_osfhandle(fd);
> >> +	if (!handle)
> >> +		return -1;
> >> +	if (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))
> >> +		return -1;
> >> +	if (mode & PIPE_NOWAIT)
> >> +		return 0;
> >> +	mode |= PIPE_NOWAIT;
> >> +	if (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))
> >> +		return -1;
> >>  	return 0;
> >>  }
> >
> > This looks plausibly correct to me. ;) We might want to change the name
> > of the compat layer to enable_pipe_nonblock(), since one assumes from
> > the function names this only works for pipes.
>
> Or how about this?  Squashable.  Needs testing.
>
> --- >8 ---
> Subject: [PATCH] nonblock: support Windows
>
> Implement enable_nonblock() using the Windows API.  Supports only named
> and anonymous pipes for now, which suffices for the current caller.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  compat/nonblock.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/compat/nonblock.c b/compat/nonblock.c
> index 897c099010..78923cd2c3 100644
> --- a/compat/nonblock.c
> +++ b/compat/nonblock.c
> @@ -14,9 +14,40 @@ int enable_nonblock(int fd)
>
>  #else
>
> +#ifdef GIT_WINDOWS_NATIVE

Maybe use an `#elif defined(GIT_WINDOWS_NATIVE)` here? That would make the
code structures clearer, methinks.

> +
> +#include "win32.h"
> +
> +int enable_nonblock(int fd)
> +{
> +	HANDLE h = (HANDLE)_get_osfhandle(fd);
> +	DWORD mode;
> +	DWORD type = GetFileType(h);
> +	if (type == FILE_TYPE_UNKNOWN && GetLastError() != NO_ERROR) {
> +		errno = EBADF;
> +		return -1;
> +	}
> +	if (type != FILE_TYPE_PIPE)
> +		BUG("unsupported file type: %lu", type);
> +	if (!GetNamedPipeHandleState(h, &mode, NULL, NULL, NULL, NULL, 0)) {
> +		errno = err_win_to_posix(GetLastError());
> +		return -1;
> +	}
> +	mode |= PIPE_NOWAIT;
> +	if (!SetNamedPipeHandleState(h, &mode, NULL, NULL)) {

Nice.

FWIW the documentation of `PIPE_NOWAIT` says:

	Note that nonblocking mode is supported for compatibility with
	Microsoft LAN Manager version 2.0 and should not be used to
	achieve asynchronous input and output (I/O) with named pipes.

(see
https://docs.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-setnamedpipehandlestate
for full details)

There is little more information to be found on the interwebs, the closest
to an in-depth explanation is here:
https://devblogs.microsoft.com/oldnewthing/20110114-00/?p=11753

Even if that comment suggests that this mode is deprecated, I think it is
safe to rely on it in Git's source code.

Thanks,
Dscho

> +		errno = err_win_to_posix(GetLastError());
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +#else
> +
>  int enable_nonblock(int fd)
>  {
>  	return 0;
>  }
>
>  #endif
> +
> +#endif
> --
> 2.37.1.windows.1
>

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Johannes Schindelin]

Hi Peff,

On Tue, 2 Aug 2022, Jeff King wrote:

> diff --git a/run-command.c b/run-command.c
> index 14f17830f5..ed99503b22 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1438,6 +1439,15 @@ int pipe_command(struct child_process *cmd,
>  		return -1;
>
>  	if (in) {
> +		if (enable_nonblock(cmd->in) < 0) {
> +			error_errno("unable to make pipe non-blocking");

It might be a bit heavy-handed to error out in this case, as it usually
does not cause problems. At least that's what the fact suggests to me that
I personally never encountered the dead-lock myself, and neither do I
recall anybody piping more than two megabytes through `git checkout -p`.

Could we turn this into `warning_errno()` and avoid reporting an error
here? We could add "; This might hang the process if large amounts of data
are piped to/from <cmd>" to improve the user experience.

Ciao,
Dscho

> +			close(cmd->in);
> +			if (out)
> +				close(cmd->out);
> +			if (err)
> +				close(cmd->err);
> +			return -1;
> +		}
>  		io[nr].fd = cmd->in;
>  		io[nr].type = POLLOUT;
>  		io[nr].u.out.buf = in;

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King]

On Mon, Aug 08, 2022 at 02:59:49PM +0200, Johannes Schindelin wrote:

> On Tue, 2 Aug 2022, Jeff King wrote:
> 
> > diff --git a/run-command.c b/run-command.c
> > index 14f17830f5..ed99503b22 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -1438,6 +1439,15 @@ int pipe_command(struct child_process *cmd,
> >  		return -1;
> >
> >  	if (in) {
> > +		if (enable_nonblock(cmd->in) < 0) {
> > +			error_errno("unable to make pipe non-blocking");
> 
> It might be a bit heavy-handed to error out in this case, as it usually
> does not cause problems. At least that's what the fact suggests to me that
> I personally never encountered the dead-lock myself, and neither do I
> recall anybody piping more than two megabytes through `git checkout -p`.

That thought crossed my mind, as well, but I'm hesitant to leave a known
bug in place that can cause a deadlock. It would be one thing if we
could muddle through without nonblock in a slower way, but I don't think
we can easily detect this situation after the fact.

So maybe some options are:

  - don't bother with O_NONBLOCK unless the size of the input is over N
    bytes. The trouble there is that it's not clear what N should be.
    It's fcntl(F_GETPIPE_SZ) on Linux, but that's not portable. We could
    possibly come up with a conservative value if we had a ballpark for
    pipe size on Windows. It feels a bit hacky, though.

  - we could actually guess at a deadlock by putting a timeout on the
    poll(). That would also catch hanging or slow filter processes. I
    really hate putting clock-based limits on things, though, as it
    means the tool behaves differently under load. And keep in mind this
    is deep in the pipe_command() code. It happens to only trigger for
    diff filters now, but it may be used in other spots (in fact it
    already is, and it's only the size of current gpg payloads/responses
    that means it doesn't happen to trigger).

Stepping back, though, I think we should consider why we'd see an error
here. I wouldn't expect it to ever fail on a system where O_NONBLOCK was
supported. If we want to make it a silent noop on some platforms, then
we can stick that into the enable_nonblock() function (which is what I
did, but as René showed, that is probably not a good enough solution).

-Peff

Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3049 bytes --]

Hi Peff,

On Tue, 9 Aug 2022, Jeff King wrote:

> On Mon, Aug 08, 2022 at 02:59:49PM +0200, Johannes Schindelin wrote:
>
> > On Tue, 2 Aug 2022, Jeff King wrote:
> >
> > > diff --git a/run-command.c b/run-command.c
> > > index 14f17830f5..ed99503b22 100644
> > > --- a/run-command.c
> > > +++ b/run-command.c
> > > @@ -1438,6 +1439,15 @@ int pipe_command(struct child_process *cmd,
> > >  		return -1;
> > >
> > >  	if (in) {
> > > +		if (enable_nonblock(cmd->in) < 0) {
> > > +			error_errno("unable to make pipe non-blocking");
> >
> > It might be a bit heavy-handed to error out in this case, as it usually
> > does not cause problems. At least that's what the fact suggests to me that
> > I personally never encountered the dead-lock myself, and neither do I
> > recall anybody piping more than two megabytes through `git checkout -p`.

Ugh, I think that my reasoning was flawed, as I somehow based it on the
assumption that `enable_nonblock()` would return -1 on platforms without
O_NONBLOCK. Even if I had read that you fall back to returning 0 on those
platforms.

And only when reading your reply did it occur to me that this was a thinko
on my part.

So I would like to retract my assessment that it is heavy-handed to error
out in this case. It would have been if we had errored out on platforms
without O_NONBLOCK support, but we don't.

Sorry for the noise,
Dscho

>
> That thought crossed my mind, as well, but I'm hesitant to leave a known
> bug in place that can cause a deadlock. It would be one thing if we
> could muddle through without nonblock in a slower way, but I don't think
> we can easily detect this situation after the fact.
>
> So maybe some options are:
>
>   - don't bother with O_NONBLOCK unless the size of the input is over N
>     bytes. The trouble there is that it's not clear what N should be.
>     It's fcntl(F_GETPIPE_SZ) on Linux, but that's not portable. We could
>     possibly come up with a conservative value if we had a ballpark for
>     pipe size on Windows. It feels a bit hacky, though.
>
>   - we could actually guess at a deadlock by putting a timeout on the
>     poll(). That would also catch hanging or slow filter processes. I
>     really hate putting clock-based limits on things, though, as it
>     means the tool behaves differently under load. And keep in mind this
>     is deep in the pipe_command() code. It happens to only trigger for
>     diff filters now, but it may be used in other spots (in fact it
>     already is, and it's only the size of current gpg payloads/responses
>     that means it doesn't happen to trigger).
>
> Stepping back, though, I think we should consider why we'd see an error
> here. I wouldn't expect it to ever fail on a system where O_NONBLOCK was
> supported. If we want to make it a silent noop on some platforms, then
> we can stick that into the enable_nonblock() function (which is what I
> did, but as René showed, that is probably not a good enough solution).
>
> -Peff
>