From c508b46a153970114495d3f7fef05d45df0f2e10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iiro=20J=C3=A4ppinen?= Date: Wed, 8 Jun 2022 13:41:52 +0300 Subject: [PATCH] perf: use `EventsEmitter` instead of `setInterval` for killing tasks on failure --- lib/resolveTaskFn.js | 24 ++++++++------- lib/state.js | 3 ++ test/gitWorkflow.spec.js | 24 +++++++++++++++ test/index2.spec.js | 12 ++++++++ test/resolveTaskFn.spec.js | 62 ++++++++++++++++++++++++++++++++++++-- test/runAll.spec.js | 18 +++++++++++ 6 files changed, 129 insertions(+), 14 deletions(-) diff --git a/lib/resolveTaskFn.js b/lib/resolveTaskFn.js index 03cdea39e..d62ab7395 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' -const ERROR_CHECK_INTERVAL = 200 +const TASK_ERROR = 'lint-staged:taskError' const debugLog = debug('lint-staged:resolveTaskFn') @@ -56,9 +56,10 @@ const killExecaProcess = async (execaProcess) => { for (const childPid of childPids) { process.kill(childPid) } - } catch { + } catch (error) { // Suppress "No matching pid found" error. This probably means // the process already died before executing. + debugLog(`Failed to find process for pid %d`, execaProcess.pid) } // The execa process is killed separately in order to get the `KILLED` status. @@ -75,20 +76,17 @@ const killExecaProcess = async (execaProcess) => { * checks the context. */ const interruptExecutionOnError = (ctx, execaChildProcess) => { - let intervalId, killPromise + let killPromise - const loop = async () => { - if (ctx.errors.size > 0) { - clearInterval(intervalId) - killPromise = killExecaProcess(execaChildProcess) - await killPromise - } + const errorListener = async () => { + killPromise = killExecaProcess(execaChildProcess) + await killPromise } - intervalId = setInterval(loop, ERROR_CHECK_INTERVAL) + ctx.events.on(TASK_ERROR, errorListener, { once: true }) return async () => { - clearInterval(intervalId) + ctx.events.off(TASK_ERROR, errorListener) await killPromise } } @@ -108,6 +106,10 @@ const interruptExecutionOnError = (ctx, execaChildProcess) => { */ const makeErr = (command, result, ctx) => { ctx.errors.add(TaskError) + + // https://nodejs.org/api/events.html#error-events + ctx.events.emit(TASK_ERROR, TaskError) + handleOutput(command, result, ctx, true) const tag = getTag(result) return new Error(`${redBright(command)} ${dim(`[${tag}]`)}`) diff --git a/lib/state.js b/lib/state.js index 0e34e5c4c..30f63c913 100644 --- a/lib/state.js +++ b/lib/state.js @@ -1,3 +1,5 @@ +import EventEmitter from 'events' + import { GIT_ERROR, TASK_ERROR } from './messages.js' import { ApplyEmptyCommitError, @@ -11,6 +13,7 @@ export const getInitialState = ({ quiet = false } = {}) => ({ hasPartiallyStagedFiles: null, shouldBackup: null, errors: new Set([]), + events: new EventEmitter(), output: [], quiet, }) diff --git a/test/gitWorkflow.spec.js b/test/gitWorkflow.spec.js index 7346b2701..6d51a48e1 100644 --- a/test/gitWorkflow.spec.js +++ b/test/gitWorkflow.spec.js @@ -70,6 +70,12 @@ describe('gitWorkflow', () => { "errors": Set { Symbol(GitError), }, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": true, "output": Array [], "quiet": false, @@ -95,6 +101,12 @@ describe('gitWorkflow', () => { Symbol(GetBackupStashError), Symbol(GitError), }, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [], "quiet": false, @@ -157,6 +169,12 @@ describe('gitWorkflow', () => { Symbol(GitError), Symbol(HideUnstagedChangesError), }, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [], "quiet": false, @@ -202,6 +220,12 @@ describe('gitWorkflow', () => { Symbol(GitError), Symbol(RestoreMergeStatusError), }, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [], "quiet": false, diff --git a/test/index2.spec.js b/test/index2.spec.js index 383f8bb3f..ee75539b2 100644 --- a/test/index2.spec.js +++ b/test/index2.spec.js @@ -43,6 +43,12 @@ describe('lintStaged', () => { Object { "ctx": Object { "errors": Set {}, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [], "quiet": true, @@ -70,6 +76,12 @@ describe('lintStaged', () => { Object { "ctx": Object { "errors": Set {}, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [], "quiet": false, diff --git a/test/resolveTaskFn.spec.js b/test/resolveTaskFn.spec.js index d5d1d6906..b6e79e1ab 100644 --- a/test/resolveTaskFn.spec.js +++ b/test/resolveTaskFn.spec.js @@ -237,6 +237,12 @@ describe('resolveTaskFn', () => { expect(context).toMatchInlineSnapshot(` Object { "errors": Set {}, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [], "quiet": false, @@ -263,6 +269,12 @@ describe('resolveTaskFn', () => { expect(context).toMatchInlineSnapshot(` Object { "errors": Set {}, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [ " @@ -295,6 +307,12 @@ describe('resolveTaskFn', () => { "errors": Set { Symbol(TaskError), }, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [ "stderr", @@ -325,6 +343,12 @@ describe('resolveTaskFn', () => { "errors": Set { Symbol(TaskError), }, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [], "quiet": true, @@ -358,7 +382,38 @@ describe('resolveTaskFn', () => { await expect(taskPromise).resolves.toEqual() }) - it('should kill a long running task when an error is added to the context', async () => { + it('should ignore pid-tree errors', async () => { + execa.mockImplementationOnce(() => + createExecaReturnValue( + { + stdout: 'a-ok', + stderr: '', + code: 0, + cmd: 'mock cmd', + failed: false, + killed: false, + signal: null, + }, + 1000 + ) + ) + + pidTree.mockImplementationOnce(() => { + throw new Error('No matching pid found') + }) + + const context = getInitialState() + const taskFn = resolveTaskFn({ command: 'node' }) + const taskPromise = taskFn(context) + + context.events.emit('lint-staged:taskError') + + jest.runAllTimers() + + await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`) + }) + + it('should kill a long running task when error event is emitted', async () => { execa.mockImplementationOnce(() => createExecaReturnValue( { @@ -378,7 +433,7 @@ describe('resolveTaskFn', () => { const taskFn = resolveTaskFn({ command: 'node' }) const taskPromise = taskFn(context) - context.errors.add({}) + context.events.emit('lint-staged:taskError') jest.runAllTimers() @@ -416,7 +471,8 @@ describe('resolveTaskFn', () => { const context = getInitialState() const taskPromise = taskFn(context) - context.errors.add({}) + context.events.emit('lint-staged:taskError') + jest.runAllTimers() await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`) diff --git a/test/runAll.spec.js b/test/runAll.spec.js index 5c1e7420b..99b9434e0 100644 --- a/test/runAll.spec.js +++ b/test/runAll.spec.js @@ -61,6 +61,12 @@ describe('runAll', () => { await expect(runAll({ configObject: {}, configPath })).resolves.toMatchInlineSnapshot(` Object { "errors": Set {}, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [ "→ No staged files found.", @@ -92,6 +98,12 @@ describe('runAll', () => { await expect(runAll({ configObject: {}, configPath })).resolves.toMatchInlineSnapshot(` Object { "errors": Set {}, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [ "→ No staged files found.", @@ -108,6 +120,12 @@ describe('runAll', () => { .toMatchInlineSnapshot(` Object { "errors": Set {}, + "events": EventEmitter { + "_events": Object {}, + "_eventsCount": 0, + "_maxListeners": undefined, + Symbol(kCapture): false, + }, "hasPartiallyStagedFiles": null, "output": Array [], "quiet": true,