Skip to content

Commit 0503681

Browse files
Trottrvagg
Trott
authored and
rvagg
committed
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. PR-URL: #6764 Fixes: #1901 Reviewed-By: Brian White <[email protected]>
1 parent 93e150f commit 0503681

5 files changed

Lines changed: 96 additions & 41 deletions

File tree

‎benchmark/child_process/child-process-exec-stdout.js‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
'use strict';
2+
const common = require('../common.js');
3+
const bench = common.createBenchmark(main, {
4+
len: [64, 256, 1024, 4096, 32768],
5+
dur: [5]
6+
});
7+
8+
const exec = require('child_process').exec;
9+
function main(conf) {
10+
bench.start();
11+
12+
const dur = +conf.dur;
13+
const len = +conf.len;
14+
15+
const msg = `"${'.'.repeat(len)}"`;
16+
msg.match(/./);
17+
const options = {'stdio': ['ignore', 'pipe', 'ignore']};
18+
// NOTE: Command below assumes bash shell.
19+
const child = exec(`while\n echo ${msg}\ndo :; done\n`, options);
20+
21+
var bytes = 0;
22+
child.stdout.on('data', function(msg) {
23+
bytes += msg.length;
24+
});
25+
26+
setTimeout(function() {
27+
child.kill();
28+
bench.end(bytes);
29+
}, dur * 1000);
30+
}

‎benchmark/child_process/child-process-read.js‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
11
'use strict';
2-
var common = require('../common.js');
3-
var bench = common.createBenchmark(main, {
2+
const common = require('../common.js');
3+
const bench = common.createBenchmark(main, {
44
len: [64, 256, 1024, 4096, 32768],
55
dur: [5]
66
});
77

8-
var spawn = require('child_process').spawn;
8+
const spawn = require('child_process').spawn;
99
function main(conf) {
1010
bench.start();
1111

12-
var dur = +conf.dur;
13-
var len = +conf.len;
12+
const dur = +conf.dur;
13+
const len = +conf.len;
1414

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

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

2424
setTimeout(function() {
2525
child.kill();
26-
var gbits = (bytes * 8) / (1024 * 1024 * 1024);
26+
const gbits = (bytes * 8) / (1024 * 1024 * 1024);
2727
bench.end(gbits);
2828
}, dur * 1000);
2929
}

‎lib/child_process.js‎

Lines changed: 41 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -146,15 +146,13 @@ exports.execFile = function(file /*, args, options, callback*/) {
146146
});
147147

148148
var encoding;
149-
var _stdout;
150-
var _stderr;
149+
var stdoutState;
150+
var stderrState;
151+
var _stdout = [];
152+
var _stderr = [];
151153
if (options.encoding !== 'buffer' && Buffer.isEncoding(options.encoding)) {
152154
encoding = options.encoding;
153-
_stdout = '';
154-
_stderr = '';
155155
} else {
156-
_stdout = [];
157-
_stderr = [];
158156
encoding = null;
159157
}
160158
var stdoutLen = 0;
@@ -176,16 +174,23 @@ exports.execFile = function(file /*, args, options, callback*/) {
176174

177175
if (!callback) return;
178176

179-
// merge chunks
180-
var stdout;
181-
var stderr;
182-
if (!encoding) {
183-
stdout = Buffer.concat(_stdout);
184-
stderr = Buffer.concat(_stderr);
185-
} else {
186-
stdout = _stdout;
187-
stderr = _stderr;
188-
}
177+
var stdout = Buffer.concat(_stdout, stdoutLen);
178+
var stderr = Buffer.concat(_stderr, stderrLen);
179+
180+
var stdoutEncoding = encoding;
181+
var stderrEncoding = encoding;
182+
183+
if (stdoutState && stdoutState.decoder)
184+
stdoutEncoding = stdoutState.decoder.encoding;
185+
186+
if (stderrState && stderrState.decoder)
187+
stderrEncoding = stderrState.decoder.encoding;
188+
189+
if (stdoutEncoding)
190+
stdout = stdout.toString(stdoutEncoding);
191+
192+
if (stderrEncoding)
193+
stderr = stderr.toString(stderrEncoding);
189194

190195
if (ex) {
191196
// Will be handled later
@@ -245,39 +250,45 @@ exports.execFile = function(file /*, args, options, callback*/) {
245250
}
246251

247252
if (child.stdout) {
248-
if (encoding)
249-
child.stdout.setEncoding(encoding);
253+
stdoutState = child.stdout._readableState;
250254

251255
child.stdout.addListener('data', function(chunk) {
252-
stdoutLen += chunk.length;
256+
// If `child.stdout.setEncoding()` happened in userland, convert string to
257+
// Buffer.
258+
if (stdoutState.decoder) {
259+
chunk = Buffer.from(chunk, stdoutState.decoder.encoding);
260+
}
261+
262+
stdoutLen += chunk.byteLength;
253263

254264
if (stdoutLen > options.maxBuffer) {
255265
ex = new Error('stdout maxBuffer exceeded');
266+
stdoutLen -= chunk.byteLength;
256267
kill();
257268
} else {
258-
if (!encoding)
259-
_stdout.push(chunk);
260-
else
261-
_stdout += chunk;
269+
_stdout.push(chunk);
262270
}
263271
});
264272
}
265273

266274
if (child.stderr) {
267-
if (encoding)
268-
child.stderr.setEncoding(encoding);
275+
stderrState = child.stderr._readableState;
269276

270277
child.stderr.addListener('data', function(chunk) {
271-
stderrLen += chunk.length;
278+
// If `child.stderr.setEncoding()` happened in userland, convert string to
279+
// Buffer.
280+
if (stderrState.decoder) {
281+
chunk = Buffer.from(chunk, stderrState.decoder.encoding);
282+
}
283+
284+
stderrLen += chunk.byteLength;
272285

273286
if (stderrLen > options.maxBuffer) {
274287
ex = new Error('stderr maxBuffer exceeded');
288+
stderrLen -= chunk.byteLength;
275289
kill();
276290
} else {
277-
if (!encoding)
278-
_stderr.push(chunk);
279-
else
280-
_stderr += chunk;
291+
_stderr.push(chunk);
281292
}
282293
});
283294
}

‎test/parallel/test-child-process-maxBuffer-stderr.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const cp = require('child_process');
5+
const unicode = '中文测试'; // Length = 4, Byte length = 13
6+
7+
if (process.argv[2] === 'child') {
8+
console.error(unicode);
9+
} else {
10+
const cmd = `${process.execPath} ${__filename} child`;
11+
12+
cp.exec(cmd, {maxBuffer: 10}, common.mustCall((err, stdout, stderr) => {
13+
assert.strictEqual(err.message, 'stderr maxBuffer exceeded');
14+
}));
15+
}

test/known_issues/test-child-process-max-buffer.js renamed to test/parallel/test-child-process-maxBuffer-stdout.js

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
'use strict';
2-
// Refs: https://github.com/nodejs/node/issues/1901
32
const common = require('../common');
43
const assert = require('assert');
54
const cp = require('child_process');
@@ -10,7 +9,7 @@ if (process.argv[2] === 'child') {
109
} else {
1110
const cmd = `${process.execPath} ${__filename} child`;
1211

13-
cp.exec(cmd, { maxBuffer: 10 }, common.mustCall((err, stdout, stderr) => {
12+
cp.exec(cmd, {maxBuffer: 10}, common.mustCall((err, stdout, stderr) => {
1413
assert.strictEqual(err.message, 'stdout maxBuffer exceeded');
1514
}));
1615
}

0 commit comments

Comments
 (0)