From e24dcf54cde9d88d263db09e54f60ddd9897384e Mon Sep 17 00:00:00 2001 From: ehmicky Date: Mon, 10 Jun 2019 10:00:00 +0200 Subject: [PATCH 1/6] Fix async early exceptions --- index.js | 6 +++++- lib/merge.js | 24 ++++++++++++++++++++++++ test.js | 9 +++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 lib/merge.js diff --git a/index.js b/index.js index 4482095cc4..70adbe0429 100644 --- a/index.js +++ b/index.js @@ -12,6 +12,7 @@ const mergeStream = require('merge-stream'); const pFinally = require('p-finally'); const onExit = require('signal-exit'); const stdio = require('./lib/stdio'); +const mergePrototypes = require('./lib/merge'); const TEN_MEGABYTES = 1000 * 1000 * 10; const DEFAULT_FORCE_KILL_TIMEOUT = 1000 * 5; @@ -275,13 +276,16 @@ const execa = (file, args, options) => { try { spawned = childProcess.spawn(parsed.file, parsed.args, parsed.options); } catch (error) { - return Promise.reject(makeError({error, stdout: '', stderr: '', all: ''}, { + const promise = Promise.reject(makeError({error, stdout: '', stderr: '', all: ''}, { command, parsed, timedOut: false, isCanceled: false, killed: false })); + // We make sure `child_process` properties are present even though no child process was created. + // This is to ensure the return value always has the same shape. + return mergePrototypes(promise, new childProcess.ChildProcess()); } const kill = spawned.kill.bind(spawned); diff --git a/lib/merge.js b/lib/merge.js new file mode 100644 index 0000000000..132d12500c --- /dev/null +++ b/lib/merge.js @@ -0,0 +1,24 @@ +'use strict'; + +// Merge two objects, including their prototypes +function mergePrototypes(to, from) { + const prototypes = [...getPrototypes(to), ...getPrototypes(from)]; + const newPrototype = prototypes.reduce(reducePrototype, {}); + return Object.assign(Object.setPrototypeOf(to, newPrototype), to, from); +} + +function getPrototypes(object, prototypes = []) { + const prototype = Object.getPrototypeOf(object); + if (prototype !== null) { + return getPrototypes(prototype, [...prototypes, prototype]); + } + + return prototypes; +} + +function reducePrototype(prototype, constructor) { + return Object.defineProperties(prototype, Object.getOwnPropertyDescriptors(constructor)); +} + +module.exports = mergePrototypes; + diff --git a/test.js b/test.js index 94a095b62e..58da5af9f3 100644 --- a/test.js +++ b/test.js @@ -301,6 +301,15 @@ test('execa() returns a promise with kill() and pid', t => { t.is(typeof pid, 'number'); }); +test('child_process.spawn() propagated errors have correct shape', t => { + const cp = execa('noop', {uid: -1}); + t.notThrows(() => { + cp.catch(() => {}); + cp.unref(); + cp.on('error', () => {}); + }); +}); + test('child_process.spawn() errors are propagated', async t => { const {failed} = await t.throwsAsync(execa('noop', {uid: -1})); t.true(failed); From bb9bf216db9bdfa3c5d0b1c3f937cbf76d56e611 Mon Sep 17 00:00:00 2001 From: ehmicky Date: Mon, 10 Jun 2019 10:00:00 +0200 Subject: [PATCH 2/6] Rename variables --- lib/merge.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/merge.js b/lib/merge.js index 132d12500c..0913323747 100644 --- a/lib/merge.js +++ b/lib/merge.js @@ -3,8 +3,8 @@ // Merge two objects, including their prototypes function mergePrototypes(to, from) { const prototypes = [...getPrototypes(to), ...getPrototypes(from)]; - const newPrototype = prototypes.reduce(reducePrototype, {}); - return Object.assign(Object.setPrototypeOf(to, newPrototype), to, from); + const prototype = prototypes.reduce(shallowMerge, {}); + return Object.assign(Object.setPrototypeOf(to, prototype), from); } function getPrototypes(object, prototypes = []) { @@ -16,8 +16,8 @@ function getPrototypes(object, prototypes = []) { return prototypes; } -function reducePrototype(prototype, constructor) { - return Object.defineProperties(prototype, Object.getOwnPropertyDescriptors(constructor)); +function shallowMerge(toObject, fromObject) { + return Object.defineProperties(toObject, Object.getOwnPropertyDescriptors(fromObject)); } module.exports = mergePrototypes; From 63a6f4aaf7736603ddc0ff3fbaa86ee6726c7c12 Mon Sep 17 00:00:00 2001 From: ehmicky Date: Fri, 14 Jun 2019 10:00:00 +0200 Subject: [PATCH 3/6] Add `mergePromise()` utility --- index.js | 44 ++++++++++++++++++++++++-------------------- lib/merge.js | 24 ------------------------ 2 files changed, 24 insertions(+), 44 deletions(-) delete mode 100644 lib/merge.js diff --git a/index.js b/index.js index 70adbe0429..cb18bf280f 100644 --- a/index.js +++ b/index.js @@ -12,7 +12,6 @@ const mergeStream = require('merge-stream'); const pFinally = require('p-finally'); const onExit = require('signal-exit'); const stdio = require('./lib/stdio'); -const mergePrototypes = require('./lib/merge'); const TEN_MEGABYTES = 1000 * 1000 * 10; const DEFAULT_FORCE_KILL_TIMEOUT = 1000 * 5; @@ -229,6 +228,20 @@ function joinCommand(file, args = []) { return [file, ...args].join(' '); } +// The return value is a mixin of `childProcess` and `Promise` +function mergePromise(spawned, getPromise) { + // eslint-disable-next-line promise/prefer-await-to-then + spawned.then = (onFulfilled, onRejected) => getPromise().then(onFulfilled, onRejected); + spawned.catch = onRejected => getPromise().catch(onRejected); + + // TODO: Remove the `if`-guard when targeting Node.js 10 + if (Promise.prototype.finally) { + spawned.finally = onFinally => getPromise().finally(onFinally); + } + + return spawned; +} + function spawnedKill(kill, signal = 'SIGTERM', options = {}) { const killResult = kill(signal); setKillTimeout(kill, signal, options, killResult); @@ -276,16 +289,15 @@ const execa = (file, args, options) => { try { spawned = childProcess.spawn(parsed.file, parsed.args, parsed.options); } catch (error) { - const promise = Promise.reject(makeError({error, stdout: '', stderr: '', all: ''}, { - command, - parsed, - timedOut: false, - isCanceled: false, - killed: false - })); - // We make sure `child_process` properties are present even though no child process was created. - // This is to ensure the return value always has the same shape. - return mergePrototypes(promise, new childProcess.ChildProcess()); + return mergePromise(new childProcess.ChildProcess(), () => + Promise.reject(makeError({error, stdout: '', stderr: '', all: ''}, { + command, + parsed, + timedOut: false, + isCanceled: false, + killed: false + })) + ); } const kill = spawned.kill.bind(spawned); @@ -407,21 +419,13 @@ const execa = (file, args, options) => { spawned.all = makeAllStream(spawned); - // eslint-disable-next-line promise/prefer-await-to-then - spawned.then = (onFulfilled, onRejected) => handlePromise().then(onFulfilled, onRejected); - spawned.catch = onRejected => handlePromise().catch(onRejected); spawned.cancel = () => { if (spawned.kill()) { isCanceled = true; } }; - // TODO: Remove the `if`-guard when targeting Node.js 10 - if (Promise.prototype.finally) { - spawned.finally = onFinally => handlePromise().finally(onFinally); - } - - return spawned; + return mergePromise(spawned, handlePromise); }; module.exports = execa; diff --git a/lib/merge.js b/lib/merge.js deleted file mode 100644 index 0913323747..0000000000 --- a/lib/merge.js +++ /dev/null @@ -1,24 +0,0 @@ -'use strict'; - -// Merge two objects, including their prototypes -function mergePrototypes(to, from) { - const prototypes = [...getPrototypes(to), ...getPrototypes(from)]; - const prototype = prototypes.reduce(shallowMerge, {}); - return Object.assign(Object.setPrototypeOf(to, prototype), from); -} - -function getPrototypes(object, prototypes = []) { - const prototype = Object.getPrototypeOf(object); - if (prototype !== null) { - return getPrototypes(prototype, [...prototypes, prototype]); - } - - return prototypes; -} - -function shallowMerge(toObject, fromObject) { - return Object.defineProperties(toObject, Object.getOwnPropertyDescriptors(fromObject)); -} - -module.exports = mergePrototypes; - From f141e4095d59789738d2c2b4c26de4dbb679942a Mon Sep 17 00:00:00 2001 From: ehmicky Date: Fri, 14 Jun 2019 10:00:00 +0200 Subject: [PATCH 4/6] Refactoring --- index.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index cb18bf280f..ba8b12b120 100644 --- a/index.js +++ b/index.js @@ -228,15 +228,18 @@ function joinCommand(file, args = []) { return [file, ...args].join(' '); } +function mergePromiseProperty(spawned, getPromise, property) { + spawned[property] = (...args) => getPromise()[property](...args); +} + // The return value is a mixin of `childProcess` and `Promise` function mergePromise(spawned, getPromise) { - // eslint-disable-next-line promise/prefer-await-to-then - spawned.then = (onFulfilled, onRejected) => getPromise().then(onFulfilled, onRejected); - spawned.catch = onRejected => getPromise().catch(onRejected); + mergePromiseProperty(spawned, getPromise, 'then'); + mergePromiseProperty(spawned, getPromise, 'catch'); // TODO: Remove the `if`-guard when targeting Node.js 10 if (Promise.prototype.finally) { - spawned.finally = onFinally => getPromise().finally(onFinally); + mergePromiseProperty(spawned, getPromise, 'finally'); } return spawned; From 47f8ae88030e46ca7f3104beb05e90c6179147f2 Mon Sep 17 00:00:00 2001 From: ehmicky Date: Fri, 14 Jun 2019 10:00:00 +0200 Subject: [PATCH 5/6] Make promise methods non-enumerable --- index.js | 9 ++++++++- test.js | 10 ++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index ba8b12b120..6f3d4b3a45 100644 --- a/index.js +++ b/index.js @@ -229,7 +229,14 @@ function joinCommand(file, args = []) { } function mergePromiseProperty(spawned, getPromise, property) { - spawned[property] = (...args) => getPromise()[property](...args); + Object.defineProperty(spawned, property, { + value(...args) { + return getPromise()[property](...args); + }, + writable: true, + enumerable: false, + configurable: true + }); } // The return value is a mixin of `childProcess` and `Promise` diff --git a/test.js b/test.js index 58da5af9f3..00bc252458 100644 --- a/test.js +++ b/test.js @@ -673,6 +673,16 @@ test('removes exit handler on exit', async t => { t.false(included); }); +test('promise methods are not enumerable', t => { + const promise = execa('noop'); + t.false(Object.getOwnPropertyDescriptor(promise, 'then').enumerable); + t.false(Object.getOwnPropertyDescriptor(promise, 'catch').enumerable); + // TOOD: Remove the `if`-guard when targeting Node.js 10 + if (Promise.prototype.finally) { + t.false(Object.getOwnPropertyDescriptor(promise, 'finally').enumerable); + } +}); + // TOOD: Remove the `if`-guard when targeting Node.js 10 if (Promise.prototype.finally) { test('finally function is executed on success', async t => { From ec4fd6f5f2f5383787056e5ca1c40566d04534a0 Mon Sep 17 00:00:00 2001 From: ehmicky Date: Fri, 14 Jun 2019 10:00:00 +0200 Subject: [PATCH 6/6] Improve test --- test.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test.js b/test.js index 00bc252458..18bee8c17f 100644 --- a/test.js +++ b/test.js @@ -674,12 +674,13 @@ test('removes exit handler on exit', async t => { }); test('promise methods are not enumerable', t => { - const promise = execa('noop'); - t.false(Object.getOwnPropertyDescriptor(promise, 'then').enumerable); - t.false(Object.getOwnPropertyDescriptor(promise, 'catch').enumerable); + const descriptors = Object.getOwnPropertyDescriptors(execa('noop')); + // eslint-disable-next-line promise/prefer-await-to-then + t.false(descriptors.then.enumerable); + t.false(descriptors.catch.enumerable); // TOOD: Remove the `if`-guard when targeting Node.js 10 if (Promise.prototype.finally) { - t.false(Object.getOwnPropertyDescriptor(promise, 'finally').enumerable); + t.false(descriptors.finally.enumerable); } });