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

["René Scharfe" <l.s.r@web.de>]

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