Skip to content

Commit 20e4117

Browse files
Mearman43081j
andauthored
fix: destroy piped streams on child exit to prevent grandchild deadlock (tinylibs#137)
* test: verify exec completes when grandchild holds piped stdout When a child process spawns a grandchild that inherits the piped stdout fd (fd 1), the grandchild keeps the pipe open after the child exits. Without the fix, both `await exec()` and the async iterator hang indefinitely because the piped streams never end. Each test runs in a subprocess (spawnSync with a 10s timeout) so the orphaned grandchild doesn't block vitest. A 5s inner race detects whether exec completes or hangs. * fix: destroy piped streams on child exit to prevent grandchild deadlock When a child process spawns a grandchild that inherits the piped stdout/stderr file descriptors, the grandchild holds them open after the child exits. Node's close event waits for all fds to be released, so it never fires. readStream and combineStreams hang indefinitely because the streams never end. Listen for the exit event (fires when the process exits, regardless of pipe state) and destroy the piped streams. This forces the PassThrough and readline consumers to complete. Refs: lint-staged/lint-staged#1800 * test: add grandchild fixture scripts for pipe inheritance tests Static test fixtures that spawn a long-lived grandchild process inheriting the piped stdout fd, simulating tsserver inheriting eslint's piped streams through lint-staged/tinyexec. * test: restructure grandchild tests to use static fixtures Use committed fixture scripts instead of generated inline scripts. Run tests in a subprocess with isolated stdio so orphaned grandchildren don't block vitest's teardown. Clean up grandchildren via pkill with a marker comment in the fixture scripts. * fix: use setImmediate instead of setTimeout for stream cleanup setImmediate is sufficient to let buffered data drain before destroying the streams. Removes the timer tracking in _resetState and _onClose that was needed for the setTimeout approach. * chore: clarify comment * test: move tests into main * fix: account for piped streams --------- Co-authored-by: James Garbutt <43081j@users.noreply.github.com>
1 parent 51e4f27 commit 20e4117

4 files changed

Lines changed: 133 additions & 1 deletion

File tree

src/main.ts

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ export {NonZeroExitError, normalizeSpawnCommand};
1717

1818
const LINE_SEPARATOR_REGEX = /\r?\n/;
1919

20+
const pipedStreams = new WeakSet<Readable>();
21+
2022
export interface Output {
2123
stderr: string;
2224
stdout: string;
@@ -334,6 +336,7 @@ export class ExecProcess implements Result {
334336

335337
this._process = handle;
336338
handle.once('error', this._onError);
339+
handle.once('exit', this._onExit);
337340
handle.once('close', this._onClose);
338341

339342
if (handle.stdin) {
@@ -342,7 +345,11 @@ export class ExecProcess implements Result {
342345
if (typeof stdin === 'string') {
343346
handle.stdin.end(stdin);
344347
} else {
345-
stdin?.process?.stdout?.pipe(handle.stdin);
348+
const src = stdin?.process?.stdout;
349+
if (src) {
350+
src.pipe(handle.stdin);
351+
pipedStreams.add(src);
352+
}
346353
}
347354
}
348355
}
@@ -366,6 +373,28 @@ export class ExecProcess implements Result {
366373
this._thrownError = err;
367374
};
368375

376+
protected _onExit = (): void => {
377+
// Node emits 'exit' before stdio streams have drained. Use setImmediate
378+
// to let buffered data flow through before destroying the streams.
379+
// If grandchild processes hold the pipe fds open, they would never fire
380+
// 'close', so we destroy here to unblock readStream and combineStreams.
381+
const out =
382+
this._streamOut && !pipedStreams.has(this._streamOut)
383+
? this._streamOut
384+
: undefined;
385+
const err =
386+
this._streamErr && !pipedStreams.has(this._streamErr)
387+
? this._streamErr
388+
: undefined;
389+
if (!out && !err) {
390+
return;
391+
}
392+
setImmediate(() => {
393+
out?.destroy();
394+
err?.destroy();
395+
});
396+
};
397+
369398
protected _onClose = (): void => {
370399
if (this._resolveClose) {
371400
this._resolveClose();

src/test/main_test.ts

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@ import {describe, test, expect} from 'vitest';
33
import os from 'node:os';
44
import fs from 'node:fs';
55
import path from 'node:path';
6+
import {spawnSync} from 'node:child_process';
67

78
const isWindows = os.platform() === 'win32';
9+
const fixturesDir = path.join(import.meta.dirname, '../../test/fixtures');
10+
const distDir = path.join(import.meta.dirname, '../../dist');
811

912
const variants = [
1013
{name: 'async', x, isAsync: true},
@@ -363,6 +366,82 @@ if (!isWindows) {
363366
fs.rmSync(dir, {recursive: true, force: true});
364367
}
365368
});
369+
370+
test('resolves when grandchild holds piped stdout open', async () => {
371+
const dir = fs.mkdtempSync(
372+
path.join(os.tmpdir(), 'tinyexec-grandchild-')
373+
);
374+
const runnerScript = path.join(dir, 'runner.mjs');
375+
const distPath = JSON.stringify(path.join(distDir, 'main.mjs'));
376+
const fixturePath = JSON.stringify(
377+
path.join(fixturesDir, 'grandchild.mjs')
378+
);
379+
380+
fs.writeFileSync(
381+
runnerScript,
382+
`import { x } from ${distPath}
383+
const result = await x('node', [${fixturePath}])
384+
process.stdout.write(JSON.stringify({ stdout: result.stdout, exitCode: result.exitCode }))
385+
`
386+
);
387+
388+
try {
389+
const proc = spawnSync('node', [runnerScript], {
390+
timeout: 10000,
391+
encoding: 'utf8',
392+
killSignal: 'SIGKILL',
393+
stdio: ['pipe', 'pipe', 'pipe']
394+
});
395+
396+
expect(proc.signal).not.toBe('SIGKILL');
397+
expect(proc.status).toBe(0);
398+
const parsed = JSON.parse(proc.stdout.trim());
399+
expect(parsed.exitCode).toBe(0);
400+
expect(parsed.stdout).toBe('output\n');
401+
} finally {
402+
spawnSync('pkill', ['-f', 'tinyexec-test-grandchild']);
403+
fs.rmSync(dir, {recursive: true, force: true});
404+
}
405+
});
406+
407+
test('iterator completes when grandchild holds piped stdout open', async () => {
408+
const dir = fs.mkdtempSync(
409+
path.join(os.tmpdir(), 'tinyexec-grandchild-')
410+
);
411+
const runnerScript = path.join(dir, 'runner.mjs');
412+
const distPath = JSON.stringify(path.join(distDir, 'main.mjs'));
413+
const fixturePath = JSON.stringify(
414+
path.join(fixturesDir, 'grandchild_multiline.mjs')
415+
);
416+
417+
fs.writeFileSync(
418+
runnerScript,
419+
`import { x } from ${distPath}
420+
const lines = []
421+
for await (const line of x('node', [${fixturePath}])) {
422+
lines.push(line)
423+
}
424+
process.stdout.write(JSON.stringify(lines))
425+
`
426+
);
427+
428+
try {
429+
const proc = spawnSync('node', [runnerScript], {
430+
timeout: 10000,
431+
encoding: 'utf8',
432+
killSignal: 'SIGKILL',
433+
stdio: ['pipe', 'pipe', 'pipe']
434+
});
435+
436+
expect(proc.signal).not.toBe('SIGKILL');
437+
expect(proc.status).toBe(0);
438+
const parsed = JSON.parse(proc.stdout.trim());
439+
expect(parsed).toEqual(['line1', 'line2']);
440+
} finally {
441+
spawnSync('pkill', ['-f', 'tinyexec-test-grandchild']);
442+
fs.rmSync(dir, {recursive: true, force: true});
443+
}
444+
});
366445
});
367446

368447
describe('exec (unix-like) (sync)', () => {

test/fixtures/grandchild.mjs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import {spawn} from 'node:child_process';
2+
3+
// Spawn a grandchild that inherits the piped stdout fd, simulating
4+
// tsserver inheriting eslint's piped streams. Short timeout to avoid
5+
// blocking test teardown.
6+
spawn(
7+
process.argv[0],
8+
['-e', 'setTimeout(() => void 0, 3000)'],
9+
{stdio: ['ignore', 1, 'ignore']}
10+
);
11+
12+
console.log('output');
13+
process.exit(0);
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import {spawn} from 'node:child_process';
2+
3+
spawn(
4+
process.argv[0],
5+
['-e', 'setTimeout(() => void 0, 3000)'],
6+
{stdio: ['ignore', 1, 'ignore']}
7+
);
8+
9+
console.log('line1');
10+
console.log('line2');
11+
process.exit(0);

0 commit comments

Comments
 (0)