regression: child_process stdin pipe close event not emitted

[jbunton-atlassian]

Summary

• Version: v8.12.0 and newer, all v10.x and all v11.x
• Platform: Linux and macOS amd64
• Subsystem: child_process

Since #18701 Node.js doesn't emit a close event when a child process closes its stdin pipe. The close is only detected if the parent tries to write to the child's stdin and receives an EPIPE.

The original behaviour is preferable because it allows immediate detection of the close event, rather than waiting for a failed write.

Reproducer

This works as expected in v8.11.4 and fails in all newer versions of v8.x, as well as all v10.x and all v11.x.

const {spawn} = require('child_process');

const cp = spawn(
    'node', [
        '-e',
        'fs.closeSync(0); setTimeout(() => {}, 2000)'
    ],
    {stdio: ['pipe', 'inherit', 'inherit']}
)

setTimeout(() => {
    console.log('BUGGY! stdin close event was not emitted')
    process.exit(1);
}, 1000);

cp.stdin.on('close', () => {
    console.log('Ok!')
    process.exit(0);
});

Problem detail

As far as I can tell the only way the close event can be emitted is if the Socket constructor calls this.read(0). This triggers onStreamRead() to be called, which does stream.push(null) and eventually results in the close event. This is a little weird because stdin isn't a readable stream :)

In Node 8.11.4 this worked because child_process.js:createSocket() called the Socket constructor with options.readable === undefined. So the Socket constructor sees that options.readable !== false and runs this.read(0)

In PR #18701 createSocket() was changed to call the Socket constructor with options.readable === true. This stops this.read(0) from being called and the close event is not emitted.

Hacky solution

--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -286,3 +286,3 @@ function flushStdio(subprocess) {
 function createSocket(pipe, readable) {
-  return net.Socket({ handle: pipe, readable, writable: !readable });
+  return net.Socket({ handle: pipe, readable, writable: !readable, childProcess: true });
 }
--- a/lib/net.js
+++ b/lib/net.js
@@ -315,3 +315,5 @@ function Socket(options) {
   // buffer.  if not, then this will happen when we connect
-  if (this._handle && options.readable !== false) {
+  if (options.childProcess) {
+    this.read(0);
+  } else if (this._handle && options.readable !== false) {
     if (options.pauseOnCreate) {

This passes the full test suite with a minor change to test-pipewrap.js. I haven't raised a PR because I'm sure somebody could think of a better solution. If someone can point me in the right direction I'm happy to try and implement it.

Thanks :)

[Fishrock123]

The provided test case also emits the following uv error for me, but only after the script is ended, so I presume it comes from the child.

Assertion failed: (fd > STDERR_FILENO), function uv__close, file ../deps/uv/src/unix/core.c, line 539.

[jbunton-atlassian]

hmm, I don't see that. Maybe I didn't compile with assertions enabled.

I agree that it should be coming from the child process. You could test by using a Python child instead :)

const cp = spawn(
    'python', [
        '-c',
        'import os, time; os.close(0); time.sleep(2)'
    ],
    {stdio: ['pipe', 'inherit', 'inherit']}
);

[adarshsaraogi]

shall I work on this issue.

[oyyd]

Reverting #18701 on master doesn't resolve this issue for me and I guess that's because of 67e2a15.

Maybe we could call stream.socket.read(0)(or socket.resume(), IDK) around here:

node/lib/internal/child_process.js

Line 390 in 80873ec

stream.socket = createSocket(this.pid !== 0 ?

rather than passing childProcess to net module If this is a specific issue of child_process.

[ronag]

Is this still a problem on e.g. v14?

[jbunton-atlassian]

@ronag, I just ran the test script with v14.12.0 and it is still broken.

[jasnell]

There's been no further discussion or activity on this in quite some time. Is there still stuff to do here?

[Tseian]

There's been no further discussion or activity on this in quite some time. Is there still stuff to do here?

@jasnell I reproduced it in the version 25. I am going to find an approach to fix it.