Skip to content

Commit

Permalink
child_process: only stop readable side of stream passed to proc
Browse files Browse the repository at this point in the history
Closing the underlying resource completely has the unwanted side effect
that the stream can no longer be used at all, including passing it
to other child processes.

What we want to avoid is accidentally reading from the stream;
accordingly, it should be sufficient to stop its readable side
manually, and otherwise leave the underlying resource intact.

Fixes: nodejs#27097
Refs: nodejs#21209
  • Loading branch information
addaleax committed Apr 23, 2019
1 parent b773f77 commit 9cd75c7
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 8 deletions.
10 changes: 5 additions & 5 deletions lib/internal/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -384,12 +384,12 @@ ChildProcess.prototype.spawn = function(options) {
continue;
}

// The stream is already cloned and piped, thus close it.
// The stream is already cloned and piped, thus stop its readable side,
// otherwise we might attempt to read from the stream when at the same time
// the child process does.
if (stream.type === 'wrap') {
stream.handle.close();
if (stream._stdio && stream._stdio instanceof EventEmitter) {
stream._stdio.emit('close');
}
stream.handle.readStop();
stream._stdio.pause();
continue;
}

Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-child-process-server-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

const server = net.createServer((conn) => {
conn.on('close', common.mustCall());

spawn(process.execPath, ['-v'], {
stdio: ['ignore', conn, 'ignore']
}).on('close', common.mustCall());
}).on('close', common.mustCall(() => {
conn.end();
}));
}).listen(common.PIPE, () => {
const client = net.connect(common.PIPE, common.mustCall());
client.on('data', () => {
Expand Down
30 changes: 30 additions & 0 deletions test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'use strict';
const common = require('../common');
const assert = require('assert');

// Regression test for https://github.com/nodejs/node/issues/27097.
// Check that (cat [p1] ; cat [p2]) | cat [p3] works.

const { spawn } = require('child_process');
const p3 = spawn('cat', { stdio: ['pipe', 'pipe', process.stderr] });
const p1 = spawn('cat', { stdio: ['pipe', p3.stdin, process.stderr] });
const p2 = spawn('cat', { stdio: ['pipe', p3.stdin, process.stderr] });
p3.stdout.setEncoding('utf8');

// Write three different chunks:
// - 'hello' from this process to p1 to p3 back to us
// - 'world' from this process to p2 to p3 back to us
// - 'foobar' from this process to p3 back to us
// Do so sequentially in order to avoid race conditions.
p1.stdin.end('hello\n');
p3.stdout.once('data', common.mustCall((chunk) => {
assert.strictEqual(chunk, 'hello\n');
p2.stdin.end('world\n');
p3.stdout.once('data', common.mustCall((chunk) => {
assert.strictEqual(chunk, 'world\n');
p3.stdin.end('foobar\n');
p3.stdout.once('data', common.mustCall((chunk) => {
assert.strictEqual(chunk, 'foobar\n');
}));
}));
}));

0 comments on commit 9cd75c7

Please sign in to comment.