Bind Promise.all in primordials so SafePromiseAll works as a free function - #30940
Bind Promise.all in primordials so SafePromiseAll works as a free function#30940robobun wants to merge 3 commits into
Conversation
|
Updated 7:51 PM PT - Jun 18th, 2026
❌ @robobun, your commit 40547c5 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30940That installs a local version of the PR into your bun-30940 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughBinds internal Promise helpers ( ChangesPromise binding fixes and regression tests
🚥 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}
There was a problem hiding this comment.
LGTM — one-line fix binding Promise.all to Promise, matching the existing PromiseResolve pattern on the next line, with a regression test.
Extended reasoning...
Overview
This PR changes a single line in src/js/internal/primordials.js: const PromiseAll = Promise.all; → const PromiseAll = Promise.all.bind(Promise);. It also adds a regression test in test/js/node/stream/node-stream.test.js that exercises the Transform.fromWeb _writev path which calls SafePromiseAll.
Correctness
Promise.all requires its this to be a constructor per spec (it calls NewPromiseCapability(this)). SafePromiseAll invokes PromiseAll(...) as a free function, so this is undefined and JSC throws TypeError: |this| is not an object. Binding to Promise is the obvious, correct fix and exactly mirrors the pattern already used for PromiseResolve = Promise.$resolve.bind(Promise) on the immediately following line. PromiseAll is only referenced once in this file (inside SafePromiseAll), so there's no risk of changing other call sites.
Security risks
None. This is internal runtime glue code; binding a built-in method to its constructor introduces no new attack surface.
Level of scrutiny
Low. One-character-of-intent bug fix following an established adjacent pattern, with a clear repro, root-cause explanation, and a passing regression test. No design decisions or behavioral ambiguity.
Other factors
No bugs flagged by the bug-hunting system, no outstanding reviewer comments, and the test verifies end-to-end behavior (buffered writes through Transform.fromWeb produce the expected output rather than throwing).
There was a problem hiding this comment.
LGTM — one-line fix that matches the existing PromiseResolve binding pattern on the next line, with a regression test.
Extended reasoning...
Overview
This PR changes a single line in src/js/internal/primordials.js, replacing const PromiseAll = Promise.all with const PromiseAll = Promise.all.bind(Promise). The only consumer is SafePromiseAll two lines below, which calls PromiseAll(...) as a free function — without a bound receiver, JSC's Promise.all throws TypeError: |this| is not an object at NewPromiseCapability(this). The fix is identical in form to the immediately following line, const PromiseResolve = Promise.$resolve.bind(Promise), so it follows an established pattern in the same file. A regression test in test/js/node/stream/node-stream.test.js exercises the Transform.fromWeb → _writev → SafePromiseAll path that triggered the bug.
Security risks
None. This is an internal primordials helper; binding Promise.all to Promise has no security implications and does not change which capabilities are exposed.
Level of scrutiny
Low. The change is mechanical, mirrors the adjacent line, and the failure mode it fixes is well-documented in the PR description with a before/after test run. The added test is straightforward and self-contained (uses the global TransformStream, which is available in Bun's runtime). No CODEOWNERS apply to either modified file.
Other factors
The one inline comment posted alongside this review flags two pre-existing instances of the same unbound-Promise-static pattern in fs/cp.ts and readline.ts. Those are not introduced or affected by this PR and can be handled in a follow-up; they don't block this fix. The robobun CI comment shows failures on the first commit, but a ci: retrigger commit was pushed afterward and the change itself cannot plausibly cause unrelated test failures.
There was a problem hiding this comment.
LGTM — thanks for sweeping up the two extra unbound Promise.$reject spots and adding the readline regression test.
Extended reasoning...
Overview
Three one-line source changes, each adding .bind(Promise) to a captured Promise static so it can be called as a free function:
src/js/internal/primordials.js:96—Promise.all→Promise.all.bind(Promise)(the original #30939 fix;PromiseResolveon the next line already used this pattern)src/js/internal/fs/cp.ts:21—Promise.$reject→Promise.$reject.bind(Promise)src/js/node/readline.ts:46— same
Plus two regression tests: Transform.fromWeb buffered writes hitting _writev/SafePromiseAll, and readline/promises question() with a pre-aborted signal.
Security risks
None. No input parsing, auth, crypto, or permission logic is touched — this only changes the receiver binding of Promise statics in internal primordial captures.
Level of scrutiny
Low. The fix is mechanical and matches the established idiom already used at primordials.js:97 (PromiseResolve), internal/streams/operators.ts, and node/diagnostics_channel.ts. Each change strictly widens correctness (previously these threw TypeError: |this| is not an object on the affected paths; now they behave as intended). There is no plausible regression surface from binding a static to its own constructor.
Other factors
The two extra .bind(Promise) fixes in cp.ts/readline.ts were the follow-ups I flagged in my earlier review; the author applied them in 3420117 and the inline thread is resolved. Both new tests are self-contained, deterministic, and directly exercise the previously-broken paths. No CODEOWNERS apply to these files. The robobun CI comment references an earlier commit (6a68f58) before the follow-up was pushed, so it doesn't reflect the current head.
|
CI lane All other lanes passed, including Already spent my one |
…alone Transform.fromWeb's _writev invokes SafePromiseAll as a free function. Since PromiseAll was captured as `Promise.all` without binding, the call's `this` was undefined and Promise.all's NewPromiseCapability threw 'TypeError: |this| is not an object'. Fixes #30939
Same pattern as the primordials fix — `PromiseReject` was captured unbound,
so calling it as a free function invoked `Promise.$reject` with `this ===
undefined` and threw `TypeError: |this| is not an object`, masking the real
rejection reason.
- readline: `rl.question(q, { signal })` with a pre-aborted signal now
correctly rejects with AbortError instead of TypeError.
- fs/cp: `pathExists` now correctly propagates non-ENOENT stat errors
(e.g. EACCES on destParent) instead of replacing them with TypeError.
Both call sites mirror the already-bound `PromiseReject` in
`diagnostics_channel.ts` and `internal/streams/operators.ts`.
3420117 to
40547c5
Compare
|
Rebased onto main (past #31830, which refactored Re-verified after rebase: |
There was a problem hiding this comment.
LGTM — the two extra .bind(Promise) spots from my earlier comment are swept up and the rebase looks clean.
Extended reasoning...
Overview
Three one-line source changes adding .bind(Promise) to Promise static method references that are later invoked as free functions: Promise.all in src/js/internal/primordials.js, and Promise.$reject in src/js/internal/fs/cp.ts and src/js/node/readline.ts. Two regression tests added (Transform.fromWeb _writev path, and readline/promises question() with a pre-aborted signal).
Security risks
None. This only changes the this binding of internal Promise static references; no auth, crypto, permissions, or external input handling is affected.
Level of scrutiny
Low. The fix is mechanical and matches the established pattern already used on the adjacent line (PromiseResolve = Promise.$resolve.bind(Promise)) and elsewhere in the codebase (operators.ts, diagnostics_channel.ts). Binding a Promise static to Promise is strictly safe — these builtins require this to be a constructor, so the change can only fix throws, not introduce new behavior.
Other factors
My earlier inline comment flagging the two additional unbound Promise.$reject sites was addressed in 3420117 with a regression test, and that thread is resolved. The PR was rebased past #31830 with a trivial conflict resolution in cp.ts (re-applying the same one-liner to the refactored file), and targeted tests were re-verified post-rebase. The bug-hunting system found no issues. No outstanding reviewer comments remain.
|
Thanks for the re-review. Rebase is clean and all three |
|
Rebased-build CI (#63426, sha
This PR's three one-line |
|
Superseded by #33343, which carries the same three The regression test in this PR no longer exercises the fix. #31991 changed the three #33343 keeps the same source fix and tests it through the path that is still broken on main: Closing in favor of #33343. |
1 participant
Repro
Bun threw
TypeError: |this| is not an objectinsideinternal:webstreams_adapters.Cause
src/js/internal/primordials.jscapturedPromise.allunbound:SafePromiseAllcallsPromiseAll(...)as a free function — so itsthisisundefined, andPromise.all's internalNewPromiseCapability(this, …)throws.Consumers: the
Transform.fromWeb/Duplex.fromWeb_writevpath andcloseWriter/closeReaderinwebstreams_adapters.ts(lines 323, 621, 721). The 5-sync-write repro forces the writable side to queue and coalesce into a single_writevcall.Fix
Bind it to
Promise, matching the existing pattern on the next line forPromiseResolve:Verification
Fixes #30939