Skip to content

Commit

Permalink
child_process: measure buffer length in bytes
Browse files Browse the repository at this point in the history
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.

PR-URL: #6764
Fixes: #1901
Reviewed-By: Brian White <mscdex@mscdex.net>
  • Loading branch information
Trott authored and Myles Borins committed Jul 11, 2016
1 parent 5f679ab commit 625d56a
Show file tree
Hide file tree
Showing 5 changed files with 96 additions and 41 deletions.
30 changes: 30 additions & 0 deletions benchmark/child_process/child-process-exec-stdout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'use strict';
const common = require('../common.js');
const bench = common.createBenchmark(main, {
len: [64, 256, 1024, 4096, 32768],
dur: [5]
});

const exec = require('child_process').exec;
function main(conf) {
bench.start();

const dur = +conf.dur;
const len = +conf.len;

const msg = `"${'.'.repeat(len)}"`;
msg.match(/./);
const options = {'stdio': ['ignore', 'pipe', 'ignore']};
// NOTE: Command below assumes bash shell.
const child = exec(`while\n echo ${msg}\ndo :; done\n`, options);

var bytes = 0;
child.stdout.on('data', function(msg) {
bytes += msg.length;
});

setTimeout(function() {
child.kill();
bench.end(bytes);
}, dur * 1000);
}
18 changes: 9 additions & 9 deletions benchmark/child_process/child-process-read.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
'use strict';
var common = require('../common.js');
var bench = common.createBenchmark(main, {
const common = require('../common.js');
const bench = common.createBenchmark(main, {
len: [64, 256, 1024, 4096, 32768],
dur: [5]
});

var spawn = require('child_process').spawn;
const spawn = require('child_process').spawn;
function main(conf) {
bench.start();

var dur = +conf.dur;
var len = +conf.len;
const dur = +conf.dur;
const len = +conf.len;

var msg = '"' + Array(len).join('.') + '"';
var options = { 'stdio': ['ignore', 'ipc', 'ignore'] };
var child = spawn('yes', [msg], options);
const msg = '"' + Array(len).join('.') + '"';
const options = {'stdio': ['ignore', 'ipc', 'ignore']};
const child = spawn('yes', [msg], options);

var bytes = 0;
child.on('message', function(msg) {
Expand All @@ -23,7 +23,7 @@ function main(conf) {

setTimeout(function() {
child.kill();
var gbits = (bytes * 8) / (1024 * 1024 * 1024);
const gbits = (bytes * 8) / (1024 * 1024 * 1024);
bench.end(gbits);
}, dur * 1000);
}
71 changes: 41 additions & 30 deletions lib/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,15 +157,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;
Expand All @@ -187,16 +185,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
Expand Down Expand Up @@ -256,39 +261,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);
}
});
}
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-child-process-maxBuffer-stderr.js
Original file line number Diff line number Diff line change
@@ -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');
}));
}
Original file line number Diff line number Diff line change
@@ -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');
Expand All @@ -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');
}));
}

0 comments on commit 625d56a

Please sign in to comment.