Skip to content

Commit c508b46

Browse files
iirojiiroj
committed
perf: use EventsEmitter instead of setInterval for killing tasks on failure
1 parent 1a77e42 commit c508b46

File tree

6 files changed

+129
-14
lines changed

6 files changed

+129
-14
lines changed

‎lib/resolveTaskFn.js

+13-11
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { error, info } from './figures.js'
88
import { getInitialState } from './state.js'
99
import { TaskError } from './symbols.js'
1010

11-
const ERROR_CHECK_INTERVAL = 200
11+
const TASK_ERROR = 'lint-staged:taskError'
1212

1313
const debugLog = debug('lint-staged:resolveTaskFn')
1414

@@ -56,9 +56,10 @@ const killExecaProcess = async (execaProcess) => {
5656
for (const childPid of childPids) {
5757
process.kill(childPid)
5858
}
59-
} catch {
59+
} catch (error) {
6060
// Suppress "No matching pid found" error. This probably means
6161
// the process already died before executing.
62+
debugLog(`Failed to find process for pid %d`, execaProcess.pid)
6263
}
6364

6465
// The execa process is killed separately in order to get the `KILLED` status.
@@ -75,20 +76,17 @@ const killExecaProcess = async (execaProcess) => {
7576
* checks the context.
7677
*/
7778
const interruptExecutionOnError = (ctx, execaChildProcess) => {
78-
let intervalId, killPromise
79+
let killPromise
7980

80-
const loop = async () => {
81-
if (ctx.errors.size > 0) {
82-
clearInterval(intervalId)
83-
killPromise = killExecaProcess(execaChildProcess)
84-
await killPromise
85-
}
81+
const errorListener = async () => {
82+
killPromise = killExecaProcess(execaChildProcess)
83+
await killPromise
8684
}
8785

88-
intervalId = setInterval(loop, ERROR_CHECK_INTERVAL)
86+
ctx.events.on(TASK_ERROR, errorListener, { once: true })
8987

9088
return async () => {
91-
clearInterval(intervalId)
89+
ctx.events.off(TASK_ERROR, errorListener)
9290
await killPromise
9391
}
9492
}
@@ -108,6 +106,10 @@ const interruptExecutionOnError = (ctx, execaChildProcess) => {
108106
*/
109107
const makeErr = (command, result, ctx) => {
110108
ctx.errors.add(TaskError)
109+
110+
// https://nodejs.org/api/events.html#error-events
111+
ctx.events.emit(TASK_ERROR, TaskError)
112+
111113
handleOutput(command, result, ctx, true)
112114
const tag = getTag(result)
113115
return new Error(`${redBright(command)} ${dim(`[${tag}]`)}`)

‎lib/state.js

+3
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import EventEmitter from 'events'
2+
13
import { GIT_ERROR, TASK_ERROR } from './messages.js'
24
import {
35
ApplyEmptyCommitError,
@@ -11,6 +13,7 @@ export const getInitialState = ({ quiet = false } = {}) => ({
1113
hasPartiallyStagedFiles: null,
1214
shouldBackup: null,
1315
errors: new Set([]),
16+
events: new EventEmitter(),
1417
output: [],
1518
quiet,
1619
})

‎test/gitWorkflow.spec.js

+24
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,12 @@ describe('gitWorkflow', () => {
7070
"errors": Set {
7171
Symbol(GitError),
7272
},
73+
"events": EventEmitter {
74+
"_events": Object {},
75+
"_eventsCount": 0,
76+
"_maxListeners": undefined,
77+
Symbol(kCapture): false,
78+
},
7379
"hasPartiallyStagedFiles": true,
7480
"output": Array [],
7581
"quiet": false,
@@ -95,6 +101,12 @@ describe('gitWorkflow', () => {
95101
Symbol(GetBackupStashError),
96102
Symbol(GitError),
97103
},
104+
"events": EventEmitter {
105+
"_events": Object {},
106+
"_eventsCount": 0,
107+
"_maxListeners": undefined,
108+
Symbol(kCapture): false,
109+
},
98110
"hasPartiallyStagedFiles": null,
99111
"output": Array [],
100112
"quiet": false,
@@ -157,6 +169,12 @@ describe('gitWorkflow', () => {
157169
Symbol(GitError),
158170
Symbol(HideUnstagedChangesError),
159171
},
172+
"events": EventEmitter {
173+
"_events": Object {},
174+
"_eventsCount": 0,
175+
"_maxListeners": undefined,
176+
Symbol(kCapture): false,
177+
},
160178
"hasPartiallyStagedFiles": null,
161179
"output": Array [],
162180
"quiet": false,
@@ -202,6 +220,12 @@ describe('gitWorkflow', () => {
202220
Symbol(GitError),
203221
Symbol(RestoreMergeStatusError),
204222
},
223+
"events": EventEmitter {
224+
"_events": Object {},
225+
"_eventsCount": 0,
226+
"_maxListeners": undefined,
227+
Symbol(kCapture): false,
228+
},
205229
"hasPartiallyStagedFiles": null,
206230
"output": Array [],
207231
"quiet": false,

‎test/index2.spec.js

+12
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,12 @@ describe('lintStaged', () => {
4343
Object {
4444
"ctx": Object {
4545
"errors": Set {},
46+
"events": EventEmitter {
47+
"_events": Object {},
48+
"_eventsCount": 0,
49+
"_maxListeners": undefined,
50+
Symbol(kCapture): false,
51+
},
4652
"hasPartiallyStagedFiles": null,
4753
"output": Array [],
4854
"quiet": true,
@@ -70,6 +76,12 @@ describe('lintStaged', () => {
7076
Object {
7177
"ctx": Object {
7278
"errors": Set {},
79+
"events": EventEmitter {
80+
"_events": Object {},
81+
"_eventsCount": 0,
82+
"_maxListeners": undefined,
83+
Symbol(kCapture): false,
84+
},
7385
"hasPartiallyStagedFiles": null,
7486
"output": Array [],
7587
"quiet": false,

‎test/resolveTaskFn.spec.js

+59-3
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,12 @@ describe('resolveTaskFn', () => {
237237
expect(context).toMatchInlineSnapshot(`
238238
Object {
239239
"errors": Set {},
240+
"events": EventEmitter {
241+
"_events": Object {},
242+
"_eventsCount": 0,
243+
"_maxListeners": undefined,
244+
Symbol(kCapture): false,
245+
},
240246
"hasPartiallyStagedFiles": null,
241247
"output": Array [],
242248
"quiet": false,
@@ -263,6 +269,12 @@ describe('resolveTaskFn', () => {
263269
expect(context).toMatchInlineSnapshot(`
264270
Object {
265271
"errors": Set {},
272+
"events": EventEmitter {
273+
"_events": Object {},
274+
"_eventsCount": 0,
275+
"_maxListeners": undefined,
276+
Symbol(kCapture): false,
277+
},
266278
"hasPartiallyStagedFiles": null,
267279
"output": Array [
268280
"
@@ -295,6 +307,12 @@ describe('resolveTaskFn', () => {
295307
"errors": Set {
296308
Symbol(TaskError),
297309
},
310+
"events": EventEmitter {
311+
"_events": Object {},
312+
"_eventsCount": 0,
313+
"_maxListeners": undefined,
314+
Symbol(kCapture): false,
315+
},
298316
"hasPartiallyStagedFiles": null,
299317
"output": Array [
300318
"stderr",
@@ -325,6 +343,12 @@ describe('resolveTaskFn', () => {
325343
"errors": Set {
326344
Symbol(TaskError),
327345
},
346+
"events": EventEmitter {
347+
"_events": Object {},
348+
"_eventsCount": 0,
349+
"_maxListeners": undefined,
350+
Symbol(kCapture): false,
351+
},
328352
"hasPartiallyStagedFiles": null,
329353
"output": Array [],
330354
"quiet": true,
@@ -358,7 +382,38 @@ describe('resolveTaskFn', () => {
358382
await expect(taskPromise).resolves.toEqual()
359383
})
360384

361-
it('should kill a long running task when an error is added to the context', async () => {
385+
it('should ignore pid-tree errors', async () => {
386+
execa.mockImplementationOnce(() =>
387+
createExecaReturnValue(
388+
{
389+
stdout: 'a-ok',
390+
stderr: '',
391+
code: 0,
392+
cmd: 'mock cmd',
393+
failed: false,
394+
killed: false,
395+
signal: null,
396+
},
397+
1000
398+
)
399+
)
400+
401+
pidTree.mockImplementationOnce(() => {
402+
throw new Error('No matching pid found')
403+
})
404+
405+
const context = getInitialState()
406+
const taskFn = resolveTaskFn({ command: 'node' })
407+
const taskPromise = taskFn(context)
408+
409+
context.events.emit('lint-staged:taskError')
410+
411+
jest.runAllTimers()
412+
413+
await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`)
414+
})
415+
416+
it('should kill a long running task when error event is emitted', async () => {
362417
execa.mockImplementationOnce(() =>
363418
createExecaReturnValue(
364419
{
@@ -378,7 +433,7 @@ describe('resolveTaskFn', () => {
378433
const taskFn = resolveTaskFn({ command: 'node' })
379434
const taskPromise = taskFn(context)
380435

381-
context.errors.add({})
436+
context.events.emit('lint-staged:taskError')
382437

383438
jest.runAllTimers()
384439

@@ -416,7 +471,8 @@ describe('resolveTaskFn', () => {
416471
const context = getInitialState()
417472
const taskPromise = taskFn(context)
418473

419-
context.errors.add({})
474+
context.events.emit('lint-staged:taskError')
475+
420476
jest.runAllTimers()
421477

422478
await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`)

‎test/runAll.spec.js

+18
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,12 @@ describe('runAll', () => {
6161
await expect(runAll({ configObject: {}, configPath })).resolves.toMatchInlineSnapshot(`
6262
Object {
6363
"errors": Set {},
64+
"events": EventEmitter {
65+
"_events": Object {},
66+
"_eventsCount": 0,
67+
"_maxListeners": undefined,
68+
Symbol(kCapture): false,
69+
},
6470
"hasPartiallyStagedFiles": null,
6571
"output": Array [
6672
"→ No staged files found.",
@@ -92,6 +98,12 @@ describe('runAll', () => {
9298
await expect(runAll({ configObject: {}, configPath })).resolves.toMatchInlineSnapshot(`
9399
Object {
94100
"errors": Set {},
101+
"events": EventEmitter {
102+
"_events": Object {},
103+
"_eventsCount": 0,
104+
"_maxListeners": undefined,
105+
Symbol(kCapture): false,
106+
},
95107
"hasPartiallyStagedFiles": null,
96108
"output": Array [
97109
"→ No staged files found.",
@@ -108,6 +120,12 @@ describe('runAll', () => {
108120
.toMatchInlineSnapshot(`
109121
Object {
110122
"errors": Set {},
123+
"events": EventEmitter {
124+
"_events": Object {},
125+
"_eventsCount": 0,
126+
"_maxListeners": undefined,
127+
Symbol(kCapture): false,
128+
},
111129
"hasPartiallyStagedFiles": null,
112130
"output": Array [],
113131
"quiet": true,

0 commit comments

Comments
 (0)