diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 9eac06d1fdf145..d71ecfa21a1de0 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -71,6 +71,7 @@ const { UV_ENFILE, UV_ENOENT, UV_ENOSYS, + UV_EOF, UV_ESRCH, } = internalBinding('uv'); @@ -337,6 +338,47 @@ function createSocket(pipe, readable) { } +// `child.stdin` is constructed with `readable: false`, which prevents +// `net.Socket` from starting the libuv read loop on the underlying pipe. +// Without it the parent never observes when the child closes its end of +// the stdin pipe, so `child.stdin` only emits 'close' once the child +// process exits (and sometimes not at all, depending on teardown order). +// This function restores the pre-#18701 behavior by attaching a minimal +// EOF watcher to the pipe handle: when the kernel signals that the peer +// (the child's fd 0) has closed, we destroy the writable side so users +// observe a 'close' event on `child.stdin`. Refs: #25131. +function watchStdinForPeerClose(socket, pipe) { + if (pipe === null || socket === null) return; + pipe.onread = function() { + const nread = streamBaseState[kReadBytesOrError]; + if (nread === UV_EOF) { + // The child closed its end of the stdin pipe. End the writable + // side gracefully (draining any pending writes) and then destroy, + // so net.Socket fires 'close' on `child.stdin`. + if (!socket.destroyed) { + socket.destroySoon(); + } + } else if (nread > 0) { + // The child is unexpectedly writing to its fd 0. Stop reading so + // we don't keep allocating buffers we never consume; teardown + // will fall back to the `onexit` handler. + pipe.reading = false; + pipe.readStop(); + } + // Other read errors (negative nread other than UV_EOF) are + // intentionally ignored; net.Socket's normal teardown remains in + // charge of the rest of the lifecycle. + }; + const err = pipe.readStart(); + if (err) { + // If readStart fails, fall back to the previous behavior. The + // `onexit` handler will still destroy the socket once the child + // process exits. + pipe.onread = FunctionPrototype; + } +} + + function getHandleWrapType(stream) { if (stream instanceof Pipe) return 'pipe'; if (stream instanceof TTY) return 'tty'; @@ -479,6 +521,10 @@ ChildProcess.prototype.spawn = function spawn(options) { stream.socket.on('close', () => { maybeClose(this); }); + } else if (i === 0 && this.pid !== 0) { + // Restore detection of the child closing its end of the stdin + // pipe so `child.stdin` emits 'close' eagerly. Refs: #25131. + watchStdinForPeerClose(stream.socket, stream.handle); } } } diff --git a/test/parallel/test-child-process-stdin-close-on-exit.js b/test/parallel/test-child-process-stdin-close-on-exit.js new file mode 100644 index 00000000000000..b3e1c990f55cf3 --- /dev/null +++ b/test/parallel/test-child-process-stdin-close-on-exit.js @@ -0,0 +1,73 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +'use strict'; + +// Regression test for https://github.com/nodejs/node/issues/25131. +// +// Since #18701 (v8.12) the parent's `child.stdin` socket no longer emitted a +// 'close' event when the child closed its end of the stdin pipe. The +// 'close' event should be observable so consumers can detect that the +// writable side of the pipe is gone without having to attempt a write and +// wait for EPIPE. This test exercises both: +// 1. The well-behaved case where the child simply exits without anyone +// touching stdin — `child.stdin` must still emit 'close'. +// 2. The exact scenario from #25131 where a long-running child closes +// its own fd 0 — the parent should observe 'close' eagerly, before +// the child process exits. + +const common = require('../common'); +const { spawn } = require('child_process'); + +// Case 1: child exits without anyone touching stdin. `child.stdin` must +// emit 'close'. +{ + const child = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 50)']); + child.stdin.on('close', common.mustCall()); + child.on('exit', common.mustCall()); +} + +// Case 2: child explicitly closes its own stdin file descriptor while +// still running. The parent should observe 'close' on `child.stdin` +// eagerly, without waiting for the child process to exit. Skipped on +// Windows because the named-pipe semantics there do not propagate the +// peer close until the child process exits. +if (!common.isWindows) { + const child = spawn( + process.execPath, + ['-e', 'require("fs").closeSync(0); setTimeout(() => {}, 2000)'], + { stdio: ['pipe', 'inherit', 'inherit'] }, + ); + // The 'close' must fire well before the child's 2s timer expires. + // common.platformTimeout keeps this comfortable on slow CI machines + // while still catching the regression (which would only fire 'close' + // after the child finally exits at ~2000ms). + const eagerWindow = setTimeout(() => { + throw new Error('child.stdin close did not fire eagerly on peer-close'); + }, common.platformTimeout(1500)); + child.stdin.on('close', common.mustCall(() => { + clearTimeout(eagerWindow); + // Kill the still-running child so the test does not have to wait + // for its 2s timer. + child.kill(); + })); + child.on('exit', common.mustCall()); +}