From 66f8c042933feb2fd7ee59226083e66b8ea1033e Mon Sep 17 00:00:00 2001 From: Andreas Opferkuch Date: Sun, 13 Mar 2022 23:04:03 +0100 Subject: [PATCH 1/4] fix: kill other running tasks on failure --- lib/resolveTaskFn.js | 29 +++++++++++++++++++++++++--- package-lock.json | 17 ++++++++++++++++ package.json | 1 + test/__mocks__/execa.js | 4 +++- test/resolveTaskFn.spec.js | 22 +++++++++++++-------- test/resolveTaskFn.unmocked.spec.js | 24 +++++++++++++++++++++++ test/runAll.spec.js | 10 ++++++---- test/utils/createExecaReturnValue.js | 12 ++++++++++++ 8 files changed, 103 insertions(+), 16 deletions(-) create mode 100644 test/utils/createExecaReturnValue.js diff --git a/lib/resolveTaskFn.js b/lib/resolveTaskFn.js index f39e6017a..411960e3d 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' +export const FAIL_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. @@ -101,9 +104,29 @@ export const resolveTaskFn = ({ debugLog('execaOptions:', execaOptions) return async (ctx = getInitialState()) => { - const result = await (shell + let isExecutionDone = false + 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 interruptExecutionOnError = async () => { + if (!isExecutionDone) { + 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() + } else { + setTimeout(interruptExecutionOnError, FAIL_CHECK_INTERVAL) + } + } + } + interruptExecutionOnError() + + const result = await execaChildProcess + isExecutionDone = true 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/resolveTaskFn.spec.js b/test/resolveTaskFn.spec.js index 1c4e70e9f..041ba02ff 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, diff --git a/test/resolveTaskFn.unmocked.spec.js b/test/resolveTaskFn.unmocked.spec.js index 2727afcc3..5b371610b 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,27 @@ 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 -e "while(true) {}"', + 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 -e \\"while(true) {}\\" [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..884048812 --- /dev/null +++ b/test/utils/createExecaReturnValue.js @@ -0,0 +1,12 @@ +export function createExecaReturnValue(value, executionTime) { + const returnValue = { ...value } + const returnedPromise = executionTime + ? new Promise((resolve) => setTimeout(() => resolve(returnValue), executionTime)) + : Promise.resolve(returnValue) + + returnedPromise.kill = () => { + returnValue.killed = true + } + + return returnedPromise +} From 558ffb19eaa0e3ac4572c1f248ea0b10d1a5fac0 Mon Sep 17 00:00:00 2001 From: Andreas Opferkuch Date: Mon, 14 Mar 2022 23:06:18 +0100 Subject: [PATCH 2/4] minor improvement to the test --- test/resolveTaskFn.unmocked.spec.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/resolveTaskFn.unmocked.spec.js b/test/resolveTaskFn.unmocked.spec.js index 5b371610b..ecef88ec0 100644 --- a/test/resolveTaskFn.unmocked.spec.js +++ b/test/resolveTaskFn.unmocked.spec.js @@ -19,7 +19,7 @@ describe('resolveTaskFn', () => { const context = getInitialState() const taskFn = resolveTaskFn({ - command: 'node -e "while(true) {}"', + command: 'node', isFn: true, }) const taskPromise = taskFn(context) @@ -33,8 +33,6 @@ describe('resolveTaskFn', () => { await expect(task2Promise).rejects.toThrowErrorMatchingInlineSnapshot( `"node -e \\"process.exit(1)\\" [FAILED]"` ) - await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot( - `"node -e \\"while(true) {}\\" [KILLED]"` - ) + await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`) }) }) From 7739c9dad1f3068f5bf94d0fb448ba921464f36f Mon Sep 17 00:00:00 2001 From: Andreas Opferkuch Date: Mon, 14 Mar 2022 23:06:47 +0100 Subject: [PATCH 3/4] extract interrupt function, improve naming --- lib/resolveTaskFn.js | 50 +++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/lib/resolveTaskFn.js b/lib/resolveTaskFn.js index 411960e3d..77b093b1e 100644 --- a/lib/resolveTaskFn.js +++ b/lib/resolveTaskFn.js @@ -8,7 +8,7 @@ import { error, info } from './figures.js' import { getInitialState } from './state.js' import { TaskError } from './symbols.js' -export const FAIL_CHECK_INTERVAL = 200 +const ERROR_CHECK_INTERVAL = 200 const debugLog = debug('lint-staged:resolveTaskFn') @@ -46,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. * @@ -104,29 +132,13 @@ export const resolveTaskFn = ({ debugLog('execaOptions:', execaOptions) return async (ctx = getInitialState()) => { - let isExecutionDone = false const execaChildProcess = shell ? execa.command(isFn ? command : `${command} ${files.join(' ')}`, execaOptions) : execa(cmd, isFn ? args : args.concat(files), execaOptions) - const interruptExecutionOnError = async () => { - if (!isExecutionDone) { - 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() - } else { - setTimeout(interruptExecutionOnError, FAIL_CHECK_INTERVAL) - } - } - } - interruptExecutionOnError() - + const quitInterruptCheck = interruptExecutionOnError(ctx, execaChildProcess) const result = await execaChildProcess - isExecutionDone = true + quitInterruptCheck() if (result.failed || result.killed || result.signal != null) { throw makeErr(command, result, ctx) From feec1e7cabd884b82f2b4866a976209d56b41c35 Mon Sep 17 00:00:00 2001 From: Andreas Opferkuch Date: Tue, 15 Mar 2022 21:30:47 +0100 Subject: [PATCH 4/4] Add pidtree mock and another unit test --- test/__mocks__/pidtree.js | 3 +++ test/resolveTaskFn.spec.js | 25 +++++++++++++++++++++++++ test/utils/createExecaReturnValue.js | 10 +++++++++- 3 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 test/__mocks__/pidtree.js 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 041ba02ff..0f3cc29c0 100644 --- a/test/resolveTaskFn.spec.js +++ b/test/resolveTaskFn.spec.js @@ -327,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/utils/createExecaReturnValue.js b/test/utils/createExecaReturnValue.js index 884048812..766008243 100644 --- a/test/utils/createExecaReturnValue.js +++ b/test/utils/createExecaReturnValue.js @@ -1,11 +1,19 @@ export function createExecaReturnValue(value, executionTime) { const returnValue = { ...value } + let triggerResolve + let resolveTimeout + const returnedPromise = executionTime - ? new Promise((resolve) => setTimeout(() => resolve(returnValue), 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