From c60742e26e2a28d18404920f33d43f5bc55cf9ae Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 22 Jun 2016 16:21:21 -0700 Subject: [PATCH 1/8] test: add test for exec() known issue Refs: https://github.com/nodejs/node/issues/7342 --- .../test-child-process-exec-stdout-data-string.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 test/known_issues/test-child-process-exec-stdout-data-string.js diff --git a/test/known_issues/test-child-process-exec-stdout-data-string.js b/test/known_issues/test-child-process-exec-stdout-data-string.js new file mode 100644 index 00000000000000..443613450699cc --- /dev/null +++ b/test/known_issues/test-child-process-exec-stdout-data-string.js @@ -0,0 +1,15 @@ +'use strict'; +// Refs: https://github.com/nodejs/node/issues/7342 +const common = require('../common'); +const assert = require('assert'); +const exec = require('child_process').exec; + +const keepAlive = setInterval(() => {}, 9999); + +const command = common.isWindows ? 'dir' : 'ls'; +const e = exec(command); + +e.stdout.on('data', function(data) { + assert.strictEqual(typeof data, 'string'); + clearInterval(keepAlive); +}); From 0bff9b3829a4684f5fe4595c71e296b07a658362 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 22 Jun 2016 16:38:58 -0700 Subject: [PATCH 2/8] Revert "child_process: measure buffer length in bytes" This reverts commit c9a5990a76ccb15872234948e23bdc12691c2e70. --- lib/child_process.js | 71 ++++++++----------- .../test-child-process-max-buffer.js} | 3 +- ...t-child-process-exec-stdout-data-string.js | 0 .../test-child-process-maxBuffer-stderr.js | 15 ---- 4 files changed, 32 insertions(+), 57 deletions(-) rename test/{parallel/test-child-process-maxBuffer-stdout.js => known_issues/test-child-process-max-buffer.js} (75%) rename test/{known_issues => parallel}/test-child-process-exec-stdout-data-string.js (100%) delete mode 100644 test/parallel/test-child-process-maxBuffer-stderr.js diff --git a/lib/child_process.js b/lib/child_process.js index 5eee5228f040d3..0c6af12bc8e81b 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -146,13 +146,15 @@ exports.execFile = function(file /*, args, options, callback*/) { }); var encoding; - var stdoutState; - var stderrState; - var _stdout = []; - var _stderr = []; + var _stdout; + var _stderr; if (options.encoding !== 'buffer' && Buffer.isEncoding(options.encoding)) { encoding = options.encoding; + _stdout = ''; + _stderr = ''; } else { + _stdout = []; + _stderr = []; encoding = null; } var stdoutLen = 0; @@ -174,23 +176,16 @@ exports.execFile = function(file /*, args, options, callback*/) { if (!callback) return; - var stdout = Buffer.concat(_stdout, stdoutLen); - var stderr = Buffer.concat(_stderr, stderrLen); - - var stdoutEncoding = encoding; - var stderrEncoding = encoding; - - if (stdoutState && stdoutState.decoder) - stdoutEncoding = stdoutState.decoder.encoding; - - if (stderrState && stderrState.decoder) - stderrEncoding = stderrState.decoder.encoding; - - if (stdoutEncoding) - stdout = stdout.toString(stdoutEncoding); - - if (stderrEncoding) - stderr = stderr.toString(stderrEncoding); + // merge chunks + var stdout; + var stderr; + if (!encoding) { + stdout = Buffer.concat(_stdout); + stderr = Buffer.concat(_stderr); + } else { + stdout = _stdout; + stderr = _stderr; + } if (ex) { // Will be handled later @@ -250,45 +245,39 @@ exports.execFile = function(file /*, args, options, callback*/) { } if (child.stdout) { - stdoutState = child.stdout._readableState; + if (encoding) + child.stdout.setEncoding(encoding); child.stdout.addListener('data', function(chunk) { - // If `child.stdout.setEncoding()` happened in userland, convert string to - // Buffer. - if (stdoutState.decoder) { - chunk = Buffer.from(chunk, stdoutState.decoder.encoding); - } - - stdoutLen += chunk.byteLength; + stdoutLen += chunk.length; if (stdoutLen > options.maxBuffer) { ex = new Error('stdout maxBuffer exceeded'); - stdoutLen -= chunk.byteLength; kill(); } else { - _stdout.push(chunk); + if (!encoding) + _stdout.push(chunk); + else + _stdout += chunk; } }); } if (child.stderr) { - stderrState = child.stderr._readableState; + if (encoding) + child.stderr.setEncoding(encoding); child.stderr.addListener('data', function(chunk) { - // If `child.stderr.setEncoding()` happened in userland, convert string to - // Buffer. - if (stderrState.decoder) { - chunk = Buffer.from(chunk, stderrState.decoder.encoding); - } - - stderrLen += chunk.byteLength; + stderrLen += chunk.length; if (stderrLen > options.maxBuffer) { ex = new Error('stderr maxBuffer exceeded'); - stderrLen -= chunk.byteLength; kill(); } else { - _stderr.push(chunk); + if (!encoding) + _stderr.push(chunk); + else + _stderr += chunk; } }); } diff --git a/test/parallel/test-child-process-maxBuffer-stdout.js b/test/known_issues/test-child-process-max-buffer.js similarity index 75% rename from test/parallel/test-child-process-maxBuffer-stdout.js rename to test/known_issues/test-child-process-max-buffer.js index 122dc499f462bf..14a344c7062a5a 100644 --- a/test/parallel/test-child-process-maxBuffer-stdout.js +++ b/test/known_issues/test-child-process-max-buffer.js @@ -1,4 +1,5 @@ 'use strict'; +// Refs: https://github.com/nodejs/node/issues/1901 const common = require('../common'); const assert = require('assert'); const cp = require('child_process'); @@ -9,7 +10,7 @@ if (process.argv[2] === 'child') { } else { const cmd = `${process.execPath} ${__filename} child`; - cp.exec(cmd, {maxBuffer: 10}, common.mustCall((err, stdout, stderr) => { + cp.exec(cmd, { maxBuffer: 10 }, common.mustCall((err, stdout, stderr) => { assert.strictEqual(err.message, 'stdout maxBuffer exceeded'); })); } diff --git a/test/known_issues/test-child-process-exec-stdout-data-string.js b/test/parallel/test-child-process-exec-stdout-data-string.js similarity index 100% rename from test/known_issues/test-child-process-exec-stdout-data-string.js rename to test/parallel/test-child-process-exec-stdout-data-string.js diff --git a/test/parallel/test-child-process-maxBuffer-stderr.js b/test/parallel/test-child-process-maxBuffer-stderr.js deleted file mode 100644 index ecaea8b152a0ca..00000000000000 --- a/test/parallel/test-child-process-maxBuffer-stderr.js +++ /dev/null @@ -1,15 +0,0 @@ -'use strict'; -const common = require('../common'); -const assert = require('assert'); -const cp = require('child_process'); -const unicode = '中文测试'; // Length = 4, Byte length = 13 - -if (process.argv[2] === 'child') { - console.error(unicode); -} else { - const cmd = `${process.execPath} ${__filename} child`; - - cp.exec(cmd, {maxBuffer: 10}, common.mustCall((err, stdout, stderr) => { - assert.strictEqual(err.message, 'stderr maxBuffer exceeded'); - })); -} From 0e3c527b9ac4835a8826ee774c787a06e1bbce60 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 14 May 2016 15:24:34 -0700 Subject: [PATCH 3/8] child_process: measure buffer length in bytes This change fixes a known issue where `maxBuffer` limits by characters rather than bytes. Benchmark added to confirm no performance regression occurs with this change. This necessarily changes default behavior of `stdout` and `stderr` callbacks such that they receive buffers rather than strings. The alternative would be a performance hit on the defaults. Refs: https://github.com/nodejs/node/pull/6764 Refs: https://github.com/nodejs/node/issues/1901 --- .../child-process-exec-stdout.js | 2 +- benchmark/child_process/child-process-read.js | 4 +- doc/api/child_process.md | 8 +-- lib/child_process.js | 71 +++++++++++-------- .../test-child-process-exec-data-buffer.js | 20 ++++++ ...t-child-process-exec-stdout-data-string.js | 15 ---- .../test-child-process-maxBuffer-stderr.js | 15 ++++ .../test-child-process-maxBuffer-stdout.js} | 3 +- 8 files changed, 84 insertions(+), 54 deletions(-) create mode 100644 test/parallel/test-child-process-exec-data-buffer.js delete mode 100644 test/parallel/test-child-process-exec-stdout-data-string.js create mode 100644 test/parallel/test-child-process-maxBuffer-stderr.js rename test/{known_issues/test-child-process-max-buffer.js => parallel/test-child-process-maxBuffer-stdout.js} (75%) diff --git a/benchmark/child_process/child-process-exec-stdout.js b/benchmark/child_process/child-process-exec-stdout.js index 6194f66b55d987..f48e8257e6c421 100644 --- a/benchmark/child_process/child-process-exec-stdout.js +++ b/benchmark/child_process/child-process-exec-stdout.js @@ -1,7 +1,7 @@ 'use strict'; const common = require('../common.js'); -var messagesLength = [64, 256, 1024, 4096]; +const messagesLength = [64, 256, 1024, 4096]; // Windows does not support that long arguments if (process.platform !== 'win32') messagesLength.push(32768); diff --git a/benchmark/child_process/child-process-read.js b/benchmark/child_process/child-process-read.js index b0128eb7969056..6f7ddf0bf59a1c 100644 --- a/benchmark/child_process/child-process-read.js +++ b/benchmark/child_process/child-process-read.js @@ -2,7 +2,7 @@ const common = require('../common.js'); const os = require('os'); -var messagesLength = [64, 256, 1024, 4096]; +const messagesLength = [64, 256, 1024, 4096]; // Windows does not support that long arguments if (os.platform() !== 'win32') messagesLength.push(32768); @@ -20,7 +20,7 @@ function main(conf) { const len = +conf.len; const msg = '"' + Array(len).join('.') + '"'; - const options = { 'stdio': ['ignore', 'pipe', 'ignore'] }; + const options = {'stdio': ['ignore', 'ipc', 'ignore']}; const child = spawn('yes', [msg], options); var bytes = 0; diff --git a/doc/api/child_process.md b/doc/api/child_process.md index a196f99b282b8d..10e52463852b37 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -161,11 +161,11 @@ signal that terminated the process. Any exit code other than `0` is considered to be an error. The `stdout` and `stderr` arguments passed to the callback will contain the -stdout and stderr output of the child process. By default, Node.js will decode -the output as UTF-8 and pass strings to the callback. The `encoding` option +stdout and stderr output of the child process. The `encoding` option can be used to specify the character encoding used to decode the stdout and -stderr output. If `encoding` is `'buffer'`, `Buffer` objects will be passed to -the callback instead. +stderr output. If `encoding` is `'buffer'` (the default), `Buffer` objects will +be passed to the callback. If a valid string encoding is specified instead, +strings will be passed to the callback. The `options` argument may be passed as the second argument to customize how the process is spawned. The default options are: diff --git a/lib/child_process.js b/lib/child_process.js index 0c6af12bc8e81b..5eee5228f040d3 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -146,15 +146,13 @@ exports.execFile = function(file /*, args, options, callback*/) { }); var encoding; - var _stdout; - var _stderr; + var stdoutState; + var stderrState; + var _stdout = []; + var _stderr = []; if (options.encoding !== 'buffer' && Buffer.isEncoding(options.encoding)) { encoding = options.encoding; - _stdout = ''; - _stderr = ''; } else { - _stdout = []; - _stderr = []; encoding = null; } var stdoutLen = 0; @@ -176,16 +174,23 @@ exports.execFile = function(file /*, args, options, callback*/) { if (!callback) return; - // merge chunks - var stdout; - var stderr; - if (!encoding) { - stdout = Buffer.concat(_stdout); - stderr = Buffer.concat(_stderr); - } else { - stdout = _stdout; - stderr = _stderr; - } + var stdout = Buffer.concat(_stdout, stdoutLen); + var stderr = Buffer.concat(_stderr, stderrLen); + + var stdoutEncoding = encoding; + var stderrEncoding = encoding; + + if (stdoutState && stdoutState.decoder) + stdoutEncoding = stdoutState.decoder.encoding; + + if (stderrState && stderrState.decoder) + stderrEncoding = stderrState.decoder.encoding; + + if (stdoutEncoding) + stdout = stdout.toString(stdoutEncoding); + + if (stderrEncoding) + stderr = stderr.toString(stderrEncoding); if (ex) { // Will be handled later @@ -245,39 +250,45 @@ exports.execFile = function(file /*, args, options, callback*/) { } if (child.stdout) { - if (encoding) - child.stdout.setEncoding(encoding); + stdoutState = child.stdout._readableState; child.stdout.addListener('data', function(chunk) { - stdoutLen += chunk.length; + // If `child.stdout.setEncoding()` happened in userland, convert string to + // Buffer. + if (stdoutState.decoder) { + chunk = Buffer.from(chunk, stdoutState.decoder.encoding); + } + + stdoutLen += chunk.byteLength; if (stdoutLen > options.maxBuffer) { ex = new Error('stdout maxBuffer exceeded'); + stdoutLen -= chunk.byteLength; kill(); } else { - if (!encoding) - _stdout.push(chunk); - else - _stdout += chunk; + _stdout.push(chunk); } }); } if (child.stderr) { - if (encoding) - child.stderr.setEncoding(encoding); + stderrState = child.stderr._readableState; child.stderr.addListener('data', function(chunk) { - stderrLen += chunk.length; + // If `child.stderr.setEncoding()` happened in userland, convert string to + // Buffer. + if (stderrState.decoder) { + chunk = Buffer.from(chunk, stderrState.decoder.encoding); + } + + stderrLen += chunk.byteLength; if (stderrLen > options.maxBuffer) { ex = new Error('stderr maxBuffer exceeded'); + stderrLen -= chunk.byteLength; kill(); } else { - if (!encoding) - _stderr.push(chunk); - else - _stderr += chunk; + _stderr.push(chunk); } }); } diff --git a/test/parallel/test-child-process-exec-data-buffer.js b/test/parallel/test-child-process-exec-data-buffer.js new file mode 100644 index 00000000000000..447d195aafe34c --- /dev/null +++ b/test/parallel/test-child-process-exec-data-buffer.js @@ -0,0 +1,20 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const exec = require('child_process').exec; + +const keepAlive = setInterval(() => {}, 9999); +var cbCalls = 0; +const expectedCalls = 2; + +const cb = common.mustCall((data) => { + assert(data instanceof Buffer); + if (++cbCalls === expectedCalls) { + clearInterval(keepAlive); + } +}, expectedCalls); + +const command = common.isWindows ? 'dir' : 'ls'; +exec(command).stdout.on('data', cb); + +exec('fhqwhgads').stderr.on('data', cb); diff --git a/test/parallel/test-child-process-exec-stdout-data-string.js b/test/parallel/test-child-process-exec-stdout-data-string.js deleted file mode 100644 index 443613450699cc..00000000000000 --- a/test/parallel/test-child-process-exec-stdout-data-string.js +++ /dev/null @@ -1,15 +0,0 @@ -'use strict'; -// Refs: https://github.com/nodejs/node/issues/7342 -const common = require('../common'); -const assert = require('assert'); -const exec = require('child_process').exec; - -const keepAlive = setInterval(() => {}, 9999); - -const command = common.isWindows ? 'dir' : 'ls'; -const e = exec(command); - -e.stdout.on('data', function(data) { - assert.strictEqual(typeof data, 'string'); - clearInterval(keepAlive); -}); diff --git a/test/parallel/test-child-process-maxBuffer-stderr.js b/test/parallel/test-child-process-maxBuffer-stderr.js new file mode 100644 index 00000000000000..ecaea8b152a0ca --- /dev/null +++ b/test/parallel/test-child-process-maxBuffer-stderr.js @@ -0,0 +1,15 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const cp = require('child_process'); +const unicode = '中文测试'; // Length = 4, Byte length = 13 + +if (process.argv[2] === 'child') { + console.error(unicode); +} else { + const cmd = `${process.execPath} ${__filename} child`; + + cp.exec(cmd, {maxBuffer: 10}, common.mustCall((err, stdout, stderr) => { + assert.strictEqual(err.message, 'stderr maxBuffer exceeded'); + })); +} diff --git a/test/known_issues/test-child-process-max-buffer.js b/test/parallel/test-child-process-maxBuffer-stdout.js similarity index 75% rename from test/known_issues/test-child-process-max-buffer.js rename to test/parallel/test-child-process-maxBuffer-stdout.js index 14a344c7062a5a..122dc499f462bf 100644 --- a/test/known_issues/test-child-process-max-buffer.js +++ b/test/parallel/test-child-process-maxBuffer-stdout.js @@ -1,5 +1,4 @@ 'use strict'; -// Refs: https://github.com/nodejs/node/issues/1901 const common = require('../common'); const assert = require('assert'); const cp = require('child_process'); @@ -10,7 +9,7 @@ if (process.argv[2] === 'child') { } else { const cmd = `${process.execPath} ${__filename} child`; - cp.exec(cmd, { maxBuffer: 10 }, common.mustCall((err, stdout, stderr) => { + cp.exec(cmd, {maxBuffer: 10}, common.mustCall((err, stdout, stderr) => { assert.strictEqual(err.message, 'stdout maxBuffer exceeded'); })); } From 3c23f12cc6262d6ba40ca3eb972fb7d4daec9e31 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 22 Jun 2016 22:18:51 -0700 Subject: [PATCH 4/8] squash: test update --- test/parallel/test-child-process-exec-data-buffer.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/parallel/test-child-process-exec-data-buffer.js b/test/parallel/test-child-process-exec-data-buffer.js index 447d195aafe34c..4636c6dc6abb0c 100644 --- a/test/parallel/test-child-process-exec-data-buffer.js +++ b/test/parallel/test-child-process-exec-data-buffer.js @@ -3,18 +3,14 @@ const common = require('../common'); const assert = require('assert'); const exec = require('child_process').exec; -const keepAlive = setInterval(() => {}, 9999); var cbCalls = 0; const expectedCalls = 2; const cb = common.mustCall((data) => { assert(data instanceof Buffer); - if (++cbCalls === expectedCalls) { - clearInterval(keepAlive); - } }, expectedCalls); const command = common.isWindows ? 'dir' : 'ls'; exec(command).stdout.on('data', cb); -exec('fhqwhgads').stderr.on('data', cb); +exec('fhqwhgads').stderr.on('data', cb); \ No newline at end of file From 2fe26f0c6f329ee8390a218ad44d0d5186d5ae3d Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 22 Jun 2016 22:24:23 -0700 Subject: [PATCH 5/8] squash: fix lint --- test/parallel/test-child-process-exec-data-buffer.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/parallel/test-child-process-exec-data-buffer.js b/test/parallel/test-child-process-exec-data-buffer.js index 4636c6dc6abb0c..c33a800fd4c0aa 100644 --- a/test/parallel/test-child-process-exec-data-buffer.js +++ b/test/parallel/test-child-process-exec-data-buffer.js @@ -3,7 +3,6 @@ const common = require('../common'); const assert = require('assert'); const exec = require('child_process').exec; -var cbCalls = 0; const expectedCalls = 2; const cb = common.mustCall((data) => { From 36942a1a7f0395791912e6387b17fd6bc1579ac2 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 22 Jun 2016 22:39:18 -0700 Subject: [PATCH 6/8] squash: undo merge mistake --- benchmark/child_process/child-process-read.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmark/child_process/child-process-read.js b/benchmark/child_process/child-process-read.js index 6f7ddf0bf59a1c..58f016c764ae0b 100644 --- a/benchmark/child_process/child-process-read.js +++ b/benchmark/child_process/child-process-read.js @@ -20,7 +20,7 @@ function main(conf) { const len = +conf.len; const msg = '"' + Array(len).join('.') + '"'; - const options = {'stdio': ['ignore', 'ipc', 'ignore']}; + const options = { 'stdio': ['ignore', 'pipe', 'ignore'] }; const child = spawn('yes', [msg], options); var bytes = 0; From 1e429a828fbfb30c8afb8af8ba3c64ed844ce1ef Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 22 Jun 2016 22:42:01 -0700 Subject: [PATCH 7/8] squash: remove incorrect doc change --- doc/api/child_process.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index 10e52463852b37..a196f99b282b8d 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -161,11 +161,11 @@ signal that terminated the process. Any exit code other than `0` is considered to be an error. The `stdout` and `stderr` arguments passed to the callback will contain the -stdout and stderr output of the child process. The `encoding` option +stdout and stderr output of the child process. By default, Node.js will decode +the output as UTF-8 and pass strings to the callback. The `encoding` option can be used to specify the character encoding used to decode the stdout and -stderr output. If `encoding` is `'buffer'` (the default), `Buffer` objects will -be passed to the callback. If a valid string encoding is specified instead, -strings will be passed to the callback. +stderr output. If `encoding` is `'buffer'`, `Buffer` objects will be passed to +the callback instead. The `options` argument may be passed as the second argument to customize how the process is spawned. The default options are: From b20cb2526d6f56ce6066c08ed23ddac3026cd8ac Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 22 Jun 2016 22:44:14 -0700 Subject: [PATCH 8/8] squash: lint --- test/parallel/test-child-process-exec-data-buffer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-child-process-exec-data-buffer.js b/test/parallel/test-child-process-exec-data-buffer.js index c33a800fd4c0aa..0854203d0ef335 100644 --- a/test/parallel/test-child-process-exec-data-buffer.js +++ b/test/parallel/test-child-process-exec-data-buffer.js @@ -12,4 +12,4 @@ const cb = common.mustCall((data) => { const command = common.isWindows ? 'dir' : 'ls'; exec(command).stdout.on('data', cb); -exec('fhqwhgads').stderr.on('data', cb); \ No newline at end of file +exec('fhqwhgads').stderr.on('data', cb);