child_process: exec's promisify impl now includes stdout/err in error

This converts the initial implementation of a promised exec that used the customPromisifyArgs support in util.promisify with a custom implementation. This is because exec and execFile, when there is an error, still supply the stdout and stderr of the process, and yet the promisified version with customPromisifyArgs does not supply this ability. I created a custom implementation and attached it to exec and execFile using the util.promisify.custom key. Fixes: #13364
nodejs · giltayar · Jun 2, 2017 · Jun 2, 2017 · Jun 2, 2017 · Jun 3, 2017
commit e44a057febfd345467e7b9ec8f1b151132a237a3
diff --git a/doc/api/child_process.md b/doc/api/child_process.md
@@ -215,7 +215,9 @@ child runs longer than `timeout` milliseconds.
 replace the existing process and uses a shell to execute the command.
 
 If this method is invoked as its [`util.promisify()`][]ed version, it returns
-a Promise for an object with `stdout` and `stderr` properties.
+a Promise for an object with `stdout` and `stderr` properties. In case of an
+error, a rejected promise is returned, with the same `error` object given in the
+callback, but with an additional two properties `stdout` and `stderr`.
 
 For example:
 
@@ -281,7 +283,9 @@ stderr output. If `encoding` is `'buffer'`, or an unrecognized character
 encoding, `Buffer` objects will be passed to the callback instead.
 
 If this method is invoked as its [`util.promisify()`][]ed version, it returns
-a Promise for an object with `stdout` and `stderr` properties.
+a Promise for an object with `stdout` and `stderr` properties. In case of an
+error, a rejected promise is returned, with the same `error` object given in the
+callback, but with an additional two properties `stdout` and `stderr`.
 
 ```js
 const util = require('util');

diff --git a/lib/child_process.js b/lib/child_process.js
@@ -22,9 +22,9 @@
 'use strict';
 
 const util = require('util');
-const {
-  deprecate, convertToValidSignal, customPromisifyArgs
-} = require('internal/util');
+const { deprecate, convertToValidSignal } = require('internal/util');
+const { createPromise,
+  promiseResolve, promiseReject } = process.binding('util');
 const debug = util.debuglog('child_process');
 
 const uv = process.binding('uv');
@@ -140,9 +140,21 @@ exports.exec = function(command /*, options, callback*/) {
                           opts.callback);
 };
 
-Object.defineProperty(exports.exec, customPromisifyArgs,
-                      { value: ['stdout', 'stderr'], enumerable: false });
-
+Object.defineProperty(exports.exec, util.promisify.custom, {
+  enumerable: false, value: function(...args) {
+    const promise = createPromise();
+    exports.exec.call(this, ...args, (err, stdout, stderr) => {
+      if (err !== null) {
+        err.stdout = stdout;
+        err.stderr = stderr;
+        promiseReject(promise, err);
+      } else {
+        promiseResolve(promise, { stdout, stderr });
+      }
+    });
+    return promise;
+  }
+});
 
 exports.execFile = function(file /*, args, options, callback*/) {
   var args = [];
@@ -338,8 +350,21 @@ exports.execFile = function(file /*, args, options, callback*/) {
   return child;
 };
 
-Object.defineProperty(exports.execFile, customPromisifyArgs,
-                      { value: ['stdout', 'stderr'], enumerable: false });
+Object.defineProperty(exports.execFile, util.promisify.custom, {
+  enumerable: false, value: function(...args) {
+    const promise = createPromise();
+    exports.execFile.call(this, ...args, (err, stdout, stderr) => {
+      if (err !== null) {
+        err.stdout = stdout;
+        err.stderr = stderr;
+        promiseReject(promise, err);
+      } else {
+        promiseResolve(promise, { stdout, stderr });
+      }
+    });
+    return promise;
+  }
+});
 
 const _deprecatedCustomFds = deprecate(
   function deprecateCustomFds(options) {

diff --git a/test/parallel/test-child-process-promisified.js b/test/parallel/test-child-process-promisified.js
@@ -32,3 +32,22 @@ const execFile = promisify(child_process.execFile);
     assert(err.message.includes('doesntexist'));
   }));
 }
+const failingCodeWithStdoutErr =
+  'console.log("out");console.error("err");process.exit(1)';
+{
+  exec(`${process.execPath} -e '${failingCodeWithStdoutErr}'`)
+    .catch(common.mustCall((err) => {
+      assert.strictEqual(err.code, 1);
+      assert.strictEqual(err.stdout, 'out\n');
+      assert.strictEqual(err.stderr, 'err\n');
+    }));
+}
+
+{
+  execFile(process.execPath, ['-e', failingCodeWithStdoutErr])
+    .catch(common.mustCall((err) => {
+      assert.strictEqual(err.code, 1);
+      assert.strictEqual(err.stdout, 'out\n');
+      assert.strictEqual(err.stderr, 'err\n');
+    }));
+}