fix(runtime): always unmount the tree in renderToString so a paint throw can't leak listeners (#186)
renderToString mounts the Vue tree, then lays out and paints, then unmounts. The app.unmount() sat inside the try AFTER paint, so when layout/paint threw (e.g. a <Transform> whose transformer throws during the paint phase) control jumped to the outer finally, which only freed yoga — Vue never tore down, so onScopeDispose never ran. Any composable that registered an external listener then leaked it: useWindowSize attaches a `resize` listener to the shared process.stdout (the no-op AppContext's stdout) and only removes it via onScopeDispose, so each failed renderToString leaked one listener, accumulating toward Node's MaxListenersExceededWarning. Fix: track that mount succeeded and, in the outer finally, run app.unmount() when `mounted && !teardownSucceeded` (best-effort, in try/catch, before the yoga free). The happy path is unaffected (it already unmounted; teardownSucceeded short-circuits the fallback). The error-path unmount frees child yoga nodes and runs onScopeDispose cleanups; freeRecursive then frees the root. The original paint error still propagates (the fallback teardown can't mask it). useWindowSize is intentionally unchanged — the unmount-in-finally is the general fix and also covers any other external listener a tree registers. Test (sequential — asserts on the process-global process.stdout resize listener count): three renderToString calls whose paint throws leak zero `resize` listeners after the fix (3 before). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,13 +1,19 @@
|
||||
// Sequential: mutates process-global state — process.env.COLUMNS/LINES and
|
||||
// process.stdout/process.stderr columns+rows. The terminal-size package (used by
|
||||
// resolveSize's fallback) reads these globals directly, so a concurrent sibling
|
||||
// would perturb the result. Tests restore every mutated prop in a finally block.
|
||||
// Sequential: mutates / asserts on process-global state —
|
||||
// • process.env.COLUMNS/LINES and process.stdout/process.stderr columns+rows
|
||||
// (the terminal-size package, used by resolveSize's fallback, reads these
|
||||
// globals directly, so a concurrent sibling would perturb the result), and
|
||||
// • process.stdout.listenerCount("resize") — the renderToString teardown-leak
|
||||
// test below mounts useWindowSize against the SHARED process.stdout (the
|
||||
// no-op AppContext's stdout) and asserts the "resize" listener count returns
|
||||
// to baseline; a concurrent sibling that also touches process.stdout would
|
||||
// make that count flaky.
|
||||
// Tests restore every mutated prop in a finally block.
|
||||
|
||||
import { PassThrough } from "node:stream";
|
||||
import process from "node:process";
|
||||
import { defineComponent } from "vue";
|
||||
import { expect, test } from "vite-plus/test";
|
||||
import { createApp, Text, useWindowSize } from "@vue-tui/runtime";
|
||||
import { createApp, renderToString, Text, Transform, useWindowSize } from "@vue-tui/runtime";
|
||||
|
||||
function makeTtyStream(columns: number): NodeJS.WriteStream {
|
||||
const s = new PassThrough() as unknown as NodeJS.WriteStream;
|
||||
@@ -84,3 +90,67 @@ test.sequential("useWindowSize falls back to terminal-size rows from env.LINES w
|
||||
process.stderr.rows = originalStderrRows;
|
||||
}
|
||||
});
|
||||
|
||||
// renderToString's no-op AppContext uses the REAL shared process.stdout (so
|
||||
// useWindowSize attaches its `resize` listener there). If layout/paint throws,
|
||||
// the happy-path app.unmount() is skipped — without an unmount in the outer
|
||||
// finally, onScopeDispose never runs and the `resize` listener leaks ONE per
|
||||
// failed call, accumulating toward Node's MaxListenersExceededWarning. A
|
||||
// throwing <Transform> transform runs during the PAINT phase, so it reproduces
|
||||
// the layout/paint-phase throw exactly. (Asserts on the shared
|
||||
// process.stdout listener count — hence this sequential file.)
|
||||
test.sequential("renderToString does not leak useWindowSize's resize listener when paint throws", () => {
|
||||
const Leaky = defineComponent(() => {
|
||||
// Registers a `resize` listener on ctx.stdout (process.stdout) via
|
||||
// onScopeDispose; only an unmount tears it down.
|
||||
useWindowSize();
|
||||
return () => (
|
||||
// The transform runs during paint, so it throws AFTER app.mount() succeeded.
|
||||
<Transform
|
||||
transform={() => {
|
||||
throw new Error("paint boom");
|
||||
}}
|
||||
>
|
||||
<Text>boom</Text>
|
||||
</Transform>
|
||||
);
|
||||
});
|
||||
|
||||
const before = process.stdout.listenerCount("resize");
|
||||
|
||||
let threwCount = 0;
|
||||
for (let i = 0; i < 3; i++) {
|
||||
try {
|
||||
renderToString(Leaky);
|
||||
} catch {
|
||||
// renderToString rethrows the paint error after cleanup — expected.
|
||||
threwCount++;
|
||||
}
|
||||
}
|
||||
|
||||
const after = process.stdout.listenerCount("resize");
|
||||
|
||||
// The error path must actually fire (otherwise we'd be testing the happy path).
|
||||
expect(threwCount).toBe(3);
|
||||
// No net listeners leaked across the three failed calls.
|
||||
expect(after).toBe(before);
|
||||
});
|
||||
|
||||
// Control: the CLEAN (non-throwing) renderToString path already unmounts and
|
||||
// runs onScopeDispose, so it leaks zero resize listeners. Proves the harness is
|
||||
// sound — the assertion above is meaningful only because this one passes too.
|
||||
test.sequential("renderToString clean path leaks no useWindowSize resize listener", () => {
|
||||
const Clean = defineComponent(() => {
|
||||
useWindowSize();
|
||||
return () => <Text>clean</Text>;
|
||||
});
|
||||
|
||||
const before = process.stdout.listenerCount("resize");
|
||||
for (let i = 0; i < 3; i++) {
|
||||
const output = renderToString(Clean);
|
||||
expect(output).toBe("clean");
|
||||
}
|
||||
const after = process.stdout.listenerCount("resize");
|
||||
|
||||
expect(after).toBe(before);
|
||||
});
|
||||
|
||||
@@ -144,10 +144,12 @@ function renderToStringInternal(
|
||||
};
|
||||
|
||||
let teardownSucceeded = false;
|
||||
let mounted = false;
|
||||
|
||||
try {
|
||||
// Synchronously render the Vue tree into the root.
|
||||
app.mount(root);
|
||||
mounted = true;
|
||||
|
||||
const restoreLayoutGuards = calculateLayoutWithContentGuards(
|
||||
root,
|
||||
@@ -198,12 +200,28 @@ function renderToStringInternal(
|
||||
|
||||
return normalizedStaticOutput || output;
|
||||
} finally {
|
||||
// If layout/paint threw, the happy-path app.unmount() above was skipped. Unmount
|
||||
// here so the tree's onScopeDispose cleanups ALWAYS run — otherwise a composable
|
||||
// that registered an external listener (e.g. useWindowSize's `resize` listener on
|
||||
// the shared process.stdout) leaks one per failed call, accumulating toward Node's
|
||||
// MaxListenersExceededWarning. Guard with `mounted` (never unmount a tree that
|
||||
// never mounted) and `teardownSucceeded` (the happy path already unmounted — no
|
||||
// double-unmount). app.unmount() also frees the CHILD yoga nodes via the node-ops
|
||||
// remove handler, leaving only the root for freeRecursive below.
|
||||
if (mounted && !teardownSucceeded) {
|
||||
try {
|
||||
app.unmount();
|
||||
} catch {
|
||||
// Best-effort teardown: a throw here must not mask the original error.
|
||||
}
|
||||
}
|
||||
|
||||
// Ensure native yoga memory is freed even if rendering or teardown threw.
|
||||
// Yoga nodes are WASM-backed and not garbage collected.
|
||||
// Yoga nodes are WASM-backed and not garbage collected. In the happy path
|
||||
// detachYoga(root) already freed the root; here (error path) freeRecursive
|
||||
// cleans up the root and any child nodes the unmount above couldn't free.
|
||||
if (!teardownSucceeded) {
|
||||
try {
|
||||
// If unmount failed, some child nodes may not have been freed.
|
||||
// Use freeRecursive to clean up the entire tree as best-effort.
|
||||
root.yoga.freeRecursive();
|
||||
} catch {
|
||||
// Best-effort: node may already be partially freed
|
||||
|
||||
Reference in New Issue
Block a user