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: #6764Fixes: #1901 Reviewed-By: Brian White <[email protected]>

Author: Trott

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

@@ -0,0 +1,30 @@
+'use strict';
+constcommon=require('../common.js');
+constbench=common.createBenchmark(main,{
+len: [64,256,1024,4096,32768],
+dur: [5]
+});
+
+constexec=require('child_process').exec;
+functionmain(conf){
+bench.start();
+
+constdur=+conf.dur;
+constlen=+conf.len;
+
+constmsg=`"${'.'.repeat(len)}"`;
+msg.match(/./);
+constoptions={'stdio': ['ignore','pipe','ignore']};
+// NOTE: Command below assumes bash shell.
+constchild=exec(`while\n echo ${msg}\ndo :; done\n`,options);
+
+varbytes=0;
+child.stdout.on('data',function(msg){
+bytes+=msg.length;
+});
+
+setTimeout(function(){
+child.kill();
+bench.end(bytes);
+},dur*1000);
+}

benchmark/child_process/child-process-read.js

@@ -1,20 +1,20 @@
 'use strict';
-varcommon=require('../common.js');
-varbench=common.createBenchmark(main,{
+constcommon=require('../common.js');
+constbench=common.createBenchmark(main,{
 len: [64,256,1024,4096,32768],
 dur: [5]
 });
 
-varspawn=require('child_process').spawn;
+constspawn=require('child_process').spawn;
 functionmain(conf){
 bench.start();
 
-vardur=+conf.dur;
-varlen=+conf.len;
+constdur=+conf.dur;
+constlen=+conf.len;
 
-varmsg='"'+Array(len).join('.')+'"';
-varoptions={'stdio': ['ignore','ipc','ignore']};
-varchild=spawn('yes',[msg],options);
+constmsg='"'+Array(len).join('.')+'"';
+constoptions={'stdio': ['ignore','ipc','ignore']};
+constchild=spawn('yes',[msg],options);
 
 varbytes=0;
 child.on('message',function(msg){
@@ -23,7 +23,7 @@ function main(conf){
 
 setTimeout(function(){
 child.kill();
-vargbits=(bytes*8)/(1024*1024*1024);
+constgbits=(bytes*8)/(1024*1024*1024);
 bench.end(gbits);
 },dur*1000);
 }

lib/child_process.js

@@ -157,15 +157,13 @@ exports.execFile = function(file /*, args, options, callback*/){
 });
 
 varencoding;
-var_stdout;
-var_stderr;
+varstdoutState;
+varstderrState;
+var_stdout=[];
+var_stderr=[];
 if(options.encoding!=='buffer'&&Buffer.isEncoding(options.encoding)){
 encoding=options.encoding;
-_stdout='';
-_stderr='';
 }else{
-_stdout=[];
-_stderr=[];
 encoding=null;
 }
 varstdoutLen=0;
@@ -187,16 +185,23 @@ exports.execFile = function(file /*, args, options, callback*/){
 
 if(!callback)return;
 
-// merge chunks
-varstdout;
-varstderr;
-if(!encoding){
-stdout=Buffer.concat(_stdout);
-stderr=Buffer.concat(_stderr);
-}else{
-stdout=_stdout;
-stderr=_stderr;
-}
+varstdout=Buffer.concat(_stdout,stdoutLen);
+varstderr=Buffer.concat(_stderr,stderrLen);
+
+varstdoutEncoding=encoding;
+varstderrEncoding=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
@@ -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=newError('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=newError('stderr maxBuffer exceeded');
+stderrLen-=chunk.byteLength;
 kill();
 }else{
-if(!encoding)
-_stderr.push(chunk);
-else
-_stderr+=chunk;
+_stderr.push(chunk);
 }
 });
 }

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

@@ -0,0 +1,15 @@
+'use strict';
+constcommon=require('../common');
+constassert=require('assert');
+constcp=require('child_process');
+constunicode='中文测试';// Length = 4, Byte length = 13
+
+if(process.argv[2]==='child'){
+console.error(unicode);
+}else{
+constcmd=`${process.execPath}${__filename} child`;
+
+cp.exec(cmd,{maxBuffer: 10},common.mustCall((err,stdout,stderr)=>{
+assert.strictEqual(err.message,'stderr maxBuffer exceeded');
+}));
+}

…_issues/test-child-process-max-buffer.js …l/test-child-process-maxBuffer-stdout.jstest/known_issues/test-child-process-max-buffer.js renamed to test/parallel/test-child-process-maxBuffer-stdout.js test/known_issues/test-child-process-max-buffer.js renamed to test/parallel/test-child-process-maxBuffer-stdout.js

@@ -1,5 +1,4 @@
 'use strict';
-// Refs: https://github.com/nodejs/node/issues/1901
 constcommon=require('../common');
 constassert=require('assert');
 constcp=require('child_process');
@@ -10,7 +9,7 @@ if (process.argv[2] === 'child'){
 }else{
 constcmd=`${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');
 }));
 }