diff --git a/.changeset/fix-effect-cleanup-detach.md b/.changeset/fix-effect-cleanup-detach.md new file mode 100644 index 000000000..e4733dc98 --- /dev/null +++ b/.changeset/fix-effect-cleanup-detach.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +Fix a throwing effect cleanup permanently breaking its effect (#2813). The previous cleanup is detached before it runs, so a cleanup that throws is never re-invoked on later runs and the effect recovers on the next update. Cleanup errors now take the standard effect error path: error boundaries can catch them, and the dev-mode strict-read guard is restored instead of leaking `STRICT_READ_UNTRACKED` false positives app-wide. diff --git a/.changeset/fix-scheduler-throwing-effect-stall.md b/.changeset/fix-scheduler-throwing-effect-stall.md new file mode 100644 index 000000000..59d1e4b05 --- /dev/null +++ b/.changeset/fix-scheduler-throwing-effect-stall.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +Fix one throwing effect killing all reactivity (#2761, #2762). An unhandled effect error no longer unwinds the flush: sibling effects queued behind it still run, and a write during the failed flush can no longer permanently stall scheduling. Errors are collected and rethrown from `flush()` once the drain completes; when several effects throw in one drain, the first is rethrown and the rest are reported via `console.error`. diff --git a/packages/solid-signals/src/core/effect.ts b/packages/solid-signals/src/core/effect.ts index b791f86d3..07fe8034f 100644 --- a/packages/solid-signals/src/core/effect.ts +++ b/packages/solid-signals/src/core/effect.ts @@ -22,6 +22,7 @@ import { cleanup } from "./owner.js"; import { _hitUnhandledAsync, GlobalQueue, + reportUncaughtError, resetUnhandledAsync, setTrackedQueueCallback } from "./scheduler.js"; @@ -89,15 +90,16 @@ function notifyEffectStatus(this: Effect, status?: number, error?: any): vo try { return this._errorFn ? this._errorFn(err, () => { - this._cleanup?.(); + const prevCleanup = this._cleanup; this._cleanup = undefined; + prevCleanup?.(); }) : console.error(err); } catch (e) { err = e; } } - if (!this._queue.notify(this, STATUS_ERROR, STATUS_ERROR)) throw err; + if (!this._queue.notify(this, STATUS_ERROR, STATUS_ERROR)) reportUncaughtError(err); } else if (this._type === EFFECT_RENDER) { this._queue.notify(this, STATUS_PENDING | STATUS_ERROR, actualStatus, actualError); if (__DEV__ && _hitUnhandledAsync) { @@ -125,9 +127,11 @@ function runEffect(node: Effect): void { if (__DEV__) { prevStrictRead = setStrictRead("an effect callback"); } - node._cleanup?.(); + // Detach first so a throwing cleanup cannot wedge future runs. + const prevCleanup = node._cleanup; node._cleanup = undefined; try { + prevCleanup?.(); const nextCleanup = node._effectFn(node._value, node._prevValue); if (__DEV__ && nextCleanup !== undefined && typeof nextCleanup !== "function") { throw new Error( @@ -142,7 +146,7 @@ function runEffect(node: Effect): void { } catch (error) { node._error = new StatusError(node, error); node._statusFlags |= STATUS_ERROR; - if (!node._queue.notify(node, STATUS_ERROR, STATUS_ERROR)) throw error; + if (!node._queue.notify(node, STATUS_ERROR, STATUS_ERROR)) reportUncaughtError(error); } finally { if (__DEV__) setStrictRead(prevStrictRead); node._prevValue = node._value; @@ -178,8 +182,10 @@ export function trackedEffect(fn: () => void | (() => void), options?: NodeOptio const node = computed( () => { - node._cleanup?.(); + // Detach first so a throwing cleanup cannot wedge future runs. + const prevCleanup = node._cleanup; node._cleanup = undefined; + prevCleanup?.(); const cleanup = staleValues(fn); if (__DEV__ && cleanup !== undefined && typeof cleanup !== "function") { throw new Error( @@ -200,7 +206,7 @@ export function trackedEffect(fn: () => void | (() => void), options?: NodeOptio if (actualStatus & STATUS_ERROR) { node._queue.notify(node, STATUS_PENDING, 0); const err = error !== undefined ? error : node._error; - if (!node._queue.notify(node, STATUS_ERROR, STATUS_ERROR)) throw err; + if (!node._queue.notify(node, STATUS_ERROR, STATUS_ERROR)) reportUncaughtError(err); } }; node._run = run; diff --git a/packages/solid-signals/src/core/scheduler.ts b/packages/solid-signals/src/core/scheduler.ts index f3a0deb7a..ea0dcf020 100644 --- a/packages/solid-signals/src/core/scheduler.ts +++ b/packages/solid-signals/src/core/scheduler.ts @@ -46,10 +46,16 @@ export const zombieQueue: Heap = { export let clock = 0; export let activeTransition: Transition | null = null; let scheduled = false; +// `scheduled` means dirty work exists; `drainArmed` means a microtask exists +// to drain it. Keeping them separate lets a failed drain recover on next write. +let drainArmed = false; let syncDepth = 0; export let projectionWriteActive = false; let inTrackedQueueCallback = false; +// Unhandled errors collected during a drain and rethrown after queues finish. +let flushErrors: unknown[] | null = null; + let _enforceLoadingBoundary = false; export let _hitUnhandledAsync = false; // When a background transition is stashed, plain optimistic signals need one @@ -218,10 +224,31 @@ function cleanupCompletedLanes(completingTransition: Transition | null): void { } } +function drainMicrotask(): void { + drainArmed = false; + flush(); +} + export function schedule() { - if (scheduled) return; scheduled = true; - if (!syncDepth && !globalQueue._running && !projectionWriteActive) queueMicrotask(flush); + if (!drainArmed && !syncDepth && !globalQueue._running && !projectionWriteActive) { + drainArmed = true; + queueMicrotask(drainMicrotask); + } +} + +/** + * Surface an error that no queue (error boundary) handled. During a drain, + * collect it so queued siblings still run (#2762); outside a drain, throw in + * place to preserve creation-time semantics. + */ +export function reportUncaughtError(error: unknown): void { + if (globalQueue._running) { + if (flushErrors === null) flushErrors = []; + flushErrors.push(error); + } else { + throw error; + } } export interface IQueue { @@ -384,6 +411,15 @@ export class GlobalQueue extends Queue { activeLanes.size && runLaneEffects(EFFECT_USER); this.run(EFFECT_USER); if (__DEV__) DEV.hooks.onUpdate?.(); + } catch (error) { + // User errors are funneled through `reportUncaughtError`; if an internal + // failure still unwinds the drain, conservatively re-arm pending work. + scheduled = true; + if (!drainArmed) { + drainArmed = true; + queueMicrotask(drainMicrotask); + } + throw error; } finally { this._running = false; } @@ -682,10 +718,25 @@ export function flush(fn?: () => T): T | void { if (__DEV__ && ++count === 1e5) throw new Error("Potential Infinite Loop Detected."); globalQueue.flush(); } + if (flushErrors !== null) { + const errors = flushErrors; + flushErrors = null; + // Only one error can be thrown; the rest must still surface, not vanish. + for (let i = 1; i < errors.length; i++) console.error(errors[i]); + throw errors[0]; + } } function runQueue(queue: QueueCallback[], type: number): void { - for (let i = 0; i < queue.length; i++) queue[i](type); + for (let i = 0; i < queue.length; i++) { + try { + queue[i](type); + } catch (error) { + // The queue array is already detached, so skipped siblings would be lost. + if (flushErrors === null) flushErrors = []; + flushErrors.push(error); + } + } } function reporterBlocksSource(reporter: Computed, source: Computed): boolean { diff --git a/packages/solid-signals/tests/createErrorBoundary.test.ts b/packages/solid-signals/tests/createErrorBoundary.test.ts index 32deb4b48..586fe1071 100644 --- a/packages/solid-signals/tests/createErrorBoundary.test.ts +++ b/packages/solid-signals/tests/createErrorBoundary.test.ts @@ -360,6 +360,45 @@ it("should catch errors thrown in user effect callbacks (back half)", () => { expect(result).toBe("errored"); }); +it("should catch errors thrown in user effect cleanups", () => { + const error = new Error("user effect cleanup error"); + const handler = vi.fn(); + const [$x, setX] = createSignal(0); + let result: any; + + createRoot(() => { + const b = createErrorBoundary( + () => { + createEffect( + () => $x(), + v => { + return () => { + if (v === 0) throw error; + }; + } + ); + return "content"; + }, + err => { + handler(err()); + return "errored"; + } + ); + createRenderEffect( + () => (result = b()), + () => {} + ); + }); + flush(); + expect(result).toBe("content"); + + setX(1); + flush(); + expect(handler).toHaveBeenCalledTimes(1); + expect(handler).toHaveBeenCalledWith(error); + expect(result).toBe("errored"); +}); + it("should catch errors thrown in user effect callbacks with error handler (back half)", () => { const error = new Error("user effect bundle error"); const handler = vi.fn(); diff --git a/packages/solid-signals/tests/createTrackedEffect.test.ts b/packages/solid-signals/tests/createTrackedEffect.test.ts index a8e238373..ef0ea46d4 100644 --- a/packages/solid-signals/tests/createTrackedEffect.test.ts +++ b/packages/solid-signals/tests/createTrackedEffect.test.ts @@ -252,6 +252,34 @@ it("should throw uncaught tracked effect errors during flush", () => { expect(() => flush()).toThrow("tracked boom"); }); +it("should not wedge on a throwing cleanup", () => { + const [$x, setX] = createSignal(0); + const ran: number[] = []; + const cleaned: number[] = []; + + createRoot(() => { + createTrackedEffect(() => { + const v = $x(); + ran.push(v); + return () => { + cleaned.push(v); + if (v === 0) throw new Error("cleanup boom"); + }; + }); + }); + flush(); + + setX(1); + expect(() => flush()).toThrow("cleanup boom"); + expect(ran).toEqual([0]); + expect(cleaned).toEqual([0]); + + setX(2); + flush(); + expect(ran).toEqual([0, 2]); + expect(cleaned).toEqual([0]); +}); + it("should throw on invalid cleanup values", () => { createRoot(() => { createTrackedEffect(() => ({}) as any); diff --git a/packages/solid-signals/tests/flush.test.ts b/packages/solid-signals/tests/flush.test.ts index 3e9925e3e..bc3ef94b9 100644 --- a/packages/solid-signals/tests/flush.test.ts +++ b/packages/solid-signals/tests/flush.test.ts @@ -1,4 +1,5 @@ -import { createEffect, createRoot, createSignal, flush } from "../src/index.js"; +import { createEffect, createRenderEffect, createRoot, createSignal, flush } from "../src/index.js"; +import { globalQueue } from "../src/core/scheduler.js"; afterEach(() => flush()); @@ -33,6 +34,218 @@ describe("scheduler hygiene around throwing effects", () => { await Promise.resolve(); expect(seenC).toEqual([3]); }); + + it("recovers scheduling when an effect writes a signal and then throws (#2761)", async () => { + const [a, setA] = createSignal(0); + const [b, setB] = createSignal(0); + const [, setC] = createSignal(0); + const seenB: number[] = []; + + createRoot(() => { + createEffect(a, v => { + if (v === 1) { + setC(100); + throw new Error("boom"); + } + }); + createEffect(b, v => { + seenB.push(v); + }); + }); + flush(); + await Promise.resolve(); + seenB.length = 0; + + expect(() => flush(() => setA(1))).toThrow("boom"); + + setB(1); + await Promise.resolve(); + await Promise.resolve(); + expect(seenB).toEqual([1]); + }); + + it("runs sibling effects queued after a throwing effect (#2762)", () => { + const [n, setN] = createSignal(0); + const seen: number[] = []; + + createRoot(() => { + createEffect(n, v => { + if (v === 1) throw new Error("boom"); + }); + createEffect(n, v => { + seen.push(v); + }); + }); + flush(); + seen.length = 0; + + setN(1); + expect(() => flush()).toThrow("boom"); + expect(seen).toEqual([1]); + + setN(2); + flush(); + expect(seen).toEqual([1, 2]); + }); + + it("throws the first error and reports the rest when multiple effects throw", () => { + const [n, setN] = createSignal(0); + const seen: number[] = []; + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); + + try { + createRoot(() => { + createEffect(n, v => { + if (v === 1) throw new Error("boom1"); + }); + createEffect(n, v => { + if (v === 1) throw new Error("boom2"); + }); + createEffect(n, v => { + seen.push(v); + }); + }); + flush(); + seen.length = 0; + + setN(1); + expect(() => flush()).toThrow("boom1"); + expect(seen).toEqual([1]); + expect(consoleError).toHaveBeenCalledWith(expect.objectContaining({ message: "boom2" })); + } finally { + consoleError.mockRestore(); + } + }); + + it("still runs user effects when a render effect throws", () => { + const [n, setN] = createSignal(0); + const seen: number[] = []; + + createRoot(() => { + createRenderEffect(n, v => { + if (v === 1) throw new Error("render boom"); + }); + createEffect(n, v => { + seen.push(v); + }); + }); + flush(); + seen.length = 0; + + setN(1); + expect(() => flush()).toThrow("render boom"); + expect(seen).toEqual([1]); + }); + + it("completes the drain when a render effect's compute throws during the heap phase", async () => { + const [n, setN] = createSignal(0); + const [b, setB] = createSignal(0); + const seen: number[] = []; + + createRoot(() => { + createRenderEffect( + () => { + if (n() === 1) throw new Error("compute boom"); + }, + () => {} + ); + createEffect(b, v => { + seen.push(v); + }); + }); + flush(); + await Promise.resolve(); + seen.length = 0; + + expect(() => + flush(() => { + setN(1); + setB(1); + }) + ).toThrow("compute boom"); + expect(seen).toEqual([1]); + + setB(2); + await Promise.resolve(); + await Promise.resolve(); + expect(seen).toEqual([1, 2]); + }); + + it("does not wedge an effect whose cleanup throws", () => { + const [n, setN] = createSignal(0); + const ran: number[] = []; + const cleaned: number[] = []; + + createRoot(() => { + createEffect(n, v => { + ran.push(v); + return () => { + cleaned.push(v); + if (v === 0) throw new Error("cleanup boom"); + }; + }); + }); + flush(); + + setN(1); + expect(() => flush()).toThrow("cleanup boom"); + expect(ran).toEqual([0]); + expect(cleaned).toEqual([0]); + + setN(2); + flush(); + expect(ran).toEqual([0, 2]); + expect(cleaned).toEqual([0]); + }); + + it("self-heals when the drain itself aborts", async () => { + const [b, setB] = createSignal(0); + const seen: number[] = []; + + createRoot(() => { + createEffect(b, v => { + seen.push(v); + }); + }); + flush(); + await Promise.resolve(); + seen.length = 0; + + let explode = true; + const badChild = { + _parent: null, + _children: [], + created: 0, + enqueue() {}, + run() { + if (explode) { + explode = false; + throw new Error("internal abort"); + } + }, + addChild() {}, + removeChild() {}, + notify: () => false, + stashQueues() {}, + restoreQueues() {} + }; + globalQueue.addChild(badChild as any); + + try { + expect(() => flush(() => setB(1))).toThrow("internal abort"); + + await Promise.resolve(); + await Promise.resolve(); + expect(seen).toEqual([1]); + + setB(2); + await Promise.resolve(); + await Promise.resolve(); + expect(seen).toEqual([1, 2]); + } finally { + globalQueue.removeChild(badChild as any); + } + }); }); it("should batch updates", () => { diff --git a/packages/solid-signals/tests/onSettled.test.ts b/packages/solid-signals/tests/onSettled.test.ts index ea67d5047..3658830d4 100644 --- a/packages/solid-signals/tests/onSettled.test.ts +++ b/packages/solid-signals/tests/onSettled.test.ts @@ -97,9 +97,9 @@ it("should throw when owner-backed onSettled calls flush reentrantly", () => { expect(() => flush()).toThrow( "Cannot call flush() from inside onSettled or createTrackedEffect. flush() is not reentrant there." ); - expect(log).toEqual(["outer"]); + expect(log).toEqual(["outer", "inner"]); expect(values[0]).toBe(0); - expect(values.at(-1)).toBe(0); + expect(values.at(-1)).toBe(5); }); it("should forbid onCleanup inside owner-backed onSettled", () => { diff --git a/packages/solid-signals/tests/strictRead.test.ts b/packages/solid-signals/tests/strictRead.test.ts index a51d93e80..be90c562d 100644 --- a/packages/solid-signals/tests/strictRead.test.ts +++ b/packages/solid-signals/tests/strictRead.test.ts @@ -36,6 +36,31 @@ describe("strictRead", () => { warn.mockRestore(); }); + it("does not leak the strict-read flag when a cleanup throws", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const [$x, setX] = createSignal(0); + const [$y] = createSignal(2); + + createRoot(() => { + createEffect( + () => $x(), + v => { + return () => { + if (v === 0) throw new Error("cleanup boom"); + }; + } + ); + }); + flush(); + + setX(1); + expect(() => flush()).toThrow("cleanup boom"); + + $y(); + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + it("warns on store property read in effect callback", () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const [$x] = createSignal(1);