Fix unhandled rejections and missing data (#658)

Author: ehmicky

index.js

@@ -6,7 +6,6 @@ import {fileURLToPath} from 'node:url';
 import crossSpawn from 'cross-spawn';
 import stripFinalNewline from 'strip-final-newline';
 import {npmRunPathEnv} from 'npm-run-path';
-import onetime from 'onetime';
 import {makeError} from './lib/error.js';
 import {handleInputAsync, pipeOutputAsync} from './lib/stdio/async.js';
 import {handleInputSync, pipeOutputSync} from './lib/stdio/sync.js';
@@ -127,27 +126,27 @@ export function execa(rawFile, rawArgs, rawOptions) {
 		return dummySpawned;
 	}
 
-	const spawnedPromise = getSpawnedPromise(spawned);
-	const timedPromise = setupTimeout(spawned, options, spawnedPromise);
-	const processDone = setExitHandler(spawned, options, timedPromise);
-
 	const context = {isCanceled: false};
 
 	spawned.kill = spawnedKill.bind(null, spawned.kill.bind(spawned));
 	spawned.cancel = spawnedCancel.bind(null, spawned, context);
 
-	const handlePromiseOnce = onetime(handlePromise.bind(undefined, {spawned, options, context, stdioStreams, command, escapedCommand, processDone}));
-
 	pipeOutputAsync(spawned, stdioStreams);
 
 	spawned.all = makeAllStream(spawned, options);
 
 	addPipeMethods(spawned);
-	mergePromise(spawned, handlePromiseOnce);
+
+	const promise = handlePromise({spawned, options, context, stdioStreams, command, escapedCommand});
+	mergePromise(spawned, promise);
 	return spawned;
 }
 
-const handlePromise = async ({spawned, options, context, stdioStreams, command, escapedCommand, processDone}) => {
+const handlePromise = async ({spawned, options, context, stdioStreams, command, escapedCommand}) => {
+	const spawnedPromise = getSpawnedPromise(spawned);
+	const timedPromise = setupTimeout(spawned, options, spawnedPromise);
+	const processDone = setExitHandler(spawned, options, timedPromise);
+
 	const [
 		{error, exitCode, signal, timedOut},
 		stdoutResult,

lib/promise.js

@@ -11,11 +11,7 @@ const descriptors = ['then', 'catch', 'finally'].map(property => [
 // The return value is a mixin of `childProcess` and `Promise`
 export const mergePromise = (spawned, promise) => {
 	for (const [property, descriptor] of descriptors) {
-		// Starting the main `promise` is deferred to avoid consuming streams
-		const value = typeof promise === 'function'
-			? (...args) => Reflect.apply(descriptor.value, promise(), args)
-			: descriptor.value.bind(promise);
-
+		const value = descriptor.value.bind(promise);
 		Reflect.defineProperty(spawned, property, {...descriptor, value});
 	}
 };

package.json

@@ -51,7 +51,6 @@
 		"human-signals": "^6.0.0",
 		"is-stream": "^3.0.0",
 		"npm-run-path": "^5.2.0",
-		"onetime": "^7.0.0",
 		"signal-exit": "^4.1.0",
 		"strip-final-newline": "^4.0.0"
 	},

test/fixtures/no-await.js

@@ -0,0 +1,9 @@
+#!/usr/bin/env node
+import process from 'node:process';
+import {once} from 'node:events';
+import {execa} from '../../index.js';
+
+const [options, file, ...args] = process.argv.slice(2);
+execa(file, args, JSON.parse(options));
+const [error] = await once(process, 'unhandledRejection');
+console.log(error.shortMessage);

test/promise.js

@@ -43,3 +43,11 @@ test('throw in finally bubbles up on error', async t => {
 	}));
 	t.is(message, 'called');
 });
+
+const testNoAwait = async (t, fixtureName, options, message) => {
+	const {stdout} = await execa('no-await.js', [JSON.stringify(options), fixtureName]);
+	t.true(stdout.includes(message));
+};
+
+test('Throws if promise is not awaited and process fails', testNoAwait, 'fail.js', {}, 'exit code 2');
+test('Throws if promise is not awaited and process times out', testNoAwait, 'forever.js', {timeout: 1}, 'timed out');

test/stream.js

@@ -1,5 +1,6 @@
 import {Buffer} from 'node:buffer';
 import {exec} from 'node:child_process';
+import {once} from 'node:events';
 import process from 'node:process';
 import {setTimeout} from 'node:timers/promises';
 import {promisify} from 'node:util';
@@ -131,12 +132,20 @@ const testNoMaxBuffer = async (t, streamName) => {
 test('do not buffer stdout when `buffer` set to `false`', testNoMaxBuffer, 'stdout');
 test('do not buffer stderr when `buffer` set to `false`', testNoMaxBuffer, 'stderr');
 
-test('do not buffer when streaming', async t => {
-	const {stdout} = execa('max-buffer.js', ['stdout', '21'], {maxBuffer: 10});
+test('do not buffer when streaming and `buffer` is `false`', async t => {
+	const {stdout} = execa('max-buffer.js', ['stdout', '21'], {maxBuffer: 10, buffer: false});
 	const result = await getStream(stdout);
 	t.is(result, '....................\n');
 });
 
+test('buffers when streaming and `buffer` is `true`', async t => {
+	const childProcess = execa('max-buffer.js', ['stdout', '21'], {maxBuffer: 10});
+	await Promise.all([
+		t.throwsAsync(childProcess, {message: /maxBuffer exceeded/}),
+		t.throwsAsync(getStream(childProcess.stdout), {code: 'ABORT_ERR'}),
+	]);
+});
+
 test('buffer: false > promise resolves', async t => {
 	await t.notThrowsAsync(execa('noop.js', {buffer: false}));
 });
@@ -208,18 +217,31 @@ test.serial('Processes buffer stdout before it is read', async t => {
 	t.is(stdout, 'foobar');
 });
 
-// This test is not the desired behavior, but is the current one.
-// I.e. this is mostly meant for documentation and regression testing.
-test.serial('Processes might successfully exit before their stdout is read', async t => {
+test.serial('Processes buffers stdout right away, on successfully exit', async t => {
 	const childProcess = execa('noop.js', ['foobar']);
 	await setTimeout(1e3);
 	const {stdout} = await childProcess;
-	t.is(stdout, '');
+	t.is(stdout, 'foobar');
 });
 
-test.serial('Processes might fail before their stdout is read', async t => {
+test.serial('Processes buffers stdout right away, on failure', async t => {
 	const childProcess = execa('noop-fail.js', ['foobar'], {reject: false});
 	await setTimeout(1e3);
 	const {stdout} = await childProcess;
-	t.is(stdout, '');
+	t.is(stdout, 'foobar');
+});
+
+test('Processes buffers stdout right away, even if directly read', async t => {
+	const childProcess = execa('noop.js', ['foobar']);
+	const data = await once(childProcess.stdout, 'data');
+	t.is(data.toString().trim(), 'foobar');
+	const {stdout} = await childProcess;
+	t.is(stdout, 'foobar');
+});
+
+test('childProcess.stdout|stderr must be read right away', async t => {
+	const childProcess = execa('noop.js', ['foobar']);
+	const {stdout} = await childProcess;
+	t.is(stdout, 'foobar');
+	t.true(childProcess.stdout.destroyed);
 });