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

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

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