Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child_process: measure buffer length in bytes #6764

Closed
wants to merge 12 commits into from
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 @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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('utf8')` happened in userland, convert
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean any setEncoding() call, or specifically w/ 'utf8' passed?

Copy link
Member Author

@Trott Trott May 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean any setEncoding(). Will change the comment...

// 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;
Copy link
Contributor

@mscdex mscdex May 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed by the way?

Copy link
Member Author

@Trott Trott May 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mscdex It's needed so that stdoutLen has the correct value when passed to Buffer.concat() later on. The alternative is to let the buffer length exceed options.maxBuffer. However, that does not match the behavior of the current implementation whereas this does.

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('utf8')` 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');
}));
}