Skip to content

Commit b8f1a55

Browse files
TrottMyles Borins
Trott
authored and
Myles Borins
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 <mscdex@mscdex.net>
1 parent 8dd7eec commit b8f1a55

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
@@ -157,15 +157,13 @@ exports.execFile = function(file /*, args, options, callback*/) {
157157
});
158158

159159
var encoding;
160-
var _stdout;
161-
var _stderr;
160+
var stdoutState;
161+
var stderrState;
162+
var _stdout = [];
163+
var _stderr = [];
162164
if (options.encoding !== 'buffer' && Buffer.isEncoding(options.encoding)) {
163165
encoding = options.encoding;
164-
_stdout = '';
165-
_stderr = '';
166166
} else {
167-
_stdout = [];
168-
_stderr = [];
169167
encoding = null;
170168
}
171169
var stdoutLen = 0;
@@ -187,16 +185,23 @@ exports.execFile = function(file /*, args, options, callback*/) {
187185

188186
if (!callback) return;
189187

190-
// merge chunks
191-
var stdout;
192-
var stderr;
193-
if (!encoding) {
194-
stdout = Buffer.concat(_stdout);
195-
stderr = Buffer.concat(_stderr);
196-
} else {
197-
stdout = _stdout;
198-
stderr = _stderr;
199-
}
188+
var stdout = Buffer.concat(_stdout, stdoutLen);
189+
var stderr = Buffer.concat(_stderr, stderrLen);
190+
191+
var stdoutEncoding = encoding;
192+
var stderrEncoding = encoding;
193+
194+
if (stdoutState && stdoutState.decoder)
195+
stdoutEncoding = stdoutState.decoder.encoding;
196+
197+
if (stderrState && stderrState.decoder)
198+
stderrEncoding = stderrState.decoder.encoding;
199+
200+
if (stdoutEncoding)
201+
stdout = stdout.toString(stdoutEncoding);
202+
203+
if (stderrEncoding)
204+
stderr = stderr.toString(stderrEncoding);
200205

201206
if (ex) {
202207
// Will be handled later
@@ -256,39 +261,45 @@ exports.execFile = function(file /*, args, options, callback*/) {
256261
}
257262

258263
if (child.stdout) {
259-
if (encoding)
260-
child.stdout.setEncoding(encoding);
264+
stdoutState = child.stdout._readableState;
261265

262266
child.stdout.addListener('data', function(chunk) {
263-
stdoutLen += chunk.length;
267+
// If `child.stdout.setEncoding()` happened in userland, convert string to
268+
// Buffer.
269+
if (stdoutState.decoder) {
270+
chunk = Buffer.from(chunk, stdoutState.decoder.encoding);
271+
}
272+
273+
stdoutLen += chunk.byteLength;
264274

265275
if (stdoutLen > options.maxBuffer) {
266276
ex = new Error('stdout maxBuffer exceeded');
277+
stdoutLen -= chunk.byteLength;
267278
kill();
268279
} else {
269-
if (!encoding)
270-
_stdout.push(chunk);
271-
else
272-
_stdout += chunk;
280+
_stdout.push(chunk);
273281
}
274282
});
275283
}
276284

277285
if (child.stderr) {
278-
if (encoding)
279-
child.stderr.setEncoding(encoding);
286+
stderrState = child.stderr._readableState;
280287

281288
child.stderr.addListener('data', function(chunk) {
282-
stderrLen += chunk.length;
289+
// If `child.stderr.setEncoding()` happened in userland, convert string to
290+
// Buffer.
291+
if (stderrState.decoder) {
292+
chunk = Buffer.from(chunk, stderrState.decoder.encoding);
293+
}
294+
295+
stderrLen += chunk.byteLength;
283296

284297
if (stderrLen > options.maxBuffer) {
285298
ex = new Error('stderr maxBuffer exceeded');
299+
stderrLen -= chunk.byteLength;
286300
kill();
287301
} else {
288-
if (!encoding)
289-
_stderr.push(chunk);
290-
else
291-
_stderr += chunk;
302+
_stderr.push(chunk);
292303
}
293304
});
294305
}

‎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)