Downstream breakage due to #4694

[santigimeno]

#4694 assumed that the default size of the buffers from the socketpair was 64k on linux, which at least, in my box is much larger: 212992, whereas after the change they're set to 131072 which is 128k (2x64k) as man (7) socket explains:

Sets or gets the maximum socket receive buffer in bytes. The kernel doubles this value (to allow space for bookkeeping overhead) when it is set using setsockopt(2), and this doubled value is returned by getsockopt(2).

This is causing failures in the node.js CI: https://ci.nodejs.org/job/node-test-commit-linux-containered/50277/nodes=ubuntu2204_sharedlibs_withoutssl_x64/testReport/junit/(root)/parallel/test_child_process_stdout_flush_exit/.

Should we revert this or just apply the changes to macOS? Whatever we decide we'll need to make a patch release afterwards.

[vtjnash]

If that test is failing, it is a nodejs bug, and not something libuv is responsible for. Though I agree that forcing a smaller buffer size on the user might not be good.

[santigimeno]

Agreed it's a bug on the assumptions made in that test. I'm worried we might be breaking people which are already making similar assumptions in their code. That's why I was proposing limiting the change to macOS only. Another option would be checking the buffer before resizing to avoid reducing it, though that implementation might differ among platforms.

[vtjnash]

I don't think the test is wrong. It appears to be specifically a test to make sure nodejs is implemented correctly independently of this particular configuration parameter.

[santigimeno]

I think the test is wrong because it assumes all the writes to stdout would complete synchronously, but it doesn't have to. I'm seeing EAGAIN when the issue happens. It's also what's stated in the node.js documentation: https://nodejs.org/api/process.html#processexitcode

[...] The reason this is problematic is because writes to process.stdout in Node.js are sometimes asynchronous and may occur over multiple ticks of the Node.js event loop. Calling process.exit(), however, forces the process to exit before those additional writes to stdout can be performed.

[vtjnash]

Ah, okay. I assumed the existence of such a test implied that nodejs had been intending this API to function differently. It seems the documented fix is to set process.exitCode instead of calling exit directly?

[santigimeno]

Yes, or just letting it end gracefully with this patch, but I'm not sure whether the test original purpose was to test that stdout was completely flushed after calling process.exit(0) and maybe it's best to remove it as it contradicts what's documented.

diff --git a/test/parallel/test-child-process-stdout-flush-exit.js b/test/parallel/test-child-process-stdout-flush-exit.js
index 3c5f00d9bb2..39e4afc2a0c 100644
--- a/test/parallel/test-child-process-stdout-flush-exit.js
+++ b/test/parallel/test-child-process-stdout-flush-exit.js
@@ -31,7 +31,6 @@ if (process.argv[2] === 'child') {
     console.log('filler');
   }
   console.log('goodbye');
-  process.exit(0);
 } else {
   // parent process
   const spawn = require('child_process').spawn;

[juanarbol]

maybe it's best to remove it as it contradicts what's documented

I agree.