From f2c8cb5a7fb321c3d814ddeefe6e10517360f73c Mon Sep 17 00:00:00 2001 From: Dave Date: Mon, 30 Nov 2015 02:41:51 -0800 Subject: [PATCH 1/4] child_process.flushStdio: resume _consuming streams When a client calls read() with a nonzero argument on a Socket, that Socket sets this._consuming to true. It never sets this._consuming back to false. child_process.flushStdio currently doesn't flush any streams where _consuming is truthy. But that means that it never flushes any stream that has ever been read from. This prevents a child process from ever closing if one of its streams has been read from, causing issue #4049. child_process.flushStdio should flush streams even if their _consuming is set to true. Then it will close even after a read. --- lib/internal/child_process.js | 2 +- test/sequential/test-child-process-flush-stdio.js | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 test/sequential/test-child-process-flush-stdio.js diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 6f396fac6c95e6..cd5313d65ce457 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -217,7 +217,7 @@ util.inherits(ChildProcess, EventEmitter); function flushStdio(subprocess) { if (subprocess.stdio == null) return; subprocess.stdio.forEach(function(stream, fd, stdio) { - if (!stream || !stream.readable || stream._consuming) + if (!stream || !stream.readable) return; stream.resume(); }); diff --git a/test/sequential/test-child-process-flush-stdio.js b/test/sequential/test-child-process-flush-stdio.js new file mode 100644 index 00000000000000..743fc2c465355f --- /dev/null +++ b/test/sequential/test-child-process-flush-stdio.js @@ -0,0 +1,13 @@ +'use strict'; +const cp = require('child_process'); +const common = require('../common'); + +var p = cp.spawn('yes'); + +p.on('close', common.mustCall(function() {})); + +p.stdout.read(); + +setTimeout(function() { + p.kill(); +}, 100); From d13f74f5adeb7904b6d66f9efc45b1b7eda83b47 Mon Sep 17 00:00:00 2001 From: Dave Date: Mon, 30 Nov 2015 10:27:57 -0800 Subject: [PATCH 2/4] child-process-flush-stdio-test: rename and use echo --- test/{sequential => parallel}/test-child-process-flush-stdio.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename test/{sequential => parallel}/test-child-process-flush-stdio.js (88%) diff --git a/test/sequential/test-child-process-flush-stdio.js b/test/parallel/test-child-process-flush-stdio.js similarity index 88% rename from test/sequential/test-child-process-flush-stdio.js rename to test/parallel/test-child-process-flush-stdio.js index 743fc2c465355f..11d726a92ad30d 100644 --- a/test/sequential/test-child-process-flush-stdio.js +++ b/test/parallel/test-child-process-flush-stdio.js @@ -2,7 +2,7 @@ const cp = require('child_process'); const common = require('../common'); -var p = cp.spawn('yes'); +var p = cp.spawn('echo'); p.on('close', common.mustCall(function() {})); From 4becc69ed9bbe99a55d7e120d04c58b0db7a8a55 Mon Sep 17 00:00:00 2001 From: Dave Date: Mon, 30 Nov 2015 11:56:40 -0800 Subject: [PATCH 3/4] test-child-process-flush-stdio: review changes Make the spawned process object a const, and add assertions to the event handler that the arguments are as expected --- test/parallel/test-child-process-flush-stdio.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-child-process-flush-stdio.js b/test/parallel/test-child-process-flush-stdio.js index 11d726a92ad30d..b1e18c5cdd6577 100644 --- a/test/parallel/test-child-process-flush-stdio.js +++ b/test/parallel/test-child-process-flush-stdio.js @@ -1,10 +1,14 @@ 'use strict'; const cp = require('child_process'); const common = require('../common'); +const assert = require('assert'); -var p = cp.spawn('echo'); +const p = cp.spawn('echo'); -p.on('close', common.mustCall(function() {})); +p.on('close', common.mustCall(function(code, signal) { + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); +})); p.stdout.read(); From 127a82ecc950ac73199ded537db5b1161b022855 Mon Sep 17 00:00:00 2001 From: Dave Date: Mon, 30 Nov 2015 13:12:28 -0800 Subject: [PATCH 4/4] test-child-process-flush-stdio: indent 2 spaces --- test/parallel/test-child-process-flush-stdio.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-child-process-flush-stdio.js b/test/parallel/test-child-process-flush-stdio.js index b1e18c5cdd6577..5fd7eb3bc99922 100644 --- a/test/parallel/test-child-process-flush-stdio.js +++ b/test/parallel/test-child-process-flush-stdio.js @@ -6,8 +6,8 @@ const assert = require('assert'); const p = cp.spawn('echo'); p.on('close', common.mustCall(function(code, signal) { - assert.strictEqual(code, 0); - assert.strictEqual(signal, null); + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); })); p.stdout.read();