From a8c359d119d4e4f11f904a348d1b36bb44c5251a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 16:05:28 +0000 Subject: [PATCH 1/2] signals: route async protocol throws and error-formatting failures to the memo (#4112) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_015hPxMkC3jg3Uivh65gvaf2 --- .changeset/async-protocol-throw-4112.md | 5 + packages/signals/src/core/async.ts | 121 +++++++------ packages/signals/src/core/error.ts | 9 +- .../tests/async-protocol-throw-4112.test.ts | 165 ++++++++++++++++++ 4 files changed, 245 insertions(+), 55 deletions(-) create mode 100644 .changeset/async-protocol-throw-4112.md create mode 100644 packages/signals/tests/async-protocol-throw-4112.test.ts diff --git a/.changeset/async-protocol-throw-4112.md b/.changeset/async-protocol-throw-4112.md new file mode 100644 index 0000000000..1770da7ad4 --- /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 c38de4d6ec..248a9666d8 100644 --- a/packages/signals/src/core/async.ts +++ b/packages/signals/src/core/async.ts @@ -622,57 +622,66 @@ export function handleAsync( // use the bare form as a promise-free fast path when a value is already // buffered (seroval's deserialized streams do), so a bare result is // assimilated as an already-settled step instead of crashing on `.then`. - const step = it.next(); - const settled: PromiseLike> = isThenable(step) - ? step - : { then: onSettle => void onSettle!(step) as any }; - settled.then( - r => { - // The sync stash only serves the INITIAL drain (handleAsync's caller - // consumes syncValue / throws NotReady from it). A sync-settled step - // after an async gap — seroval buffering values between pulls, a - // sync-thenable producer mid-stream — has no caller reading the - // stash: it must write through the async path or the value is - // silently dropped. (The deferred posture never has a caller, so - // initialRead starts false there and everything writes through.) - if (isSync && initialRead) { - syncResult = r; - resolved = true; - if (r.done) completed = true; - } else if (el._x?._inFlight !== result) { - return; - } else if (!r.done) { - hadValue = true; - asyncWrite(r.value, iterateOrRelease); - } else { - completed = true; - if (hadValue) { - schedule(); - flush(); + // A throw while invoking the protocol (next(), the step's then) is a + // rejection of this step: after an async gap there is no compute catch + // to route it, so it settles through the same path (#4112). + try { + const step = it.next(); + const settled: PromiseLike> = isThenable(step) + ? step + : { then: onSettle => void onSettle!(step) as any }; + settled.then( + r => { + // The sync stash only serves the INITIAL drain (handleAsync's caller + // consumes syncValue / throws NotReady from it). A sync-settled step + // after an async gap — seroval buffering values between pulls, a + // sync-thenable producer mid-stream — has no caller reading the + // stash: it must write through the async path or the value is + // silently dropped. (The deferred posture never has a caller, so + // initialRead starts false there and everything writes through.) + if (isSync && initialRead) { + syncResult = r; + resolved = true; + if (r.done) completed = true; + } else if (el._x?._inFlight !== result) { + return; + } else if (!r.done) { + hadValue = true; + asyncWrite(r.value, iterateOrRelease); } else { - // Empty completion settles like the immediately-done sync path. - asyncWrite(undefined as T); + completed = true; + if (hadValue) { + schedule(); + flush(); + } else { + // Empty completion settles like the immediately-done sync path. + asyncWrite(undefined as T); + } + settleAutodispose(); + } + }, + e => { + if (isSync && initialRead) { + syncError = e; + rejected = true; + } else if (el._x?._inFlight === result) { + completed = true; + handleError(e); + settleAutodispose(); } - settleAutodispose(); - } - }, - e => { - if (isSync && initialRead) { - syncError = e; - rejected = true; - } else if (el._x?._inFlight === result) { - completed = true; - handleError(e); - settleAutodispose(); } - } - ); + ); + } catch (e) { + syncError = e; + rejected = true; + } isSync = false; if (rejected) { // Match the promise branch, but only rethrow during the initial read. completed = true; handleError(syncError); if (initialRead) throw syncError; + settleAutodispose(); return true; } if (resolved && !syncResult!.done) { @@ -734,17 +743,25 @@ export function handleAsync( 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 { + try { + // 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.) + if ( + el._x?._inFlight === result && + !(el._flags & REACTIVE_DISPOSED) && + flattenIfIterable(v, registerDeferredClose) + ) + return; + } catch (e) { + // Iterator setup threw outside any compute catch (#4112). + handleError(e); + settleAutodispose(); + return; + } asyncWrite(v); settleAutodispose(); } diff --git a/packages/signals/src/core/error.ts b/packages/signals/src/core/error.ts index ceee5dbc7e..4410e7499c 100644 --- a/packages/signals/src/core/error.ts +++ b/packages/signals/src/core/error.ts @@ -51,9 +51,12 @@ 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). + let message: string | undefined; + try { + message = String(original instanceof Error ? original.message : original); + } catch {} + super(message, { cause: original }); } } 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 0000000000..df060351b2 --- /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); + }); +}); From 0697d979d00470a7b215613d30d322599a69769d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 17:45:24 +0000 Subject: [PATCH 2/2] signals: golf #4112 fix to +3 B on the core floor Guard only the async continuation sites (iterateOrRelease and the deferred fulfillment callback) instead of the whole iterate() protocol block, and drop registerDeferredClose in favour of cleanup(fn, owner). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_015hPxMkC3jg3Uivh65gvaf2 --- packages/signals/src/core/async.ts | 158 +++++++++++++---------------- packages/signals/src/core/error.ts | 5 +- packages/signals/src/core/owner.ts | 14 +-- 3 files changed, 82 insertions(+), 95 deletions(-) diff --git a/packages/signals/src/core/async.ts b/packages/signals/src/core/async.ts index 248a9666d8..8a23d4ca96 100644 --- a/packages/signals/src/core/async.ts +++ b/packages/signals/src/core/async.ts @@ -42,7 +42,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 — @@ -577,18 +577,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; @@ -598,7 +594,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. @@ -607,8 +603,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 => { @@ -622,66 +625,57 @@ export function handleAsync( // use the bare form as a promise-free fast path when a value is already // buffered (seroval's deserialized streams do), so a bare result is // assimilated as an already-settled step instead of crashing on `.then`. - // A throw while invoking the protocol (next(), the step's then) is a - // rejection of this step: after an async gap there is no compute catch - // to route it, so it settles through the same path (#4112). - try { - const step = it.next(); - const settled: PromiseLike> = isThenable(step) - ? step - : { then: onSettle => void onSettle!(step) as any }; - settled.then( - r => { - // The sync stash only serves the INITIAL drain (handleAsync's caller - // consumes syncValue / throws NotReady from it). A sync-settled step - // after an async gap — seroval buffering values between pulls, a - // sync-thenable producer mid-stream — has no caller reading the - // stash: it must write through the async path or the value is - // silently dropped. (The deferred posture never has a caller, so - // initialRead starts false there and everything writes through.) - if (isSync && initialRead) { - syncResult = r; - resolved = true; - if (r.done) completed = true; - } else if (el._x?._inFlight !== result) { - return; - } else if (!r.done) { - hadValue = true; - asyncWrite(r.value, iterateOrRelease); + const step = it.next(); + const settled: PromiseLike> = isThenable(step) + ? step + : { then: onSettle => void onSettle!(step) as any }; + settled.then( + r => { + // The sync stash only serves the INITIAL drain (handleAsync's caller + // consumes syncValue / throws NotReady from it). A sync-settled step + // after an async gap — seroval buffering values between pulls, a + // sync-thenable producer mid-stream — has no caller reading the + // stash: it must write through the async path or the value is + // silently dropped. (The deferred posture never has a caller, so + // initialRead starts false there and everything writes through.) + if (isSync && initialRead) { + syncResult = r; + resolved = true; + if (r.done) completed = true; + } else if (el._x?._inFlight !== result) { + return; + } else if (!r.done) { + hadValue = true; + asyncWrite(r.value, iterateOrRelease); + } else { + completed = true; + if (hadValue) { + schedule(); + flush(); } else { - completed = true; - if (hadValue) { - schedule(); - flush(); - } else { - // Empty completion settles like the immediately-done sync path. - asyncWrite(undefined as T); - } - settleAutodispose(); - } - }, - e => { - if (isSync && initialRead) { - syncError = e; - rejected = true; - } else if (el._x?._inFlight === result) { - completed = true; - handleError(e); - settleAutodispose(); + // Empty completion settles like the immediately-done sync path. + asyncWrite(undefined as T); } + settleAutodispose(); } - ); - } catch (e) { - syncError = e; - rejected = true; - } + }, + e => { + if (isSync && initialRead) { + syncError = e; + rejected = true; + } else if (el._x?._inFlight === result) { + completed = true; + handleError(e); + settleAutodispose(); + } + } + ); isSync = false; if (rejected) { // Match the promise branch, but only rethrow during the initial read. completed = true; handleError(syncError); if (initialRead) throw syncError; - settleAutodispose(); return true; } if (resolved && !syncResult!.done) { @@ -708,7 +702,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(() => { @@ -717,8 +711,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; }; @@ -727,42 +721,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 { + // 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 { - // 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.) if ( el._x?._inFlight === result && !(el._flags & REACTIVE_DISPOSED) && - flattenIfIterable(v, registerDeferredClose) + flattenIfIterable(v, el) ) return; + asyncWrite(v); } catch (e) { - // Iterator setup threw outside any compute catch (#4112). handleError(e); - settleAutodispose(); - return; } - asyncWrite(v); settleAutodispose(); } }, diff --git a/packages/signals/src/core/error.ts b/packages/signals/src/core/error.ts index 4410e7499c..263645f235 100644 --- a/packages/signals/src/core/error.ts +++ b/packages/signals/src/core/error.ts @@ -52,9 +52,10 @@ export class StatusError extends Error { original: any ) { // Best-effort message: formatting must never replace the original (#4112). - let message: string | undefined; try { - message = String(original instanceof Error ? original.message : original); + 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 8f4289ae34..1fec7f9045 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; }