Repository: sindresorhus/execa (js)

Issue:
Fix missing `stdout`/`stderr` data and unhandled promise rejections 
## Background

Currently, `childProcess.stdout` and `childProcess.stderr` are only consumed if `await childProcess` is called. This was implemented by #95 to solve #84. The main rationale is to avoid buffering `result.stdout` and `result.stderr` if the user does not use those, since this requires CPU, I/O and memory (potentially reaching the `maxBuffer` limit).

Eventually, the `buffer: false` option was also added for this use case.

The way this is implemented is that the process exit/error handling and stream consumption logic only starts after `await childProcess` has been called. Unfortunately, this implementation creates the following problems.

## Unhandled promise rejections

The above pattern encourages those users to not call `await childProcess`. However, this results in unhandled promise rejections.

Even when `await childProcess` is not called, Execa still creates a child process promise. When the child process fails, that promise is rejected. If `await childProcess` is not called, this results in an `unhandledRejection` event on the parent process, which crashes it. This also happens when the child process times out (`timeout` option) or when `childProcess.stdout` or `childProcess.stderr` abort or error.

## Missing `stdout`/`stderr` data if `await childProcess` is late

When users need to call the `childProcess.*` methods or access its properties, they need to delay calling `await childProcess`.

```js
const childProcess = execa(...)
childProcess.stdout.on('data', ...)
const result = await childProcess
```

This pattern becomes problematic when `await childProcess` is not done in the same microtask. For example:

```js
const childProcess = execa(...)
childProcess.stdout.on('data', ...)
await doSomethingAsync()
const result = await childProcess
```

When this happens, `childProcess.stdout|stderr` only starts being read when `await childProcess` is called. This means `result.stdout` will miss any data output while `doSomethingAsync()` was ongoing. Since that data is consumed by `childProcess.stdout.on('data', ...)`, it won't be buffered by Node.js. 

## Missing `stdout`/`stderr` data if the child process is fast

For similar reasons, `result.stdout` and `result.stderr` will miss data if the child process exits before `await childProcess` is called. This is more likely to happen if the child process has a short duration.

We have the following tests which show this problem.

https://github.com/sindresorhus/execa/blob/6cf1c5e7e07f4d25100fc04cffb03182974838e2/test/stream.js#L211-L225

## Solution

Overall, the pattern of avoiding to consume `childProcess.stdout` and `childProcess.stderr` by not calling `await childProcess` is not something we should recommend, for the above reasons. Instead, the `buffer: false` option should be used.

Most users do use `await childProcess` (right away or eventually). For the ones who do not, they will only notice a difference in terms of resources (CPU, I/O, memory), and only if they do not use `buffer: false`.

To fix the above problems, we should start consuming `childProcess.stdout` and `childProcess.stderr` as soon as the child process is spawned, before `await childProcess` is called.

PR at #658.

Pull request:
Fix unhandled rejections and missing data
Fixes #657.

Patch:
diff --git a/index.js b/index.js
index 512ebee..3993602 100644
--- a/index.js
+++ b/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,
diff --git a/lib/promise.js b/lib/promise.js
index 73b0171..0a33b69 100644
--- a/lib/promise.js
+++ b/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});
 	}
 };
diff --git a/package.json b/package.json
index b3a9ce3..2a24b8e 100644
--- a/package.json
+++ b/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 patch:
diff --git a/test/fixtures/no-await.js b/test/fixtures/no-await.js
new file mode 100755
index 0000000..79121ae
--- /dev/null
+++ b/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);
diff --git a/test/promise.js b/test/promise.js
index f2310c2..80ebeaa 100644
--- a/test/promise.js
+++ b/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');
diff --git a/test/stream.js b/test/stream.js
index cb8370d..64a1b72 100644
--- a/test/stream.js
+++ b/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);
 });