From 34fe31986201983c33ea2bde41f4b451947b826b Mon Sep 17 00:00:00 2001 From: Andreas Opferkuch Date: Wed, 16 Mar 2022 07:09:24 +0100 Subject: [PATCH] fix: kill other running tasks on failure (#1117) --- lib/resolveTaskFn.js | 41 ++++++++++++++++++++++-- package-lock.json | 17 ++++++++++ package.json | 1 + test/__mocks__/execa.js | 4 ++- test/__mocks__/pidtree.js | 3 ++ test/resolveTaskFn.spec.js | 47 +++++++++++++++++++++++----- test/resolveTaskFn.unmocked.spec.js | 22 +++++++++++++ test/runAll.spec.js | 10 +++--- test/utils/createExecaReturnValue.js | 20 ++++++++++++ 9 files changed, 149 insertions(+), 16 deletions(-) create mode 100644 test/__mocks__/pidtree.js create mode 100644 test/utils/createExecaReturnValue.js diff --git a/lib/resolveTaskFn.js b/lib/resolveTaskFn.js index f39e6017a..77b093b1e 100644 --- a/lib/resolveTaskFn.js +++ b/lib/resolveTaskFn.js @@ -2,14 +2,17 @@ import { redBright, dim } from 'colorette' import execa from 'execa' import debug from 'debug' import { parseArgsStringToArgv } from 'string-argv' +import pidTree from 'pidtree' import { error, info } from './figures.js' import { getInitialState } from './state.js' import { TaskError } from './symbols.js' +const ERROR_CHECK_INTERVAL = 200 + const debugLog = debug('lint-staged:resolveTaskFn') -const getTag = ({ code, killed, signal }) => signal || (killed && 'KILLED') || code || 'FAILED' +const getTag = ({ code, killed, signal }) => (killed && 'KILLED') || signal || code || 'FAILED' /** * Handle task console output. @@ -43,6 +46,34 @@ const handleOutput = (command, result, ctx, isError = false) => { } } +/** + * Interrupts the execution of the execa process that we spawned if + * another task adds an error to the context. + * + * @param {Object} ctx + * @param {execa.ExecaChildProcess} execaChildProcess + * @returns {function(): void} Function that clears the interval that + * checks the context. + */ +const interruptExecutionOnError = (ctx, execaChildProcess) => { + async function loop() { + if (ctx.errors.size > 0) { + const ids = await pidTree(execaChildProcess.pid) + ids.forEach(process.kill) + + // The execa process is killed separately in order + // to get the `KILLED` status. + execaChildProcess.kill() + } + } + + const loopIntervalId = setInterval(loop, ERROR_CHECK_INTERVAL) + + return () => { + clearInterval(loopIntervalId) + } +} + /** * Create a error output dependding on process result. * @@ -101,9 +132,13 @@ export const resolveTaskFn = ({ debugLog('execaOptions:', execaOptions) return async (ctx = getInitialState()) => { - const result = await (shell + const execaChildProcess = shell ? execa.command(isFn ? command : `${command} ${files.join(' ')}`, execaOptions) - : execa(cmd, isFn ? args : args.concat(files), execaOptions)) + : execa(cmd, isFn ? args : args.concat(files), execaOptions) + + const quitInterruptCheck = interruptExecutionOnError(ctx, execaChildProcess) + const result = await execaChildProcess + quitInterruptCheck() if (result.failed || result.killed || result.signal != null) { throw makeErr(command, result, ctx) diff --git a/package-lock.json b/package-lock.json index 0ffda7f07..93375dceb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -19,6 +19,7 @@ "micromatch": "^4.0.4", "normalize-path": "^3.0.0", "object-inspect": "^1.12.0", + "pidtree": "^0.5.0", "string-argv": "^0.3.1", "supports-color": "^9.2.1", "yaml": "^1.10.2" @@ -7494,6 +7495,17 @@ "url": "https://github.com/sponsors/jonschlinkert" } }, + "node_modules/pidtree": { + "version": "0.5.0", + "resolved": "https://registry.npmjs.org/pidtree/-/pidtree-0.5.0.tgz", + "integrity": "sha512-9nxspIM7OpZuhBxPg73Zvyq7j1QMPMPsGKTqRc2XOaFQauDvoNz9fM1Wdkjmeo7l9GXOZiRs97sPkuayl39wjA==", + "bin": { + "pidtree": "bin/pidtree.js" + }, + "engines": { + "node": ">=0.10" + } + }, "node_modules/pirates": { "version": "4.0.4", "resolved": "https://registry.npmjs.org/pirates/-/pirates-4.0.4.tgz", @@ -14258,6 +14270,11 @@ "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-2.3.0.tgz", "integrity": "sha512-lY1Q/PiJGC2zOv/z391WOTD+Z02bCgsFfvxoXXf6h7kv9o+WmsmzYqrAwY63sNgOxE4xEdq0WyUnXfKeBrSvYw==" }, + "pidtree": { + "version": "0.5.0", + "resolved": "https://registry.npmjs.org/pidtree/-/pidtree-0.5.0.tgz", + "integrity": "sha512-9nxspIM7OpZuhBxPg73Zvyq7j1QMPMPsGKTqRc2XOaFQauDvoNz9fM1Wdkjmeo7l9GXOZiRs97sPkuayl39wjA==" + }, "pirates": { "version": "4.0.4", "resolved": "https://registry.npmjs.org/pirates/-/pirates-4.0.4.tgz", diff --git a/package.json b/package.json index dfbc35936..0ff97d1a1 100644 --- a/package.json +++ b/package.json @@ -42,6 +42,7 @@ "micromatch": "^4.0.4", "normalize-path": "^3.0.0", "object-inspect": "^1.12.0", + "pidtree": "^0.5.0", "string-argv": "^0.3.1", "supports-color": "^9.2.1", "yaml": "^1.10.2" diff --git a/test/__mocks__/execa.js b/test/__mocks__/execa.js index 4f204a37d..5d0d45078 100644 --- a/test/__mocks__/execa.js +++ b/test/__mocks__/execa.js @@ -1,5 +1,7 @@ +import { createExecaReturnValue } from '../utils/createExecaReturnValue' + const execa = jest.fn(() => - Promise.resolve({ + createExecaReturnValue({ stdout: 'a-ok', stderr: '', code: 0, diff --git a/test/__mocks__/pidtree.js b/test/__mocks__/pidtree.js new file mode 100644 index 000000000..26ca35289 --- /dev/null +++ b/test/__mocks__/pidtree.js @@ -0,0 +1,3 @@ +module.exports = () => { + return Promise.resolve([]) +} diff --git a/test/resolveTaskFn.spec.js b/test/resolveTaskFn.spec.js index 1c4e70e9f..0f3cc29c0 100644 --- a/test/resolveTaskFn.spec.js +++ b/test/resolveTaskFn.spec.js @@ -4,8 +4,14 @@ import { resolveTaskFn } from '../lib/resolveTaskFn' import { getInitialState } from '../lib/state' import { TaskError } from '../lib/symbols' +import { createExecaReturnValue } from './utils/createExecaReturnValue' + const defaultOpts = { files: ['test.js'] } +function mockExecaImplementationOnce(value) { + execa.mockImplementationOnce(() => createExecaReturnValue(value)) +} + describe('resolveTaskFn', () => { beforeEach(() => { execa.mockClear() @@ -135,7 +141,7 @@ describe('resolveTaskFn', () => { it('should throw error for failed linters', async () => { expect.assertions(1) - execa.mockResolvedValueOnce({ + mockExecaImplementationOnce({ stdout: 'Mock error', stderr: '', code: 0, @@ -149,7 +155,7 @@ describe('resolveTaskFn', () => { it('should throw error for interrupted processes', async () => { expect.assertions(1) - execa.mockResolvedValueOnce({ + mockExecaImplementationOnce({ stdout: 'Mock error', stderr: '', code: 0, @@ -167,7 +173,7 @@ describe('resolveTaskFn', () => { it('should throw error for killed processes without signal', async () => { expect.assertions(1) - execa.mockResolvedValueOnce({ + mockExecaImplementationOnce({ stdout: 'Mock error', stderr: '', code: 0, @@ -192,7 +198,7 @@ describe('resolveTaskFn', () => { }) it('should add TaskError on error', async () => { - execa.mockResolvedValueOnce({ + mockExecaImplementationOnce({ stdout: 'Mock error', stderr: '', code: 0, @@ -210,7 +216,7 @@ describe('resolveTaskFn', () => { it('should not add output when there is none', async () => { expect.assertions(2) - execa.mockResolvedValueOnce({ + mockExecaImplementationOnce({ stdout: '', stderr: '', code: 0, @@ -236,7 +242,7 @@ describe('resolveTaskFn', () => { it('should add output even when task succeeds if `verbose: true`', async () => { expect.assertions(2) - execa.mockResolvedValueOnce({ + mockExecaImplementationOnce({ stdout: 'Mock success', stderr: '', code: 0, @@ -266,7 +272,7 @@ describe('resolveTaskFn', () => { it('should not add title to output when task errors while quiet', async () => { expect.assertions(2) - execa.mockResolvedValueOnce({ + mockExecaImplementationOnce({ stdout: '', stderr: 'stderr', code: 1, @@ -296,7 +302,7 @@ describe('resolveTaskFn', () => { it('should not print anything when task errors without output while quiet', async () => { expect.assertions(2) - execa.mockResolvedValueOnce({ + mockExecaImplementationOnce({ stdout: '', stderr: '', code: 1, @@ -321,4 +327,29 @@ describe('resolveTaskFn', () => { } `) }) + + it('should kill a long running task when an error is added to the context', async () => { + execa.mockImplementationOnce(() => + createExecaReturnValue( + { + stdout: 'a-ok', + stderr: '', + code: 0, + cmd: 'mock cmd', + failed: false, + killed: false, + signal: null, + }, + 1000 + ) + ) + + const context = getInitialState() + const taskFn = resolveTaskFn({ command: 'node' }) + const taskPromise = taskFn(context) + + context.errors.add({}) + + await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`) + }) }) diff --git a/test/resolveTaskFn.unmocked.spec.js b/test/resolveTaskFn.unmocked.spec.js index 2727afcc3..ecef88ec0 100644 --- a/test/resolveTaskFn.unmocked.spec.js +++ b/test/resolveTaskFn.unmocked.spec.js @@ -1,4 +1,5 @@ import { resolveTaskFn } from '../lib/resolveTaskFn' +import { getInitialState } from '../lib/state' jest.unmock('execa') @@ -13,4 +14,25 @@ describe('resolveTaskFn', () => { await expect(taskFn()).resolves.toMatchInlineSnapshot(`undefined`) }) + + it('should kill a long running task when another fails', async () => { + const context = getInitialState() + + const taskFn = resolveTaskFn({ + command: 'node', + isFn: true, + }) + const taskPromise = taskFn(context) + + const taskFn2 = resolveTaskFn({ + command: 'node -e "process.exit(1)"', + isFn: true, + }) + const task2Promise = taskFn2(context) + + await expect(task2Promise).rejects.toThrowErrorMatchingInlineSnapshot( + `"node -e \\"process.exit(1)\\" [FAILED]"` + ) + await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`) + }) }) diff --git a/test/runAll.spec.js b/test/runAll.spec.js index 84fa9c4ae..6865939de 100644 --- a/test/runAll.spec.js +++ b/test/runAll.spec.js @@ -12,6 +12,8 @@ import { ConfigNotFoundError, GitError } from '../lib/symbols' import * as searchConfigsNS from '../lib/searchConfigs' import * as getConfigGroupsNS from '../lib/getConfigGroups' +import { createExecaReturnValue } from './utils/createExecaReturnValue' + jest.mock('../lib/file') jest.mock('../lib/getStagedFiles') jest.mock('../lib/gitWorkflow') @@ -192,7 +194,7 @@ describe('runAll', () => { expect.assertions(2) getStagedFiles.mockImplementationOnce(async () => ['sample.js']) execa.mockImplementation(() => - Promise.resolve({ + createExecaReturnValue({ stdout: '', stderr: 'Linter finished with error', code: 1, @@ -229,7 +231,7 @@ describe('runAll', () => { expect.assertions(2) getStagedFiles.mockImplementationOnce(async () => ['sample.js']) execa.mockImplementation(() => - Promise.resolve({ + createExecaReturnValue({ stdout: '', stderr: '', code: 0, @@ -252,8 +254,8 @@ describe('runAll', () => { LOG [STARTED] — 1 file LOG [STARTED] *.js — 1 file LOG [STARTED] echo \\"sample\\" - ERROR [FAILED] echo \\"sample\\" [SIGINT] - ERROR [FAILED] echo \\"sample\\" [SIGINT] + ERROR [FAILED] echo \\"sample\\" [KILLED] + ERROR [FAILED] echo \\"sample\\" [KILLED] LOG [SUCCESS] Running tasks for staged files... LOG [STARTED] Applying modifications from tasks... INFO [SKIPPED] Skipped because of errors from tasks. diff --git a/test/utils/createExecaReturnValue.js b/test/utils/createExecaReturnValue.js new file mode 100644 index 000000000..766008243 --- /dev/null +++ b/test/utils/createExecaReturnValue.js @@ -0,0 +1,20 @@ +export function createExecaReturnValue(value, executionTime) { + const returnValue = { ...value } + let triggerResolve + let resolveTimeout + + const returnedPromise = executionTime + ? new Promise((resolve) => { + triggerResolve = resolve.bind(null, returnValue) + resolveTimeout = setTimeout(triggerResolve, executionTime) + }) + : Promise.resolve(returnValue) + + returnedPromise.kill = () => { + returnValue.killed = true + clearTimeout(resolveTimeout) + triggerResolve() + } + + return returnedPromise +}