Fix timeout option not leaving early when on sleep mode (#199)

Fix #157
sindresorhus · Apr 12, 2019 · 136b340 · 136b340
1 parent 27c22e5
commit 136b340
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 44 deletions.
diff --git a/fixtures/sleeper b/fixtures/sleeper
@@ -0,0 +1,4 @@
+#!/bin/bash
+
+sleep 5
+echo "ok"
diff --git a/index.js b/index.js
@@ -267,9 +267,25 @@ const execa = (command, args, options) => {
 		}, parsed.options.timeout);
 	}
 
+	const resolvable = (() => {
+		let extracted;
+		const promise = new Promise(resolve => {
+			extracted = resolve;
+		});
+		promise.resolve = extracted;
+		return promise;
+	})();
+
 	const processDone = new Promise(resolve => {
 		spawned.on('exit', (code, signal) => {
 			cleanup();
+
+			if (timedOut) {
+				resolvable.resolve([
+					{code, signal}, '', '', ''
+				]);
+			}
+
 			resolve({code, signal});
 		});
 
@@ -300,52 +316,67 @@ const execa = (command, args, options) => {
 		}
 	}
 
-	// TODO: Use native "finally" syntax when targeting Node.js 10
-	const handlePromise = () => pFinally(Promise.all([
-		processDone,
-		getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}),
-		getStream(spawned, 'stderr', {encoding, buffer, maxBuffer}),
-		getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2})
-	]).then(results => { // eslint-disable-line promise/prefer-await-to-then
-		const result = results[0];
-		result.stdout = results[1];
-		result.stderr = results[2];
-		result.all = results[3];
-
-		if (result.error || result.code !== 0 || result.signal !== null || isCanceled) {
-			const error = makeError(result, {
-				joinedCommand,
-				parsed,
-				timedOut,
-				isCanceled
-			});
+	const handlePromise = () => {
+		let processComplete = Promise.all([
+			processDone,
+			getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}),
+			getStream(spawned, 'stderr', {encoding, buffer, maxBuffer}),
+			getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2})
+		]);
+
+		if (timeoutId) {
+			processComplete = Promise.race([
+				processComplete,
+				resolvable
+			]);
+		}
 
-			// TODO: missing some timeout logic for killed
-			// https://github.com/nodejs/node/blob/master/lib/child_process.js#L203
-			// error.killed = spawned.killed || killed;
-			error.killed = error.killed || spawned.killed;
+		const finalize = async () => {
+			const results = await processComplete;
 
-			if (!parsed.options.reject) {
-				return error;
-			}
+			const result = results[0];
+			result.stdout = results[1];
+			result.stderr = results[2];
+			result.all = results[3];
 
-			throw error;
-		}
+			if (result.error || result.code !== 0 || result.signal !== null || isCanceled) {
+				const error = makeError(result, {
+					joinedCommand,
+					parsed,
+					timedOut,
+					isCanceled
+				});
+
+				// TODO: missing some timeout logic for killed
+				// https://github.com/nodejs/node/blob/master/lib/child_process.js#L203
+				// error.killed = spawned.killed || killed;
+				error.killed = error.killed || spawned.killed;
 
-		return {
-			stdout: handleOutput(parsed.options, result.stdout),
-			stderr: handleOutput(parsed.options, result.stderr),
-			all: handleOutput(parsed.options, result.all),
-			code: 0,
-			exitCode: 0,
-			exitCodeName: 'SUCCESS',
-			failed: false,
-			killed: false,
-			command: joinedCommand,
-			timedOut: false,
-			isCanceled: false
+				if (!parsed.options.reject) {
+					return error;
+				}
+
+				throw error;
+			}
+
+			return {
+				stdout: handleOutput(parsed.options, result.stdout),
+				stderr: handleOutput(parsed.options, result.stderr),
+				all: handleOutput(parsed.options, result.all),
+				code: 0,
+				exitCode: 0,
+				exitCodeName: 'SUCCESS',
+				failed: false,
+				killed: false,
+				command: joinedCommand,
+				timedOut: false,
+				isCanceled: false
+			};
 		};
-	}), destroy);
+
+		// TODO: Use native "finally" syntax when targeting Node.js 10
+		return pFinally(finalize(), destroy);
+	};
 
 	crossSpawn._enoent.hookChildProcess(spawned, parsed.parsed);
 

diff --git a/test.js b/test.js
@@ -385,15 +385,28 @@ test('error.code is 2', code, 2);
 test('error.code is 3', code, 3);
 test('error.code is 4', code, 4);
 
-test('timeout will kill the process early', async t => {
-	const error = await t.throwsAsync(execa('delay', ['60000', '0'], {timeout: 1500, message: TIMEOUT_REGEXP}));
+test.serial('timeout will kill the process early', async t => {
+	const time = Date.now();
+	const error = await t.throwsAsync(execa('delay', ['60000', '0'], {timeout: 500, message: TIMEOUT_REGEXP}));
+	const diff = Date.now() - time;
 
 	t.true(error.timedOut);
 	t.not(error.exitCode, 22);
+	t.true(diff < 4000);
+});
+
+test.serial('timeout will kill the process early (sleep)', async t => {
+	const time = Date.now();
+	const error = await t.throwsAsync(execa('sleeper', [], {timeout: 500, message: TIMEOUT_REGEXP}));
+	const diff = Date.now() - time;
+
+	t.true(error.timedOut);
+	t.not(error.stdout, 'ok');
+	t.true(diff < 4000);
 });
 
 test('timeout will not kill the process early', async t => {
-	const error = await t.throwsAsync(execa('delay', ['3000', '22'], {timeout: 30000}), {code: 22, message: getExitRegExp('22')});
+	const error = await t.throwsAsync(execa('delay', ['2000', '22'], {timeout: 30000}), {code: 22, message: getExitRegExp('22')});
 	t.false(error.timedOut);
 });