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.

This necessarily changes default behavior of `stdout` and `stderr`
callbacks such that they receive buffers rather than strings. The
alternative would be a performance hit on the defaults.

Refs: nodejs#6764
Refs: nodejs#1901

Author: Trott

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

@@ -1,7 +1,7 @@
 'use strict';
 const common = require('../common.js');
 
-var messagesLength = [64, 256, 1024, 4096];
+const messagesLength = [64, 256, 1024, 4096];
 // Windows does not support that long arguments
 if (process.platform !== 'win32')
   messagesLength.push(32768);

benchmark/child_process/child-process-read.js

@@ -2,7 +2,7 @@
 const common = require('../common.js');
 const os = require('os');
 
-var messagesLength = [64, 256, 1024, 4096];
+const messagesLength = [64, 256, 1024, 4096];
 // Windows does not support that long arguments
 if (os.platform() !== 'win32')
   messagesLength.push(32768);
@@ -20,7 +20,7 @@ function main(conf) {
   const len = +conf.len;
 
   const msg = '"' + Array(len).join('.') + '"';
-  const options = { 'stdio': ['ignore', 'pipe', 'ignore'] };
+  const options = {'stdio': ['ignore', 'ipc', 'ignore']};
   const child = spawn('yes', [msg], options);
 
   var bytes = 0;

doc/api/child_process.md

@@ -161,11 +161,11 @@ signal that terminated the process. Any exit code other than `0` is considered
 to be an error.
 
 The `stdout` and `stderr` arguments passed to the callback will contain the
-stdout and stderr output of the child process. By default, Node.js will decode
-the output as UTF-8 and pass strings to the callback. The `encoding` option
+stdout and stderr output of the child process. The `encoding` option
 can be used to specify the character encoding used to decode the stdout and
-stderr output. If `encoding` is `'buffer'`, `Buffer` objects will be passed to
-the callback instead.
+stderr output. If `encoding` is `'buffer'` (the default), `Buffer` objects will
+be passed to the callback. If a valid string encoding is specified instead,
+strings will be passed to the callback.
 
 The `options` argument may be passed as the second argument to customize how
 the process is spawned. The default options are:

lib/child_process.js

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

test/parallel/test-child-process-exec-data-buffer.js

@@ -0,0 +1,20 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const exec = require('child_process').exec;
+
+const keepAlive = setInterval(() => {}, 9999);
+var cbCalls = 0;
+const expectedCalls = 2;
+
+const cb = common.mustCall((data) => {
+  assert(data instanceof Buffer);
+  if (++cbCalls === expectedCalls) {
+    clearInterval(keepAlive);
+  }
+}, expectedCalls);
+
+const command = common.isWindows ? 'dir' : 'ls';
+exec(command).stdout.on('data', cb);
+
+exec('fhqwhgads').stderr.on('data', cb);

test/parallel/test-child-process-exec-stdout-data-string.js

@@ -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');
+  }));
+}

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

@@ -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');
@@ -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');
   }));
 }