From a97e8fb3642af1c9a04f2c43e59376c0b9375c20 Mon Sep 17 00:00:00 2001 From: Guillaume Martigny Date: Wed, 3 Apr 2019 16:02:25 +0200 Subject: [PATCH 1/4] Fix timeout option not leaving early when on `sleep` mode Fix #157 Add regression test --- fixtures/sleeper | 4 ++ index.js | 111 +++++++++++++++++++++++++++++------------------ test.js | 17 +++++++- 3 files changed, 88 insertions(+), 44 deletions(-) create mode 100755 fixtures/sleeper diff --git a/fixtures/sleeper b/fixtures/sleeper new file mode 100755 index 000000000..ab9343b88 --- /dev/null +++ b/fixtures/sleeper @@ -0,0 +1,4 @@ +#!/bin/bash + +sleep 5 +echo "ok" diff --git a/index.js b/index.js index 92bfc6afc..9da2248dc 100644 --- a/index.js +++ b/index.js @@ -267,9 +267,25 @@ const execa = (command, args, options) => { }, parsed.options.timeout); } + const resolvable = (() => { + let extracted; + const promise = new Promise(resolve => { + extracted = resolve; + }); + promise.resolve = extracted; + return promise; + })(); + const processDone = new Promise(resolve => { spawned.on('exit', (code, signal) => { cleanup(); + + if (timedOut) { + resolvable.resolve([ + {code, signal}, '', '', '' + ]); + } + resolve({code, signal}); }); @@ -301,51 +317,62 @@ const execa = (command, args, options) => { } // TODO: Use native "finally" syntax when targeting Node.js 10 - const handlePromise = () => pFinally(Promise.all([ - processDone, - getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}), - getStream(spawned, 'stderr', {encoding, buffer, maxBuffer}), - getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2}) - ]).then(results => { // eslint-disable-line promise/prefer-await-to-then - const result = results[0]; - result.stdout = results[1]; - result.stderr = results[2]; - result.all = results[3]; - - if (result.error || result.code !== 0 || result.signal !== null || isCanceled) { - const error = makeError(result, { - joinedCommand, - parsed, - timedOut, - isCanceled - }); - - // TODO: missing some timeout logic for killed - // https://github.com/nodejs/node/blob/master/lib/child_process.js#L203 - // error.killed = spawned.killed || killed; - error.killed = error.killed || spawned.killed; + const handlePromise = () => { + let processComplete = Promise.all([ + processDone, + getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}), + getStream(spawned, 'stderr', {encoding, buffer, maxBuffer}), + getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2}) + ]); + + if (timeoutId) { + processComplete = Promise.race([ + processComplete, + resolvable + ]); + } - if (!parsed.options.reject) { - return error; + return pFinally(processComplete.then(results => { // eslint-disable-line promise/prefer-await-to-then + const result = results[0]; + result.stdout = results[1]; + result.stderr = results[2]; + result.all = results[3]; + + if (result.error || result.code !== 0 || result.signal !== null || isCanceled) { + const error = makeError(result, { + joinedCommand, + parsed, + timedOut, + isCanceled + }); + + // TODO: missing some timeout logic for killed + // https://github.com/nodejs/node/blob/master/lib/child_process.js#L203 + // error.killed = spawned.killed || killed; + error.killed = error.killed || spawned.killed; + + if (!parsed.options.reject) { + return error; + } + + throw error; } - throw error; - } - - return { - stdout: handleOutput(parsed.options, result.stdout), - stderr: handleOutput(parsed.options, result.stderr), - all: handleOutput(parsed.options, result.all), - code: 0, - exitCode: 0, - exitCodeName: 'SUCCESS', - failed: false, - killed: false, - command: joinedCommand, - timedOut: false, - isCanceled: false - }; - }), destroy); + return { + stdout: handleOutput(parsed.options, result.stdout), + stderr: handleOutput(parsed.options, result.stderr), + all: handleOutput(parsed.options, result.all), + code: 0, + exitCode: 0, + exitCodeName: 'SUCCESS', + failed: false, + killed: false, + command: joinedCommand, + timedOut: false, + isCanceled: false + }; + }), destroy); + }; crossSpawn._enoent.hookChildProcess(spawned, parsed.parsed); diff --git a/test.js b/test.js index eba24b6a1..dc603485a 100644 --- a/test.js +++ b/test.js @@ -386,14 +386,27 @@ test('error.code is 3', code, 3); test('error.code is 4', code, 4); test('timeout will kill the process early', async t => { - const error = await t.throwsAsync(execa('delay', ['60000', '0'], {timeout: 1500, message: TIMEOUT_REGEXP})); + const time = Date.now(); + const error = await t.throwsAsync(execa('delay', ['60000', '0'], {timeout: 500, message: TIMEOUT_REGEXP})); + const diff = Date.now() - time; t.true(error.timedOut); t.not(error.exitCode, 22); + t.true(diff < 2000); +}); + +test('timeout will kill the process early (sleep)', async t => { + const time = Date.now(); + const error = await t.throwsAsync(execa('sleeper', [], {timeout: 500, message: TIMEOUT_REGEXP})); + const diff = Date.now() - time; + + t.true(error.timedOut); + t.not(error.stdout, 'ok'); + t.true(diff < 2000); }); test('timeout will not kill the process early', async t => { - const error = await t.throwsAsync(execa('delay', ['3000', '22'], {timeout: 30000}), {code: 22, message: getExitRegExp('22')}); + const error = await t.throwsAsync(execa('delay', ['2000', '22'], {timeout: 30000}), {code: 22, message: getExitRegExp('22')}); t.false(error.timedOut); }); From aee46e7b64155da68c5170887f7cbd57cccd627c Mon Sep 17 00:00:00 2001 From: Guillaume Martigny Date: Wed, 3 Apr 2019 16:50:15 +0200 Subject: [PATCH 2/4] Extends expected timeout margin --- test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test.js b/test.js index dc603485a..ed4d6f90c 100644 --- a/test.js +++ b/test.js @@ -392,7 +392,7 @@ test('timeout will kill the process early', async t => { t.true(error.timedOut); t.not(error.exitCode, 22); - t.true(diff < 2000); + t.true(diff < 4000); }); test('timeout will kill the process early (sleep)', async t => { @@ -402,7 +402,7 @@ test('timeout will kill the process early (sleep)', async t => { t.true(error.timedOut); t.not(error.stdout, 'ok'); - t.true(diff < 2000); + t.true(diff < 4000); }); test('timeout will not kill the process early', async t => { From 582f2b002e2e64c79c30582a77d11f7abda3c7a9 Mon Sep 17 00:00:00 2001 From: Guillaume Martigny Date: Wed, 3 Apr 2019 17:07:11 +0200 Subject: [PATCH 3/4] Run time depend tests serially --- test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test.js b/test.js index ed4d6f90c..b05d0d4d6 100644 --- a/test.js +++ b/test.js @@ -385,7 +385,7 @@ test('error.code is 2', code, 2); test('error.code is 3', code, 3); test('error.code is 4', code, 4); -test('timeout will kill the process early', async t => { +test.serial('timeout will kill the process early', async t => { const time = Date.now(); const error = await t.throwsAsync(execa('delay', ['60000', '0'], {timeout: 500, message: TIMEOUT_REGEXP})); const diff = Date.now() - time; @@ -395,7 +395,7 @@ test('timeout will kill the process early', async t => { t.true(diff < 4000); }); -test('timeout will kill the process early (sleep)', async t => { +test.serial('timeout will kill the process early (sleep)', async t => { const time = Date.now(); const error = await t.throwsAsync(execa('sleeper', [], {timeout: 500, message: TIMEOUT_REGEXP})); const diff = Date.now() - time; From fd6bec8cc313508cd042260b8606e85ed39b8460 Mon Sep 17 00:00:00 2001 From: Guillaume Martigny Date: Fri, 12 Apr 2019 11:25:16 +0200 Subject: [PATCH 4/4] Extract final step to function to remove lint warning --- index.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index 9da2248dc..94ff30758 100644 --- a/index.js +++ b/index.js @@ -316,7 +316,6 @@ const execa = (command, args, options) => { } } - // TODO: Use native "finally" syntax when targeting Node.js 10 const handlePromise = () => { let processComplete = Promise.all([ processDone, @@ -332,7 +331,9 @@ const execa = (command, args, options) => { ]); } - return pFinally(processComplete.then(results => { // eslint-disable-line promise/prefer-await-to-then + const finalize = async () => { + const results = await processComplete; + const result = results[0]; result.stdout = results[1]; result.stderr = results[2]; @@ -371,7 +372,10 @@ const execa = (command, args, options) => { timedOut: false, isCanceled: false }; - }), destroy); + }; + + // TODO: Use native "finally" syntax when targeting Node.js 10 + return pFinally(finalize(), destroy); }; crossSpawn._enoent.hookChildProcess(spawned, parsed.parsed);