git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
René Scharfe <l.s.r@web.de>
Date
Aug 10, 2022, 05:39 UTC
Message-ID
<0e1b8066-3f67-cec6-675a-05d2cf54c119@web.de>
In-Reply-To
<Yu/5LU+ZhbVRnSdM@coredump.intra.peff.net>
Am 07.08.22 um 19:41 schrieb Jeff King:
Show 42 quoted lines
> 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.

Show 5 quoted lines
> 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
Previous: Jeff KingNext: Jeff King
Message 36 of 44 in “pipe_command(): mark stdin descriptor as non-blocking”
  1. pipe_command(): mark stdin descriptor as non-blockingJeff King, Aug 2, 2022
  2. Junio C HamanoAug 2, 2022
  3. Jeff KingAug 2, 2022
  4. Junio C HamanoAug 2, 2022
  5. pipe_command(): mark stdin descriptor as non-blockingJeff King, Aug 3, 2022
  6. René ScharfeAug 3, 2022
  7. Jeff KingAug 3, 2022
  8. René ScharfeAug 3, 2022
  9. Jeff KingAug 5, 2022
  10. René ScharfeAug 5, 2022
  11. René ScharfeAug 7, 2022
  12. Jeff KingAug 7, 2022
  13. mingw: handle writes to non-blocking pipeRené Scharfe, Aug 10, 2022
  14. Johannes SchindelinAug 10, 2022
  15. Jeff KingAug 10, 2022
  16. René ScharfeAug 10, 2022
  17. Jeff KingAug 11, 2022
  18. René ScharfeAug 11, 2022
  19. Jeff KingAug 11, 2022
  20. René ScharfeAug 14, 2022
  21. Jeff KingAug 17, 2022
  22. 0/6 fix pipe_command() deadlockJeff King, Aug 17, 2022
  23. 1/6 compat: add function to enable nonblocking pipesJeff King, Aug 17, 2022
  24. Junio C HamanoAug 17, 2022
  25. Jeff KingAug 18, 2022
  26. 2/6 nonblock: support WindowsJeff King, Aug 17, 2022
  27. 3/6 git-compat-util: make MAX_IO_SIZE define globally availableJeff King, Aug 17, 2022
  28. 4/6 pipe_command(): avoid xwrite() for writing to pipeJeff King, Aug 17, 2022
  29. 5/6 pipe_command(): handle ENOSPC when writing to a pipeJeff King, Aug 17, 2022
  30. Junio C HamanoAug 17, 2022
  31. Jeff KingAug 18, 2022
  32. 6/6 pipe_command(): mark stdin descriptor as non-blockingJeff King, Aug 17, 2022
  33. Jeff KingAug 17, 2022
  34. René ScharfeAug 19, 2022
  35. Jeff KingAug 20, 2022
  36. René ScharfeAug 10, 2022
  37. Jeff KingAug 10, 2022
  38. René ScharfeAug 10, 2022
  39. Jeff KingAug 11, 2022
  40. René ScharfeAug 7, 2022
  41. Johannes SchindelinAug 8, 2022
  42. Johannes SchindelinAug 8, 2022
  43. Jeff KingAug 9, 2022
  44. Johannes SchindelinAug 9, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.