diff --git a/.changeset/brave-heaps-refuse.md b/.changeset/brave-heaps-refuse.md new file mode 100644 index 000000000..6c488d86a --- /dev/null +++ b/.changeset/brave-heaps-refuse.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +fix scheduler livelock after disposing a subtree with a stale height-adjust heap entry diff --git a/packages/solid-signals/src/core/owner.ts b/packages/solid-signals/src/core/owner.ts index 68e620eba..7d3cf7737 100644 --- a/packages/solid-signals/src/core/owner.ts +++ b/packages/solid-signals/src/core/owner.ts @@ -4,6 +4,7 @@ import { defaultContext, REACTIVE_DISPOSED, REACTIVE_IN_HEAP, + REACTIVE_IN_HEAP_HEIGHT, REACTIVE_ZOMBIE } from "./constants.js"; import { @@ -16,7 +17,7 @@ import { } from "./core.js"; import { clearSignals, DEV, emitDiagnostic } from "./dev.js"; import { unlinkSubs } from "./graph.js"; -import { deleteFromHeap, insertIntoHeap } from "./heap.js"; +import { deleteFromHeap, insertIntoHeap, insertIntoHeapHeight } from "./heap.js"; import { dirtyQueue, globalQueue, zombieQueue } from "./scheduler.js"; import type { Computed, Disposable, Owner, Root } from "./types.js"; @@ -25,10 +26,20 @@ const PENDING_OWNER = {} as Owner; // Dummy owner to trigger store's read() path export function markDisposal(el: Owner): void { let child = el._firstChild; while (child) { - (child as Computed)._flags |= REACTIVE_ZOMBIE; - if ((child as Computed)._flags & REACTIVE_IN_HEAP) { - deleteFromHeap(child as Computed, dirtyQueue); - insertIntoHeap(child as Computed, zombieQueue); + const flags = (child as Computed)._flags; + (child as Computed)._flags = flags | REACTIVE_ZOMBIE; + // migrate height-adjust entries too, not just recompute entries: every + // `deleteFromHeap` call site picks the queue from the zombie flag, so a + // node left physically linked in `dirtyQueue` after being zombified gets + // unlinked from the wrong queue on dispose, corrupting the bucket and + // livelocking the next `runHeap` that reaches it (#2759) + if (flags & (REACTIVE_IN_HEAP | REACTIVE_IN_HEAP_HEIGHT)) { + deleteFromHeap( + child as Computed, + flags & REACTIVE_ZOMBIE ? zombieQueue : dirtyQueue + ); + if (flags & REACTIVE_IN_HEAP) insertIntoHeap(child as Computed, zombieQueue); + else insertIntoHeapHeight(child as Computed, zombieQueue); } markDisposal(child); child = child._nextSibling; diff --git a/packages/solid-signals/tests/scheduler-livelock.test.ts b/packages/solid-signals/tests/scheduler-livelock.test.ts new file mode 100644 index 000000000..7dd3be784 --- /dev/null +++ b/packages/solid-signals/tests/scheduler-livelock.test.ts @@ -0,0 +1,123 @@ +import { expect, it } from "vitest"; +import { createMemo, createRenderEffect, createRoot, createSignal, flush } from "../src/index.js"; +import { REACTIVE_IN_HEAP, REACTIVE_IN_HEAP_HEIGHT } from "../src/core/constants.js"; +import { dirtyQueue, zombieQueue } from "../src/core/scheduler.js"; +import type { Computed } from "../src/core/types.js"; + +/** A chain of pass-through memos, standing in for a deep derived-data pipeline. */ +function chain(src: () => T, n: number): () => T { + let cur = src; + for (let i = 0; i < n; i++) { + const prev = cur; + cur = createMemo(() => prev()); + } + return cur; +} + +/** + * Every node physically linked in a heap must carry an in-heap flag, because + * `runHeap` makes no progress on a bucket head that `deleteFromHeap` refuses + * to unlink (it early-returns on the flag check), and every `deleteFromHeap` + * call site picks the queue from the node's flags. + */ +function entriesWithoutInHeapFlags(heap: typeof dirtyQueue): number[] { + const corrupted: number[] = []; + for (let height = 0; height < heap._heap.length; height++) { + for ( + let el: Computed | undefined = heap._heap[height]; + el !== undefined; + el = el._nextHeap + ) { + if (!(el._flags & (REACTIVE_IN_HEAP | REACTIVE_IN_HEAP_HEIGHT))) { + corrupted.push(el._flags); + } + } + } + return corrupted; +} + +/** + * @see https://github.com/solidjs/solid/issues/2759 + */ +it("disposing a subtree with a stale height-adjust entry does not corrupt the dirty queue", () => { + const [open, setOpen] = createSignal(true); + let setRows!: (v: number[] | null) => void; + let view: () => unknown = () => null; + let dispose!: () => void; + + createRoot(d => { + dispose = d; + // keeps the unmount recompute at a low height, so the flush that disposes + // the subtree never advances the cursor up to the stale entry's bucket + const gate = chain(() => open(), 4); + const appView = createMemo(() => { + if (!gate()) return null; + + // ~: an async data source that lands after mount + const [rows, set] = createSignal(null); + setRows = set; + + // parks the "condition value" memo at a height above everything the + // unmount flush will visit + const tall = chain( + createMemo(() => 1), + 12 + ); + const deepA = chain( + createMemo(() => 1), + 20 + ); + const deepB = chain(() => rows(), 45); + + // mirrors : the compiler wraps the `when` expression + // in a memo that is created lazily inside (and owned by) Show's + // "condition value" memo, so the parent's height never tracks the + // child's height + let expr: (() => unknown) | undefined; + const conditionValue = createMemo(() => { + tall(); + expr ??= createMemo(() => (rows() === null ? deepA() : deepB()) && true); + return expr(); + }); + const condition = createMemo(() => conditionValue(), { + equals: (a, b) => !a === !b + }); + const value = createMemo(() => (condition() ? "inner ready" : null)); + return value(); + }); + view = appView; + createRenderEffect(appView, () => undefined); + }); + + flush(); + expect(view()).toBe("inner ready"); + + // The async data lands: the condition expression memo recomputes, its + // height grows past the already-advanced cursor while its value stays the + // same, so its subscriber is re-inserted for height adjustment at a stale + // height the cursor has already passed and survives the flush linked in + // `dirtyQueue` with only `REACTIVE_IN_HEAP_HEIGHT` set. + setRows([1, 2, 3]); + flush(); + + // Unmount: `markDisposal` zombifies the subtree and the pending disposal + // commits. The height-only entry must migrate queues together with its + // zombie flag, otherwise the commit deletes it from the queue its flag + // names (`zombieQueue`) while it is physically linked in `dirtyQueue`. + setOpen(false); + flush(); + + expect(entriesWithoutInHeapFlags(dirtyQueue)).toEqual([]); + expect(entriesWithoutInHeapFlags(zombieQueue)).toEqual([]); + + // Remount: on corrupted state this flush livelocks in `runHeap`, pinning + // the thread forever. + setOpen(true); + flush(); + setRows([1, 2, 3]); + flush(); + expect(view()).toBe("inner ready"); + + dispose(); + flush(); +});