From 4d65029922dc5fdfb00dd0358099f127509af5cd Mon Sep 17 00:00:00 2001 From: ehmicky Date: Wed, 29 May 2019 10:00:00 +0200 Subject: [PATCH 1/4] Improve error handling of streams --- index.d.ts | 2 ++ index.js | 57 ++++++++++++++++++++++++++---------------------------- readme.md | 2 ++ test.js | 30 +++++++++++++++++++--------- 4 files changed, 52 insertions(+), 39 deletions(-) diff --git a/index.d.ts b/index.d.ts index 973c99afb..738c697a4 100644 --- a/index.d.ts +++ b/index.d.ts @@ -41,6 +41,8 @@ declare namespace execa { /** Buffer the output from the spawned process. When buffering is disabled you must consume the output of the `stdout` and `stderr` streams because the promise will not be resolved/rejected until they have completed. + If the spawned process fails, `error.stdout`, `error.stderr` and `error.all` will contain the buffered data. + @default true */ readonly buffer?: boolean; diff --git a/index.js b/index.js index 9d55dbc46..53074845a 100644 --- a/index.js +++ b/index.js @@ -106,6 +106,16 @@ function makeAllStream(spawned) { return mixed; } +function getBufferedData(stream, streamPromise) { + if (!stream) { + return; + } + + stream.destroy(); + + return streamPromise.catch(error => error.bufferedData); +} + function getStream(process, stream, {encoding, buffer, maxBuffer}) { if (!process[stream]) { return; @@ -129,11 +139,7 @@ function getStream(process, stream, {encoding, buffer, maxBuffer}) { ret = _getStream.buffer(process[stream], {maxBuffer}); } - return ret.catch(error => { - error.stream = stream; - error.message = `${stream} ${error.message}`; - throw error; - }); + return ret; } function makeError(result, options) { @@ -163,6 +169,10 @@ function makeError(result, options) { error.all = result.all; } + if ('bufferedData' in error) { + delete error.bufferedData; + } + error.failed = true; error.timedOut = timedOut; error.isCanceled = isCanceled; @@ -325,35 +335,23 @@ const execa = (file, args, options) => { } }), cleanup); - function destroy() { - if (spawned.stdout) { - spawned.stdout.destroy(); - } - - if (spawned.stderr) { - spawned.stderr.destroy(); - } - - if (spawned.all) { - spawned.all.destroy(); - } - } - const handlePromise = () => { - const processComplete = Promise.all([ - processDone, - getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}), - getStream(spawned, 'stderr', {encoding, buffer, maxBuffer}), - getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2}) - ]); + const stdoutPromise = getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}); + const stderrPromise = getStream(spawned, 'stderr', {encoding, buffer, maxBuffer}); + const allPromise = getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2}); const finalize = async () => { let results; try { - results = await processComplete; + results = await Promise.all([processDone, stdoutPromise, stderrPromise, allPromise]); } catch (error) { - const {stream, code, signal} = error; - results = [{error, stream, code, signal}, '', '', '']; + const {code, signal} = error; + results = await Promise.all([ + {error, code, signal}, + getBufferedData(spawned.stdout, stdoutPromise), + getBufferedData(spawned.stderr, stderrPromise), + getBufferedData(spawned.all, allPromise) + ]); } const [result, stdout, stderr, all] = results; @@ -392,8 +390,7 @@ const execa = (file, args, options) => { }; }; - // TODO: Use native "finally" syntax when targeting Node.js 10 - return pFinally(finalize(), destroy); + return finalize(); }; crossSpawn._enoent.hookChildProcess(spawned, parsed.parsed); diff --git a/readme.md b/readme.md index 6f8087eab..0b85f5ea2 100644 --- a/readme.md +++ b/readme.md @@ -282,6 +282,8 @@ Default: `true` Buffer the output from the spawned process. When buffering is disabled you must consume the output of the `stdout` and `stderr` streams because the promise will not be resolved/rejected until they have completed. +If the spawned process fails, [`error.stdout`](#stdout), [`error.stderr`](#stderr) and [`error.all`](#all) will contain the buffered data. + #### input Type: `string | Buffer | stream.Readable` diff --git a/test.js b/test.js index a572c3a26..94a095b62 100644 --- a/test.js +++ b/test.js @@ -65,11 +65,15 @@ test('stdout/stderr/all are undefined if ignored in sync mode', t => { t.is(all, undefined); }); +const WRONG_COMMAND = process.platform === 'win32' ? + '\'wrong\' is not recognized as an internal or external command,\r\noperable program or batch file.' : + ''; + test('stdout/stderr/all on process errors', async t => { const {stdout, stderr, all} = await t.throwsAsync(execa('wrong command')); t.is(stdout, ''); - t.is(stderr, ''); - t.is(all, ''); + t.is(stderr, WRONG_COMMAND); + t.is(all, WRONG_COMMAND); }); test('stdout/stderr/all on process errors, in sync mode', t => { @@ -77,9 +81,7 @@ test('stdout/stderr/all on process errors, in sync mode', t => { execa.sync('wrong command'); }); t.is(stdout, ''); - t.is(stderr, process.platform === 'win32' ? - '\'wrong\' is not recognized as an internal or external command,\r\noperable program or batch file.' : - ''); + t.is(stderr, WRONG_COMMAND); t.is(all, undefined); }); @@ -258,10 +260,16 @@ test('input option can be a Buffer - sync', t => { t.is(stdout, 'testing12'); }); +test('stdin errors are handled', async t => { + const child = execa('noop'); + child.stdin.emit('error', new Error('test')); + await t.throwsAsync(child, /test/); +}); + test('child process errors are handled', async t => { const child = execa('noop'); child.emit('error', new Error('test')); - await t.throwsAsync(child, /Command failed.*\ntest/); + await t.throwsAsync(child, /test/); }); test('opts.stdout:ignore - stdout will not collect data', async t => { @@ -306,13 +314,17 @@ test('child_process.spawnSync() errors are propagated with a correct shape', t = }); test('maxBuffer affects stdout', async t => { - await t.throwsAsync(execa('max-buffer', ['stdout', '11'], {maxBuffer: 10}), /stdout maxBuffer exceeded/); await t.notThrowsAsync(execa('max-buffer', ['stdout', '10'], {maxBuffer: 10})); + const {stdout, all} = await t.throwsAsync(execa('max-buffer', ['stdout', '11'], {maxBuffer: 10}), /max-buffer stdout/); + t.is(stdout, '.'.repeat(10)); + t.is(all, '.'.repeat(10)); }); test('maxBuffer affects stderr', async t => { - await t.throwsAsync(execa('max-buffer', ['stderr', '13'], {maxBuffer: 12}), /stderr maxBuffer exceeded/); - await t.notThrowsAsync(execa('max-buffer', ['stderr', '12'], {maxBuffer: 12})); + await t.notThrowsAsync(execa('max-buffer', ['stderr', '10'], {maxBuffer: 10})); + const {stderr, all} = await t.throwsAsync(execa('max-buffer', ['stderr', '11'], {maxBuffer: 10}), /max-buffer stderr/); + t.is(stderr, '.'.repeat(10)); + t.is(all, '.'.repeat(10)); }); test('do not buffer stdout when `buffer` set to `false`', async t => { From d0de844b09085cb726f129b4ac6c943c6b9c412e Mon Sep 17 00:00:00 2001 From: Sindre Sorhus Date: Fri, 14 Jun 2019 13:53:07 +0700 Subject: [PATCH 2/4] Update index.d.ts --- index.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.d.ts b/index.d.ts index 738c697a4..e5d640257 100644 --- a/index.d.ts +++ b/index.d.ts @@ -41,7 +41,7 @@ declare namespace execa { /** Buffer the output from the spawned process. When buffering is disabled you must consume the output of the `stdout` and `stderr` streams because the promise will not be resolved/rejected until they have completed. - If the spawned process fails, `error.stdout`, `error.stderr` and `error.all` will contain the buffered data. + If the spawned process fails, `error.stdout`, `error.stderr`, and `error.all` will contain the buffered data. @default true */ From 6e4a43a7033dcecd80a1712a44913e3341a703a9 Mon Sep 17 00:00:00 2001 From: Sindre Sorhus Date: Fri, 14 Jun 2019 13:53:45 +0700 Subject: [PATCH 3/4] Update readme.md --- readme.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readme.md b/readme.md index 0b85f5ea2..4ffbb3508 100644 --- a/readme.md +++ b/readme.md @@ -282,7 +282,7 @@ Default: `true` Buffer the output from the spawned process. When buffering is disabled you must consume the output of the `stdout` and `stderr` streams because the promise will not be resolved/rejected until they have completed. -If the spawned process fails, [`error.stdout`](#stdout), [`error.stderr`](#stderr) and [`error.all`](#all) will contain the buffered data. +If the spawned process fails, [`error.stdout`](#stdout), [`error.stderr`](#stderr), and [`error.all`](#all) will contain the buffered data. #### input From 0c417769a78b2d12238c3e7939502a3dd5911ea0 Mon Sep 17 00:00:00 2001 From: ehmicky Date: Fri, 14 Jun 2019 10:00:00 +0200 Subject: [PATCH 4/4] Use try/catch --- index.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index 53074845a..4482095cc 100644 --- a/index.js +++ b/index.js @@ -106,14 +106,18 @@ function makeAllStream(spawned) { return mixed; } -function getBufferedData(stream, streamPromise) { +async function getBufferedData(stream, streamPromise) { if (!stream) { return; } stream.destroy(); - return streamPromise.catch(error => error.bufferedData); + try { + return await streamPromise; + } catch (error) { + return error.bufferedData; + } } function getStream(process, stream, {encoding, buffer, maxBuffer}) {