Refactor the timeout option (#333)

sindresorhus · Jun 27, 2019 · 2a7551f · 2a7551f
1 parent 9020f28
commit 2a7551f
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 25 deletions.
diff --git a/index.js b/index.js
@@ -94,21 +94,23 @@ const execa = (file, args, options) => {
 		return mergePromise(dummySpawned, errorPromise);
 	}
 
-	const context = {timedOut: false, isCanceled: false};
+	const spawnedPromise = getSpawnedPromise(spawned);
+
+	const context = {isCanceled: false};
 
 	spawned.kill = spawnedKill.bind(null, spawned.kill.bind(spawned));
 	spawned.cancel = spawnedCancel.bind(null, spawned, context);
 
-	const timeoutId = setupTimeout(spawned, parsed.options, context);
+	const timedPromise = setupTimeout(spawned, parsed.options, spawnedPromise);
 	const removeExitHandler = setExitHandler(spawned, parsed.options);
 
 	// TODO: Use native "finally" syntax when targeting Node.js 10
-	const processDone = pFinally(getSpawnedPromise(spawned, context), () => {
-		cleanup(timeoutId, removeExitHandler);
+	const processDone = pFinally(timedPromise, () => {
+		cleanup(removeExitHandler);
 	});
 
 	const handlePromise = async () => {
-		const [{error, code, signal}, stdoutResult, stderrResult, allResult] = await getSpawnedResult(spawned, parsed.options, processDone);
+		const [{error, code, signal, timedOut}, stdoutResult, stderrResult, allResult] = await getSpawnedResult(spawned, parsed.options, processDone);
 		const stdout = handleOutput(parsed.options, stdoutResult);
 		const stderr = handleOutput(parsed.options, stderrResult);
 		const all = handleOutput(parsed.options, allResult);
@@ -123,7 +125,7 @@ const execa = (file, args, options) => {
 				all,
 				command,
 				parsed,
-				timedOut: context.timedOut,
+				timedOut,
 				isCanceled: context.isCanceled,
 				killed: spawned.killed
 			});

diff --git a/lib/error.js b/lib/error.js
@@ -74,7 +74,7 @@ const makeError = ({
 	}
 
 	error.failed = true;
-	error.timedOut = timedOut;
+	error.timedOut = Boolean(timedOut);
 	error.isCanceled = isCanceled;
 	error.killed = killed && !timedOut;
 	// `signal` emitted on `spawned.on('exit')` event can be `null`. We normalize

diff --git a/lib/kill.js b/lib/kill.js
@@ -1,6 +1,7 @@
 'use strict';
 const os = require('os');
 const onExit = require('signal-exit');
+const pFinally = require('p-finally');
 
 const DEFAULT_FORCE_KILL_TIMEOUT = 1000 * 5;
 
@@ -52,14 +53,33 @@ const spawnedCancel = (spawned, context) => {
 	}
 };
 
+const timeoutKill = (spawned, signal, reject) => {
+	spawned.kill(signal);
+	reject(Object.assign(new Error('Timed out'), {timedOut: true, signal}));
+};
+
 // `timeout` option handling
-const setupTimeout = (spawned, {timeout, killSignal}, context) => {
-	if (timeout > 0) {
-		return setTimeout(() => {
-			context.timedOut = true;
-			spawned.kill(killSignal);
-		}, timeout);
+const setupTimeout = (spawned, {timeout, killSignal = 'SIGTERM'}, spawnedPromise) => {
+	if (timeout === 0 || timeout === undefined) {
+		return spawnedPromise;
+	}
+
+	if (!Number.isInteger(timeout) || timeout < 0) {
+		throw new TypeError(`Expected the \`timeout\` option to be a non-negative integer, got \`${timeout}\` (${typeof timeout})`);
 	}
+
+	let timeoutId;
+	const timeoutPromise = new Promise((resolve, reject) => {
+		timeoutId = setTimeout(() => {
+			timeoutKill(spawned, killSignal, reject);
+		}, timeout);
+	});
+
+	const safeSpawnedPromise = pFinally(spawnedPromise, () => {
+		clearTimeout(timeoutId);
+	});
+
+	return Promise.race([timeoutPromise, safeSpawnedPromise]);
 };
 
 // `cleanup` option handling
@@ -73,11 +93,7 @@ const setExitHandler = (spawned, {cleanup, detached}) => {
 	});
 };
 
-const cleanup = (timeoutId, removeExitHandler) => {
-	if (timeoutId !== undefined) {
-		clearTimeout(timeoutId);
-	}
-
+const cleanup = removeExitHandler => {
 	if (removeExitHandler !== undefined) {
 		removeExitHandler();
 	}

diff --git a/lib/promise.js b/lib/promise.js
@@ -27,14 +27,9 @@ const mergePromise = (spawned, promise) => {
 };
 
 // Use promises instead of `child_process` events
-const getSpawnedPromise = (spawned, context) => {
+const getSpawnedPromise = spawned => {
 	return new Promise((resolve, reject) => {
 		spawned.on('exit', (code, signal) => {
-			if (context.timedOut) {
-				reject(Object.assign(new Error('Timed out'), {code, signal}));
-				return;
-			}
-
 			resolve({code, signal});
 		});
 

diff --git a/lib/stream.js b/lib/stream.js
@@ -83,7 +83,7 @@ const getSpawnedResult = async ({stdout, stderr, all}, {encoding, buffer, maxBuf
 		return await Promise.all([processDone, stdoutPromise, stderrPromise, allPromise]);
 	} catch (error) {
 		return Promise.all([
-			{error, code: error.code, signal: error.signal},
+			{error, code: error.code, signal: error.signal, timedOut: error.timedOut},
 			getBufferedData(stdout, stdoutPromise),
 			getBufferedData(stderr, stderrPromise),
 			getBufferedData(all, allPromise)

diff --git a/test/kill.js b/test/kill.js
@@ -98,6 +98,20 @@ test('timeout does not kill the process if it does not time out', async t => {
 	t.false(timedOut);
 });
 
+const INVALID_TIMEOUT_REGEXP = /`timeout` option to be a non-negative integer/;
+
+test('timeout must not be negative', async t => {
+	await t.throws(() => {
+		execa('noop', {timeout: -1});
+	}, INVALID_TIMEOUT_REGEXP);
+});
+
+test('timeout must be an integer', async t => {
+	await t.throws(() => {
+		execa('noop', {timeout: false});
+	}, INVALID_TIMEOUT_REGEXP);
+});
+
 test('timedOut is false if no timeout was set', async t => {
 	const {timedOut} = await execa('noop');
 	t.false(timedOut);