Skip to content

Commit

Permalink
perf: use EventsEmitter instead of setInterval for killing tasks …
Browse files Browse the repository at this point in the history 
…on failure
  • Loading branch information
@iiroj
iiroj committed Jun 8, 2022
1 parent 1a77e42 commit c508b46
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 14 deletions.
24 changes: 13 additions & 11 deletions lib/resolveTaskFn.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand Down Expand Up @@ -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.
Expand All @@ -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
}
}
Expand All @@ -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}]`)}`)
Expand Down
3 changes: 3 additions & 0 deletions lib/state.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import EventEmitter from 'events'

import { GIT_ERROR, TASK_ERROR } from './messages.js'
import {
ApplyEmptyCommitError,
Expand All @@ -11,6 +13,7 @@ export const getInitialState = ({ quiet = false } = {}) => ({
hasPartiallyStagedFiles: null,
shouldBackup: null,
errors: new Set([]),
events: new EventEmitter(),
output: [],
quiet,
})
Expand Down
24 changes: 24 additions & 0 deletions test/gitWorkflow.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
12 changes: 12 additions & 0 deletions test/index2.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
62 changes: 59 additions & 3 deletions test/resolveTaskFn.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 [
"
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(
{
Expand All @@ -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()

Expand Down Expand Up @@ -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]"`)
Expand Down
18 changes: 18 additions & 0 deletions test/runAll.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down Expand Up @@ -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.",
Expand All @@ -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,
Expand Down

0 comments on commit c508b46

Please sign in to comment.