From e44a057febfd345467e7b9ec8f1b151132a237a3 Mon Sep 17 00:00:00 2001 From: Gil Tayar Date: Fri, 2 Jun 2017 11:46:46 +0300 Subject: [PATCH 1/4] 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: https://github.com/nodejs/node/issues/13364 --- doc/api/child_process.md | 8 +++- lib/child_process.js | 41 +++++++++++++++---- .../test-child-process-promisified.js | 19 +++++++++ 3 files changed, 58 insertions(+), 10 deletions(-) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index 4800d5bc9302a9..cbfa1a551f0449 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 95b643c85d131e..974548b34c9914 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,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 index 322cb110eb619a..9b2f8f0186d54a 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("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'); + })); +} From e2182e5688a6a660e15e4d9698b801f358c6712d Mon Sep 17 00:00:00 2001 From: Gil Tayar Date: Fri, 2 Jun 2017 21:21:09 +0300 Subject: [PATCH 2/4] child_process: refactorings after review comments --- lib/child_process.js | 29 ++++++++----------- .../test-child-process-promisified.js | 10 +++---- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 974548b34c9914..4d1f1ab16e9622 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -140,10 +140,11 @@ exports.exec = function(command /*, options, callback*/) { opts.callback); }; -Object.defineProperty(exports.exec, util.promisify.custom, { - enumerable: false, value: function(...args) { +const customPromiseExecFunction = (orig) => { + return (...args) => { const promise = createPromise(); - exports.exec.call(this, ...args, (err, stdout, stderr) => { + + orig(...args, (err, stdout, stderr) => { if (err !== null) { err.stdout = stdout; err.stderr = stderr; @@ -153,7 +154,12 @@ Object.defineProperty(exports.exec, util.promisify.custom, { } }); return promise; - } + }; +}; + +Object.defineProperty(exports.exec, util.promisify.custom, { + enumerable: false, + value: customPromiseExecFunction(exports.exec) }); exports.execFile = function(file /*, args, options, callback*/) { @@ -351,19 +357,8 @@ exports.execFile = function(file /*, args, options, callback*/) { }; 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; - } + enumerable: false, + value: customPromiseExecFunction(exports.execFile) }); const _deprecatedCustomFds = deprecate( diff --git a/test/parallel/test-child-process-promisified.js b/test/parallel/test-child-process-promisified.js index 9b2f8f0186d54a..28ed448dcaa8d6 100644 --- a/test/parallel/test-child-process-promisified.js +++ b/test/parallel/test-child-process-promisified.js @@ -33,13 +33,13 @@ const execFile = promisify(child_process.execFile); })); } const failingCodeWithStdoutErr = - 'console.log("out");console.error("err");process.exit(1)'; + 'console.log(42);console.error(42);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'); + assert.strictEqual(err.stdout, '42\n'); + assert.strictEqual(err.stderr, '43\n'); })); } @@ -47,7 +47,7 @@ const failingCodeWithStdoutErr = 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'); + assert.strictEqual(err.stdout, '42\n'); + assert.strictEqual(err.stderr, '43\n'); })); } From 6adbd98634e60f153d3392e2485aa0c194251190 Mon Sep 17 00:00:00 2001 From: Gil Tayar Date: Fri, 2 Jun 2017 21:48:43 +0300 Subject: [PATCH 3/4] child_process: fixed bug in promisified exec test --- test/parallel/test-child-process-promisified.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-child-process-promisified.js b/test/parallel/test-child-process-promisified.js index 28ed448dcaa8d6..6bb0c82662b0c2 100644 --- a/test/parallel/test-child-process-promisified.js +++ b/test/parallel/test-child-process-promisified.js @@ -33,7 +33,7 @@ const execFile = promisify(child_process.execFile); })); } const failingCodeWithStdoutErr = - 'console.log(42);console.error(42);process.exit(1)'; + 'console.log(42);console.error(43);process.exit(1)'; { exec(`${process.execPath} -e '${failingCodeWithStdoutErr}'`) .catch(common.mustCall((err) => { From e37cb7e6eaad8b168cb3d85a4ab5941dfee49a52 Mon Sep 17 00:00:00 2001 From: Gil Tayar Date: Sat, 3 Jun 2017 11:01:30 +0300 Subject: [PATCH 4/4] child_process: fixed win compat bug in test --- test/parallel/test-child-process-promisified.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-child-process-promisified.js b/test/parallel/test-child-process-promisified.js index 6bb0c82662b0c2..0fa9c68a92d884 100644 --- a/test/parallel/test-child-process-promisified.js +++ b/test/parallel/test-child-process-promisified.js @@ -35,7 +35,7 @@ const execFile = promisify(child_process.execFile); const failingCodeWithStdoutErr = 'console.log(42);console.error(43);process.exit(1)'; { - exec(`${process.execPath} -e '${failingCodeWithStdoutErr}'`) + exec(`${process.execPath} -e "${failingCodeWithStdoutErr}"`) .catch(common.mustCall((err) => { assert.strictEqual(err.code, 1); assert.strictEqual(err.stdout, '42\n');