From 8d7f07f3799d5b4ec541eb606e785d7ce17e82d0 Mon Sep 17 00:00:00 2001
From: Gil Tayar <gil@tayar.org>
Date: Fri, 2 Jun 2017 11:46:46 +0300
Subject: [PATCH] child_process: promisify includes stdio 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: https://github.com/nodejs/node/issues/13364
PR-URL: https://github.com/nodejs/node/pull/13388
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 doc/api/child_process.md                      |  8 +++--
 lib/child_process.js                          | 34 +++++++++++++++----
 .../test-child-process-promisified.js         | 19 +++++++++++
 3 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/doc/api/child_process.md b/doc/api/child_process.md
index 2120e36f4be..17d54e7f7e8 100644
--- 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
index 95b643c85d1..4d1f1ab16e9 100644
--- 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,27 @@ exports.exec = function(command /*, options, callback*/) {
                           opts.callback);
 };
 
-Object.defineProperty(exports.exec, customPromisifyArgs,
-                      { value: ['stdout', 'stderr'], enumerable: false });
+const customPromiseExecFunction = (orig) => {
+  return (...args) => {
+    const promise = createPromise();
 
+    orig(...args, (err, stdout, stderr) => {
+      if (err !== null) {
+        err.stdout = stdout;
+        err.stderr = stderr;
+        promiseReject(promise, err);
+      } else {
+        promiseResolve(promise, { stdout, stderr });
+      }
+    });
+    return promise;
+  };
+};
+
+Object.defineProperty(exports.exec, util.promisify.custom, {
+  enumerable: false,
+  value: customPromiseExecFunction(exports.exec)
+});
 
 exports.execFile = function(file /*, args, options, callback*/) {
   var args = [];
@@ -338,8 +356,10 @@ 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: customPromiseExecFunction(exports.execFile)
+});
 
 const _deprecatedCustomFds = deprecate(
   function deprecateCustomFds(options) {
diff --git a/test/parallel/test-child-process-promisified.js b/test/parallel/test-child-process-promisified.js
index 322cb110eb6..0fa9c68a92d 100644
--- 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(42);console.error(43);process.exit(1)';
+{
+  exec(`${process.execPath} -e "${failingCodeWithStdoutErr}"`)
+    .catch(common.mustCall((err) => {
+      assert.strictEqual(err.code, 1);
+      assert.strictEqual(err.stdout, '42\n');
+      assert.strictEqual(err.stderr, '43\n');
+    }));
+}
+
+{
+  execFile(process.execPath, ['-e', failingCodeWithStdoutErr])
+    .catch(common.mustCall((err) => {
+      assert.strictEqual(err.code, 1);
+      assert.strictEqual(err.stdout, '42\n');
+      assert.strictEqual(err.stderr, '43\n');
+    }));
+}