streams: use Web IDL "a promise resolved with" semantics for callback results - #32620
streams: use Web IDL "a promise resolved with" semantics for callback results#32620alii wants to merge 8 commits into
Conversation
… results Web IDL "a promise resolved with x" is `new Promise(r => r(x))` — always a fresh promise, with the thenable-assimilation hop observable in the spec's microtask ordering. `Promise.resolve(x)` returns x unchanged when x is already a native Promise and skips that hop; the whatwg/streams ref-impl's `promiseResolvedWith` explicitly avoids it for this reason. `shieldingPromiseResolve` (the wrapper every `$promiseInvokeOrNoop*` call goes through for start/pull/cancel/write/close/abort/transform/flush across all three controller types) used `Promise.$resolve(result)`, and `writableStreamDefaultControllerStart` wrapped its `startAlgorithm()` result the same way. When the result is already a Promise — TransformStream's `startAlgorithm` returns the constructor's startPromise capability — the short-circuit shifts `[[started]]` one microtask early relative to the spec, which is observable in the WPT transform-streams tests that depend on a controller-abort/cancel reaction observing the writable mid-"erroring" rather than already "errored". Both sites now go through `$newPromise()` + `$resolvePromise()`, matching the ref-impl. No callers depend on the identity short-circuit (every consumer only `.$then()`s the result). Claude-Session: https://claude.ai/code/session_01Qn8dDrArUZxc4towq11qzu
|
Closed: superseded by #33193, which rewrites the streams builtins in C++ and removes |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesFresh-promise assimilation in Streams builtins
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
534cc2a switched shieldingPromiseResolve to $newPromise() + $resolvePromise(p, result) to get the Web IDL 'a promise resolved with' two-hop microtask ordering the WPT transform-streams/cancel tests depend on. But $resolvePromise does thenable assimilation via Get(result, 'then') and invokes it when it differs from the builtin, so a monkey-patched Promise.prototype.then is reached for every async underlying-source/sink/transformer callback result. readablestreamtoarraybuffer.test.ts pins exactly that. For native-promise results, replicate PromiseResolveThenableJob's timing manually: $enqueueJob a job that chains through the intrinsic .$then. Same two-hop ordering (WPT cancel.any.js stays 11/11), Promise.prototype.then is not touched. writableStreamDefaultControllerStart now routes through the same helper instead of new Promise(r => r(startResult)), which had the same Get(x, 'then') exposure.
|
@robobun adopt |
|
Closed: superseded by #33193, which rewrites the streams builtins in C++ and removes |
UnderlyingSinkStartCallback's IDL return type is `any`, so Web IDL
"invoke" performs no promise conversion at that layer. The single
"a promise resolved with startResult" wrap happens in
SetUpWritableStreamDefaultController step 17 (via
$shieldingPromiseResolve in writableStreamDefaultControllerStart).
Going through $promiseInvokeOrNoopMethodNoCatch here applied an extra
$shieldingPromiseResolve wrap, which with the new always-fresh-promise
semantics delayed [[started]] by ~2 microtask ticks for
new WritableStream({start}). Matches ReadableStreamDefaultControllerStart
and the whatwg/streams reference implementation.
Adds microtask-ordering tests that match Node exactly for both the
promise-returning and sync-returning start() cases.
r.$then is a prototype-chain lookup of @then, so a start()-returned native promise with a null prototype would throw TypeError inside the $enqueueJob callback and leave the wrapper pending forever. And even with @then found, intrinsic Promise.prototype.then runs SpeciesConstructor(this), so a throwing @@species on the result threw inside the job the same way. Call $Promise.prototype.$then.$call(r, ...) (independent of r's prototype) wrapped in try/catch that rejects the wrapper on abrupt completion, matching PromiseResolveThenableJob step 3. A throwing @@species now errors the stream instead of leaving [[started]] pending plus an uncaught exception; null-prototype results flip [[started]] and writes proceed. Tests cover both tamper vectors; the @@species case now matches Node.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This adds an extra microtask. Can we avoid that?
|
The hop itself is the point of the PR: Web IDL "a promise resolved with x" is It only fires when the callback returns a native What can go: the export function shieldingPromiseResolve(result) {
const promise = $newPromise();
$resolvePromise(promise, result);
return promise;
}which produces the same tick timing (JSC's If that test's expectation can be relaxed to match spec, the 3-line |
…point tests shieldingPromiseResolve is now $newPromise() + $resolvePromise() — exactly Web IDL "a promise resolved with x" (NewPromiseCapability + Resolve), no $isPromise branch. The branch was a JS-level workaround for an upstream JSC bug (PromiseResolveThenableJobFastSlow bare-returns on a SpeciesConstructor throw, oven-sh/WebKit#256) plus a non-spec tamper-proof preservation; the JSC fix belongs in WebKit, and per spec the assimilation does observe a patched Promise.prototype.then (Node matches). readablestreamtoarraybuffer.test.ts: sync start(). The test pins that Bun.readableStreamToArray returns an InternalPromise; the async start() was incidental setup that only had counter==0 under the old Promise.$resolve short-circuit. streams.test.js: subprocess-isolate the .then-observability test (patching Promise.prototype.then permanently invalidates JSC's promiseThenWatchpointSet for the process) and the @@species test (so a pristine watchpoint routes to the FastSlow path under test). @@species test is .todo until oven-sh/WebKit#256 lands and WEBKIT_VERSION bumps. .then-observability now asserts exactly 2 (catches double-wrap regression). Null-proto comment corrected to spec semantics. WritableStreamInternals.ts: drop the stale ".$then so a monkey-patched .then is not reached" comment. Benched (release, macOS arm64, 100k no-op transform writes): sync 0.47→0.50 µs/write (+6%), async 0.46→0.50 (+9%). The previous $isPromise+$enqueueJob shape measured 0.56 async (+22%); the spec-exact intrinsic is both simpler and faster. Claude-Session: https://claude.ai/code/session_01Qn8dDrArUZxc4towq11qzu
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/streams/streams.test.js`:
- Line 348: The subprocess assertions in the streams tests are omitting stderr
from the expected payload, which makes failures harder to diagnose. Update the
relevant expect(...) calls in the streams test cases to include stderr alongside
stdout and exitCode, using a loose matcher for stderr instead of asserting it is
exactly empty. Use the existing subprocess assertion objects in the stream test
cases so both the speciesError case and the other affected assertion keep stderr
in the combined comparison.
- Around line 1443-1446: The test is using Bun.sleep(0) as a timer yield, but it
should wait for the stream startup condition instead. In streams.test.js,
replace the Bun.sleep(0) pause after creating the ReadableStream and
WritableStream with an await on a stream-visible completion signal tied to the
startup assimilation behavior, so the test advances only when the stream
condition is actually reached. Keep the Promise.prototype.then restoration in
place after the wait.
- Around line 328-349: The current test is marked as todo but still contains
executable subprocess assertions, so the throwing-@@species startup path is not
actually being validated. In streams.test.js, either convert the WritableStream
startup case in the `it.todo(...)` block to an active `it(...)` if the behavior
should now pass, or keep it as a true TODO by removing the subprocess body; use
the `WritableStream`/`start()` scenario and the `Bun.spawn` assertion as the
unique anchor when updating it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9344edbd-c448-4fbe-8b70-005652dbd0c8
📒 Files selected for processing (4)
src/js/builtins/StreamInternals.tssrc/js/builtins/WritableStreamInternals.tstest/js/bun/util/readablestreamtoarraybuffer.test.tstest/js/web/streams/streams.test.js
…sks instead of Bun.sleep(0) stderr in the asserted object surfaces the subprocess's diagnostic output in the failure diff without pinning it to empty (ASAN/debug builds emit benign warnings). The .then-observability fixture now drains microtasks via `for (let i = 0; i < 20; i++) await 1` (await on a non-thenable doesn't reach the patched .then) instead of Bun.sleep(0). The PromiseResolveThenableJob is a microtask, so no task-level yield is needed. Matches the flush pattern in the adjacent null-proto test.
There was a problem hiding this comment.
All prior review threads are resolved and no new issues found, but since this rewrites the promise-wrapping primitive used by every stream callback (with a documented +6-9%/write cost and a spec-mandated observable behavior change around .then), it's worth an explicit human sign-off on the final $newPromise()+$resolvePromise() shape.
Extended reasoning...
Overview
This PR changes shieldingPromiseResolve in src/js/builtins/StreamInternals.ts — the helper that wraps the result of every WHATWG stream callback (start/pull/cancel/write/close/abort/transform/flush) — from Promise.$resolve(x) (identity on native promises) to $newPromise() + $resolvePromise(x) (always-fresh, spec-exact Web IDL "a promise resolved with x"). It also restructures writableStreamDefaultControllerStart / setUpWritableStreamDefaultControllerFromUnderlyingSink so the spec's single wrap happens at exactly one layer, and adds ~150 lines of microtask-ordering / tamper-vector tests.
Security risks
None. No auth, crypto, permissions, or untrusted-input parsing is touched. The change is internal promise machinery; the only user-observable surface is microtask ordering and whether a user-patched Promise.prototype.then is reached during assimilation (now yes, per spec/Node).
Level of scrutiny
High. shieldingPromiseResolve sits on the hot path for every async stream operation in Bun. The PR documents a +6-9% per-write cost vs main, intentionally changes observable behavior (the new subprocess test pins exactly 2 .then calls where main saw 0), and modifies an existing test's setup (readablestreamtoarraybuffer.test.ts switched to sync start()) to keep its assertion passing. The implementation went through three distinct shapes during review ($resolvePromise → $enqueueJob+closures → back to $resolvePromise), with the final shape chosen after a maintainer was explicitly asked "which do you prefer?". One edge case (@@species throw) is .todo'd pending an upstream JSC fix.
Other factors
- All four of my prior inline comments (double-wrap on the underlying-sink path × 3 rounds;
$enqueueJobtamper-proofing) were addressed and are resolved. The one remaining nit (TransformerStartCallback timing) was explicitly and reasonably deferred to stacked work. - All CodeRabbit threads are resolved; the bug hunter found nothing this run.
- No CODEOWNERS cover the touched paths.
- The robobun summary references "per Jarred's review" for the final shape and a maintainer issued
adopt, so humans have clearly been engaged — but there is no formal approval on the timeline. Given the hot-path scope, perf trade-off, and intentional behavior change, an explicit human approval is the right gate rather than a bot shadow-approval.
Honest answer: no — the microtask is Web IDL "a promise resolved with x" ( So the choice is:
The 2 tests this fixes are edge-case microtask-ordering races ( (Side finding while investigating: this surfaced an unrelated upstream JSC bug where |
|
Superseded by #33193, which rewrites the streams builtins in C++ and deletes |
3 participants
Web IDL "a promise resolved with x" is
NewPromiseCapability+Resolve(x)— always a fresh promise.Promise.resolve(x)returns x unchanged when x is already a Promise and skips the assimilation hop, which WPT's transform-streams microtask-ordering tests observe.Changes
shieldingPromiseResolve→$newPromise()+$resolvePromise()writableStreamDefaultControllerStart→$shieldingPromiseResolve(startAlgorithm())SetUpWritableStreamDefaultControllerstep 17. TransformStream'sstartAlgorithmreturns itsstartPromisecapability;Promise.$resolvewould short-circuit.setUpWritableStreamDefaultControllerFromUnderlyingSink→ rawstartMethod.$call(...)UnderlyingSinkStartCallbackIDL return type isany— no promise conversion at that layer; the single wrap is step 17. Prevents double-wrap.readDirectStreamclosePromiseCapabilityremovalreadablestreamtoarraybuffer.test.ts→ syncstart()Bun.readableStreamToArrayreturns anInternalPromise. Itsasync start()was incidental setup that hadcounter==0only because of the old short-circuit; per spec the wrap does call public.thenfor thenables (Node matches)..then-observability test (new, subprocess).thencalls for twoasync start()s.toBe(2)catches double-wrap regression. Subprocess-isolated because patchingPromise.prototype.thenpermanently invalidates JSC'spromiseThenWatchpointSetfor the process.@@speciestest → subprocess +.todopromiseResolveThenableJobFastSlowbare-returns onSpeciesConstructorthrow. Subprocess so a pristine watchpoint routes to the FastSlow path under test. Un-todo onceWEBKIT_VERSIONbumps.[[started]]timing tests (4)start() → undefined/→ Promise/ nostart()/ TransformStream.Perf (release, macOS arm64, 100k no-op transform writes)
Promise.$resolve)The earlier
$isPromise+$enqueueJobiteration (a JS-level workaround for the JSC bug) measured 0.56 async (+22%); the spec-exact intrinsic is both simpler and faster.Unblocks
#32595 (
transformer.cancel) → 11/11 WPTcancel.any.js. #32601 (transform-streams WPT) → 132/133. The one remaining failure is an unrelated pre-existing hang.