fix review finding: exitsWithin cleans up its listener and timer on both arms
Each timed-out wait used to leave the once('exit') listener from its inner
waitForExit attached to the child; the dispose ladder accumulates at most a
couple, but in a shared library a caller polling exitsWithin in a loop would
pile listeners onto one child (MaxListenersExceededWarning at 11) and retain
their closures. The race now owns its wiring: the timeout arm removes the
exit listener, the exit arm clears the (still unref'ed) grace timer, and an
already-exited child short-circuits true without attaching anything. Tests
pin listenerCount('exit') === 0 after every outcome.
This commit is contained in:
@@ -16,7 +16,7 @@ Spawn-failure capture: a promise that resolves (never rejects) with the child's
|
|||||||
|
|
||||||
### `waitForExit(child)` / `exitsWithin(child, ms)`
|
### `waitForExit(child)` / `exitsWithin(child, ms)`
|
||||||
|
|
||||||
Exit waits over a `ChildProcess`: resolve once the child exits by any code or signal (immediately if it is already gone), or race that against a timer (`true` = exited in time; the pending timer is `unref()`ed so a grace window never keeps the parent's event loop alive).
|
Exit waits over a `ChildProcess`: resolve once the child exits by any code or signal (immediately if it is already gone), or race that against a timer (`true` = exited in time). The race cleans up after itself on both outcomes — the pending timer is `unref()`ed and cleared on exit, the exit listener removed on timeout — so repeated calls (the dispose ladder's tiers, a poll loop) never accumulate listeners on the child.
|
||||||
|
|
||||||
### `disposeChildProcess(child, graces)`
|
### `disposeChildProcess(child, graces)`
|
||||||
|
|
||||||
|
|||||||
@@ -78,17 +78,29 @@ export function waitForExit(child: ChildProcess): Promise<void> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Race the child's exit against a timer.
|
* Race the child's exit against a timer. Neither outcome leaves anything
|
||||||
|
* behind on the child: the exit listener is removed on timeout and the timer
|
||||||
|
* is cleared on exit, so repeated calls (the dispose ladder's tiers, a poll
|
||||||
|
* loop) never accumulate listeners.
|
||||||
* @param child - the child process to watch.
|
* @param child - the child process to watch.
|
||||||
* @param ms - the wait window in milliseconds.
|
* @param ms - the wait window in milliseconds.
|
||||||
* @returns `true` if the child exits within `ms`, `false` on timeout.
|
* @returns `true` if the child exits within `ms` (immediately if it is
|
||||||
|
* already gone), `false` on timeout.
|
||||||
*/
|
*/
|
||||||
export function exitsWithin(child: ChildProcess, ms: number): Promise<boolean> {
|
export function exitsWithin(child: ChildProcess, ms: number): Promise<boolean> {
|
||||||
return Promise.race([
|
if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve(true)
|
||||||
waitForExit(child).then(() => true),
|
return new Promise<boolean>((resolve) => {
|
||||||
|
const onExit = (): void => {
|
||||||
|
clearTimeout(timer)
|
||||||
|
resolve(true)
|
||||||
|
}
|
||||||
// `.unref()` so a pending grace timer never keeps the parent's loop alive.
|
// `.unref()` so a pending grace timer never keeps the parent's loop alive.
|
||||||
new Promise<boolean>(resolve => setTimeout(() => { resolve(false) }, ms).unref()),
|
const timer = setTimeout(() => {
|
||||||
])
|
child.removeListener('exit', onExit)
|
||||||
|
resolve(false)
|
||||||
|
}, ms).unref()
|
||||||
|
child.once('exit', onExit)
|
||||||
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -181,15 +181,27 @@ describe('waitForExit / exitsWithin', () => {
|
|||||||
expect(fake.signalCode).toBe('SIGTERM')
|
expect(fake.signalCode).toBe('SIGTERM')
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('exitsWithin resolves true immediately for an already-exited child (no listener attached)', async () => {
|
||||||
|
const fake = new FakeChild()
|
||||||
|
fake.exitCode = 0
|
||||||
|
await expect(exitsWithin(asChild(fake), 1000)).resolves.toBe(true)
|
||||||
|
expect(fake.listenerCount('exit')).toBe(0)
|
||||||
|
})
|
||||||
|
|
||||||
it('exitsWithin resolves true when the child exits inside the window', async () => {
|
it('exitsWithin resolves true when the child exits inside the window', async () => {
|
||||||
const fake = new FakeChild({ diesOn: 'SIGTERM', delayMs: 5 })
|
const fake = new FakeChild({ diesOn: 'SIGTERM', delayMs: 5 })
|
||||||
fake.kill('SIGTERM')
|
fake.kill('SIGTERM')
|
||||||
await expect(exitsWithin(asChild(fake), 1000)).resolves.toBe(true)
|
await expect(exitsWithin(asChild(fake), 1000)).resolves.toBe(true)
|
||||||
|
// The once-listener fired and the grace timer was cleared — nothing lingers.
|
||||||
|
expect(fake.listenerCount('exit')).toBe(0)
|
||||||
})
|
})
|
||||||
|
|
||||||
it('exitsWithin resolves false on timeout for a child that never exits', async () => {
|
it('exitsWithin resolves false on timeout for a child that never exits', async () => {
|
||||||
const fake = new FakeChild() // nothing short of SIGKILL fells it; no signal sent
|
const fake = new FakeChild() // nothing short of SIGKILL fells it; no signal sent
|
||||||
await expect(exitsWithin(asChild(fake), 20)).resolves.toBe(false)
|
await expect(exitsWithin(asChild(fake), 20)).resolves.toBe(false)
|
||||||
|
// The timeout arm removed its exit listener: repeated waits (a poll loop,
|
||||||
|
// the ladder's tiers) never accumulate listeners on the same child.
|
||||||
|
expect(fake.listenerCount('exit')).toBe(0)
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user