diff --git a/.changeset/async-protocol-throw-4112.md b/.changeset/async-protocol-throw-4112.md new file mode 100644 index 000000000..1770da7ad --- /dev/null +++ b/.changeset/async-protocol-throw-4112.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +Route synchronous throws from the async iterator / thenable protocol (a later `next()`, a step's `then`, or deferred iterator setup) to the owning memo's error handling instead of escaping as unhandled rejections, and make `StatusError` message formatting best-effort so it never replaces the original error (#4112). diff --git a/packages/signals/src/core/async.ts b/packages/signals/src/core/async.ts index 813ad5f88..54f0c68a7 100644 --- a/packages/signals/src/core/async.ts +++ b/packages/signals/src/core/async.ts @@ -41,7 +41,7 @@ import { freshHook, laneStageHook } from "./scheduler.js"; -import type { Computed, Link } from "./types.js"; +import type { Computed, Link, Owner } from "./types.js"; // The lazily-created Set is the ONE container for pending sources. Its // predecessor — a singular slot promoted to a Set on the second source — @@ -578,18 +578,14 @@ export function handleAsync( // close registration uses the ambient owner; and DEFERRED (the flattening // path — a thenable resolved to an iterable in a later microtask), where // there is no caller to serve and no ambient owner: sync-settled steps - // write through asyncWrite, and close registration goes through the slot - // the thenable branch pre-registered while it still owned the context. - // Returns whether a sync answer landed (first yield or empty completion) — - // meaningful only in the live posture. - const consumeIterator = ( - source: AsyncIterable, - registerClose?: (fn: () => void) => void - ): boolean => { + // write through asyncWrite, and close registration targets `owner` (el + // itself) directly. Returns whether a sync answer landed (first yield or + // empty completion) — meaningful only in the live posture. + const consumeIterator = (source: AsyncIterable, owner?: Owner): boolean => { const it = source[Symbol.asyncIterator](); let hadValue = false; let completed = false; - let initialRead = !registerClose; + let initialRead = !owner; const close = () => { if (completed) return; @@ -599,7 +595,7 @@ export function handleAsync( if (isThenable(returned)) returned.then(undefined, () => {}); } catch {} }; - registerClose ? registerClose(close) : cleanup(close); + cleanup(close, owner); // Flight-identity cancellation (#3122): the registration above is the // owner-death backstop; the teardown slot fires at the _inFlight release // sites so supersede stops this stream immediately. @@ -608,8 +604,15 @@ export function handleAsync( // Release check before each next pull: an unobserved lazy node must tear // down (its close above runs via disposal, closing the iterator) instead // of pumping the stream forever with zero subscribers (#2935). + // Later pulls run from an async continuation with no compute catch above + // them: a throw invoking the protocol (next(), the step's then) settles + // as this flight's error instead of escaping as a rejection (#4112). const iterateOrRelease = () => { - if (!settleAutodispose()) iterate(); + try { + if (!settleAutodispose()) iterate(); + } catch (e) { + handleError(e); + } }; const iterate = (): boolean => { @@ -700,7 +703,7 @@ export function handleAsync( // stream itself rather than settling on the iterable object. One level // only: A+ `then` already collapses nested thenables, so the resolved // value is never itself a thenable. - const flattenIfIterable = (value: any, registerClose?: (fn: () => void) => void): boolean => { + const flattenIfIterable = (value: any, owner?: Owner): boolean => { let innerIterator: any = false; if (typeof value === "object" && value !== null) { untrack(() => { @@ -709,8 +712,8 @@ export function handleAsync( } if (!innerIterator) return false; syncValue = undefined as T; - const landed = consumeIterator(value as AsyncIterable, registerClose); - if (!registerClose) liveLanded = landed; + const landed = consumeIterator(value as AsyncIterable, owner); + if (!owner) liveLanded = landed; return true; }; @@ -719,34 +722,32 @@ export function handleAsync( rejected = false, syncError: any, isSync = true; - // Close registration for the flattening path. Consumption starts in a - // microtask where the ambient owner is gone (or worse, someone else's), - // so `cleanup()` can't be used — the close targets el's disposal list - // directly, exactly where a live cleanup() during this recompute would - // have put it. Only a flight that actually flattens becomes - // disposal-bearing. - const registerDeferredClose = (fn: () => void) => { - if (!el._disposal) el._disposal = fn; - else if (Array.isArray(el._disposal)) el._disposal.push(fn); - else el._disposal = [el._disposal, fn]; - }; (__DEV__ ? watchAsyncTail(el, result as PromiseLike) : (result as PromiseLike)).then( v => { if (isSync) { syncValue = v; resolved = true; - } else if ( - el._x?._inFlight === result && - !(el._flags & REACTIVE_DISPOSED) && - flattenIfIterable(v, registerDeferredClose) - ) { - // Flattened: the stream is the value. Each yield lands through - // asyncWrite under this flight's identity; the first one clears - // pending exactly like a plain promise resolution would have. - // (Disposed nodes never start a pump — their disposal list has - // already run, so nothing could ever close the iterator.) } else { - asyncWrite(v); + // Flattening consumes the stream as the value: each yield lands + // through asyncWrite under this flight's identity; the first one + // clears pending exactly like a plain promise resolution would. + // Disposed nodes never start a pump (their disposal list already + // ran, so nothing could close the iterator). Consumption starts in + // a microtask where the ambient owner is gone, so the close + // registers on el's own disposal list. Iterator setup here has no + // compute catch above it: a protocol throw settles as this + // flight's error (#4112). + try { + if ( + el._x?._inFlight === result && + !(el._flags & REACTIVE_DISPOSED) && + flattenIfIterable(v, el) + ) + return; + asyncWrite(v); + } catch (e) { + handleError(e); + } settleAutodispose(); } }, diff --git a/packages/signals/src/core/error.ts b/packages/signals/src/core/error.ts index ceee5dbc7..263645f23 100644 --- a/packages/signals/src/core/error.ts +++ b/packages/signals/src/core/error.ts @@ -51,9 +51,13 @@ export class StatusError extends Error { public source: any, original: any ) { - super(original instanceof Error ? original.message : String(original), { - cause: original - }); + // Best-effort message: formatting must never replace the original (#4112). + try { + var message: string | undefined = String( + original instanceof Error ? original.message : original + ); + } catch {} + super(message, { cause: original }); } } diff --git a/packages/signals/src/core/owner.ts b/packages/signals/src/core/owner.ts index 8f4289ae3..1fec7f904 100644 --- a/packages/signals/src/core/owner.ts +++ b/packages/signals/src/core/owner.ts @@ -301,13 +301,15 @@ export function getOwner(): Owner | null { /** * Low-level: registers `fn` as a disposal callback on the current owner. * Most code should use `onCleanup()` from `solid-js`, which adds dev-mode - * checks. `cleanup()` is the unchecked primitive used by internals. + * checks. `cleanup()` is the unchecked primitive used by internals; `owner` + * (internal) targets a specific owner's disposal list instead of the ambient + * one. */ -export function cleanup(fn: Disposable): Disposable { - if (!context) return fn; - if (!context._disposal) context._disposal = fn; - else if (Array.isArray(context._disposal)) context._disposal.push(fn); - else context._disposal = [context._disposal, fn]; +export function cleanup(fn: Disposable, owner: Owner | null = context): Disposable { + if (!owner) return fn; + if (!owner._disposal) owner._disposal = fn; + else if (Array.isArray(owner._disposal)) owner._disposal.push(fn); + else owner._disposal = [owner._disposal, fn]; return fn; } diff --git a/packages/signals/tests/async-protocol-throw-4112.test.ts b/packages/signals/tests/async-protocol-throw-4112.test.ts new file mode 100644 index 000000000..df060351b --- /dev/null +++ b/packages/signals/tests/async-protocol-throw-4112.test.ts @@ -0,0 +1,165 @@ +import { + createErrorBoundary, + createMemo, + createRenderEffect, + createRoot, + flush +} from "../src/index.js"; + +// #4112: throws raised while Solid invokes the async-iterator / thenable +// protocol (not rejections delivered through it) must reach the owning memo's +// error handling, and error formatting must never replace the original error. + +afterEach(() => flush()); + +const tick = () => new Promise(r => setTimeout(r, 0)); +const original = (e: any) => e?.cause ?? e; + +describe("#4112 async protocol throws reach the memo", () => { + let unhandled: unknown[]; + const onUnhandled = (e: unknown) => void unhandled.push(e); + beforeEach(() => { + unhandled = []; + process.on("unhandledRejection", onUnhandled); + }); + afterEach(() => void process.off("unhandledRejection", onUnhandled)); + + // Yields 1 after a timer, then defers to `second` for the next pull. + function iterableWith(second: () => any): AsyncIterable { + return { + [Symbol.asyncIterator]() { + let n = 0; + return { + next() { + if (n++ === 0) + return new Promise>(r => + setTimeout(() => r({ done: false, value: 1 })) + ); + return second(); + } + } as AsyncIterator; + } + }; + } + + // Mirrors {memo()}: the boundary's fallback is where a + // protocol failure must surface. + async function run(source: () => any) { + const shown: unknown[] = []; + createRoot(() => { + const memo = createMemo(source as () => number); + const b = createErrorBoundary( + () => memo(), + err => original(err()) + ); + createRenderEffect( + () => { + try { + return b(); + } catch { + return "loading"; + } + }, + v => void shown.push(v) + ); + }); + flush(); + for (let i = 0; i < 4; i++) { + await tick(); + flush(); + } + return shown; + } + + const last = (shown: unknown[]) => (shown[shown.length - 1] as any)?.message; + + it("control: later next() rejecting reaches the boundary", async () => { + const shown = await run(() => iterableWith(() => Promise.reject(new Error("rejected")))); + expect(last(shown)).toBe("rejected"); + expect(unhandled).toEqual([]); + }); + + it("later next() throwing synchronously reaches the boundary", async () => { + const shown = await run(() => + iterableWith(() => { + throw new Error("next failed"); + }) + ); + expect(last(shown)).toBe("next failed"); + expect(unhandled).toEqual([]); + }); + + it("next step's then() throwing reaches the boundary", async () => { + const shown = await run(() => + iterableWith(() => ({ + then() { + throw new Error("then failed"); + } + })) + ); + expect(last(shown)).toBe("then failed"); + expect(unhandled).toEqual([]); + }); + + it("deferred iterator factory throwing reaches the boundary", async () => { + const shown = await run(() => + new Promise(r => setTimeout(r)).then(() => ({ + [Symbol.asyncIterator]() { + throw new Error("iterator failed"); + } + })) + ); + expect(last(shown)).toBe("iterator failed"); + expect(unhandled).toEqual([]); + }); + + it("deferred first next() throwing reaches the boundary", async () => { + const shown = await run(() => + new Promise(r => setTimeout(r)).then(() => ({ + [Symbol.asyncIterator]() { + return { + next() { + throw new Error("first next failed"); + } + }; + } + })) + ); + expect(last(shown)).toBe("first next failed"); + expect(unhandled).toEqual([]); + }); +}); + +describe("#4112 error formatting keeps the original error", () => { + function create(thrown: unknown) { + let memo!: () => unknown; + expect(() => + createRoot(() => { + memo = createMemo(() => { + throw thrown; + }); + }) + ).not.toThrow(); + let caught: unknown; + try { + memo(); + } catch (e) { + caught = e; + } + expect(original(caught)).toBe(thrown); + } + + it("unstringifiable thrown value does not escape createMemo", () => { + create(Object.create(null)); + }); + + it("throwing message getter does not escape createMemo", () => { + const err = new Error("x"); + Object.defineProperty(err, "message", { + get() { + throw new Error("getter"); + } + }); + create(err); + }); +});