Refactor subprocess.kill(error)

lib/convert/concurrent.js

@@ -1,4 +1,4 @@
-import {createDeferred} from './shared.js';
+import {createDeferred} from '../utils/deferred.js';
 
 // When using multiple `.readable()`/`.writable()`/`.duplex()`, `final` and `destroy` should wait for other streams
 export const initializeConcurrentStreams = () => ({

lib/convert/readable.js

@@ -3,9 +3,9 @@ import {callbackify} from 'node:util';
 import {BINARY_ENCODINGS} from '../arguments/encoding-option.js';
 import {getFromStream} from '../arguments/fd-options.js';
 import {iterateOnSubprocessStream, DEFAULT_OBJECT_HIGH_WATER_MARK} from '../io/iterate.js';
+import {createDeferred} from '../utils/deferred.js';
 import {addConcurrentStream, waitForConcurrentStreams} from './concurrent.js';
 import {
-	createDeferred,
 	safeWaitForSubprocessStdin,
 	waitForSubprocessStdout,
 	waitForSubprocess,

lib/convert/shared.js

@@ -44,11 +44,3 @@ export const destroyOtherStream = (stream, isOpen, error) => {
 		stream.destroy();
 	}
 };
-
-export const createDeferred = () => {
-	let resolve;
-	const promise = new Promise(resolve_ => {
-		resolve = resolve_;
-	});
-	return Object.assign(promise, {resolve});
-};

lib/methods/main-async.js

@@ -17,6 +17,7 @@ import {logEarlyResult} from '../verbose/complete.js';
 import {makeAllStream} from '../resolve/all-async.js';
 import {waitForSubprocessResult} from '../resolve/wait-subprocess.js';
 import {addConvertedStreams} from '../convert/add.js';
+import {createDeferred} from '../utils/deferred.js';
 import {mergePromise} from './promise.js';
 
 // Main shared logic for all async methods: `execa()`, `execaCommand()`, `$`, `execaNode()`
@@ -100,10 +101,11 @@ const spawnSubprocessAsync = ({file, commandArguments, options, startTime, verbo
 	pipeOutputAsync(subprocess, fileDescriptors, controller);
 	cleanupOnExit(subprocess, options, controller);
 
+	const onInternalError = createDeferred();
 	subprocess.kill = subprocessKill.bind(undefined, {
 		kill: subprocess.kill.bind(subprocess),
-		subprocess,
 		options,
+		onInternalError,
 		controller,
 	});
 	subprocess.all = makeAllStream(subprocess, options);
@@ -118,13 +120,14 @@ const spawnSubprocessAsync = ({file, commandArguments, options, startTime, verbo
 		originalStreams,
 		command,
 		escapedCommand,
+		onInternalError,
 		controller,
 	});
 	return {subprocess, promise};
 };
 
 // Asynchronous logic, as opposed to the previous logic which can be run synchronously, i.e. can be returned to user right away
-const handlePromise = async ({subprocess, options, startTime, verboseInfo, fileDescriptors, originalStreams, command, escapedCommand, controller}) => {
+const handlePromise = async ({subprocess, options, startTime, verboseInfo, fileDescriptors, originalStreams, command, escapedCommand, onInternalError, controller}) => {
 	const context = {timedOut: false};
 
 	const [errorInfo, [exitCode, signal], stdioResults, allResult] = await waitForSubprocessResult({
@@ -134,9 +137,11 @@ const handlePromise = async ({subprocess, options, startTime, verboseInfo, fileD
 		verboseInfo,
 		fileDescriptors,
 		originalStreams,
+		onInternalError,
 		controller,
 	});
 	controller.abort();
+	onInternalError.resolve();
 
 	const stdio = stdioResults.map((stdioResult, fdNumber) => stripNewline(stdioResult, options, fdNumber));
 	const all = stripNewline(allResult, options, 'all');

lib/resolve/wait-subprocess.js

@@ -1,6 +1,5 @@
 import {once} from 'node:events';
 import {isStream as isNodeStream} from 'is-stream';
-import {errorSignal} from '../terminate/kill.js';
 import {throwOnTimeout} from '../terminate/timeout.js';
 import {isStandardStream} from '../utils/standard-stream.js';
 import {TRANSFORM_TYPES} from '../stdio/type.js';
@@ -18,6 +17,7 @@ export const waitForSubprocessResult = async ({
 	verboseInfo,
 	fileDescriptors,
 	originalStreams,
+	onInternalError,
 	controller,
 }) => {
 	const exitPromise = waitForExit(subprocess);
@@ -62,8 +62,8 @@ export const waitForSubprocessResult = async ({
 				...originalPromises,
 				...customStreamsEndPromises,
 			]),
+			onInternalError,
 			throwOnSubprocessError(subprocess, controller),
-			throwOnInternalError(subprocess, controller),
 			...throwOnTimeout(subprocess, timeout, context, controller),
 		]);
 	} catch (error) {
@@ -100,10 +100,3 @@ const throwOnSubprocessError = async (subprocess, {signal}) => {
 	const [error] = await once(subprocess, 'error', {signal});
 	throw error;
 };
-
-// Fails right away when calling `subprocess.kill(error)`.
-// Does not wait for actual signal termination.
-const throwOnInternalError = async (subprocess, {signal}) => {
-	const [error] = await once(subprocess, errorSignal, {signal});
-	throw error;
-};

lib/terminate/kill.js

@@ -23,12 +23,12 @@ const DEFAULT_FORCE_KILL_TIMEOUT = 1000 * 5;
 
 // Monkey-patches `subprocess.kill()` to add `forceKillAfterDelay` behavior and `.kill(error)`
 export const subprocessKill = (
-	{kill, subprocess, options: {forceKillAfterDelay, killSignal}, controller},
+	{kill, options: {forceKillAfterDelay, killSignal}, onInternalError, controller},
 	signalOrError,
 	errorArgument,
 ) => {
 	const {signal, error} = parseKillArguments(signalOrError, errorArgument, killSignal);
-	emitKillError(subprocess, error);
+	emitKillError(error, onInternalError);
 	const killResult = kill(signal);
 	setKillTimeout({
 		kill,
@@ -57,16 +57,15 @@ const parseKillArguments = (signalOrError, errorArgument, killSignal) => {
 	return {signal, error};
 };
 
-const emitKillError = (subprocess, error) => {
+// Fails right away when calling `subprocess.kill(error)`.
+// Does not wait for actual signal termination.
+// Uses a deferred promise instead of the `error` event on the subprocess, as this is less intrusive.
+const emitKillError = (error, onInternalError) => {
 	if (error !== undefined) {
-		subprocess.emit(errorSignal, error);
+		onInternalError.reject(error);
 	}
 };
 
-// Like `error` signal but internal to Execa.
-// E.g. does not make subprocess crash when no `error` listener is set.
-export const errorSignal = Symbol('error');
-
 const setKillTimeout = async ({kill, signal, forceKillAfterDelay, killSignal, killResult, controller}) => {
 	if (!shouldForceKill(signal, forceKillAfterDelay, killSignal, killResult)) {
 		return;

lib/utils/deferred.js

@@ -0,0 +1,7 @@
+export const createDeferred = () => {
+	const methods = {};
+	const promise = new Promise((resolve, reject) => {
+		Object.assign(methods, {resolve, reject});
+	});
+	return Object.assign(promise, methods);
+};

test/resolve/wait-subprocess.js

@@ -19,7 +19,7 @@ test('stdio[*] is undefined if ignored - sync', testIgnore, 3, execaSync);
 
 const testSubprocessEventsCleanup = async (t, fixtureName) => {
 	const subprocess = execa(fixtureName, {reject: false});
-	t.deepEqual(subprocess.eventNames().map(String).sort(), ['Symbol(error)', 'error', 'exit', 'spawn']);
+	t.deepEqual(subprocess.eventNames().map(String).sort(), ['error', 'exit', 'spawn']);
 	await subprocess;
 	t.deepEqual(subprocess.eventNames(), []);
 };