readline: reject instead of throwing when question() gets an aborted signal by robobun · Pull Request #33343 · oven-sh/bun

robobun · 2026-07-05T03:47:39Z

Repro

import { createInterface } from "node:readline/promises";
const rl = createInterface({ input: process.stdin, output: process.stdout });
rl.question("q", { signal: AbortSignal.abort() }).catch(e => console.log(e.code));

node prints ABORT_ERR. Bun throws synchronously before .catch is ever attached:

TypeError: |this| is not an object

Passing an already-aborted signal is the normal shape of cancellation-aware code (a request cancelled before the prompt is reached), and an async API that throws synchronously takes the caller down with it.

Cause

src/js/node/readline.ts captured Promise.reject detached from its receiver:

const PromiseReject = Promise.$reject;

Promise.reject builds its result with NewPromiseCapability(this), so calling it bare leaves this undefined and it throws |this| is not an object. The two call sites are the already-aborted fast paths of Interface.prototype.question[util.promisify.custom] and the node:readline/promises Interface#question. A signal that aborts later goes through new Promise(...)/$newPromiseCapability, which is why only the pre-aborted path was broken.

Fix

Bind the receiver, matching the ten other Promise.$reject / $resolve / withResolvers captures already in src/js:

const PromiseReject = Promise.$reject.bind(Promise);

Two sibling captures had the same shape and are bound in the same commit:

src/js/internal/fs/cp.ts — PromiseReject in pathExistsRejected, which would replace a real stat() error with the same TypeError.
src/js/internal/primordials.js — PromiseAll, which made SafePromiseAll throw on every call (PromiseResolve on the next line was already bound).

Verification

Three cases added to test/js/node/readline/readline_promises.node.test.ts: node:readline/promises question(), the util.promisify(rl.question) path, and a signal that aborts after the question was asked. All three match node (AbortError / ABORT_ERR, cause preserved, no prompt written for the pre-aborted case).

The first two fail on the unfixed build with TypeError: |this| is not an object and pass with the fix.

…signal Promise.reject needs the Promise constructor as its receiver, so a detached `Promise.$reject` threw "|this| is not an object" on the already-aborted fast path of Interface.prototype.question. Bind the same two other detached captures: internal/fs/cp.ts and PromiseAll in internal/primordials.js (used by SafePromiseAll).

coderabbitai · 2026-07-05T03:47:45Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2705acbe-42df-4769-ab40-1bf13e4563e5

📥 Commits

Reviewing files that changed from the base of the PR and between b8e7a95 and aefe760.

📒 Files selected for processing (4)

src/js/internal/fs/cp.ts
src/js/internal/primordials.js
src/js/node/readline.ts
test/js/node/readline/readline_promises.node.test.ts

Walkthrough

Internal Promise helper references (PromiseReject in cp.ts and readline.ts, PromiseAll in primordials.js) are changed to bound versions via .bind(Promise). New tests are added for readline/promises question() rejection behavior when an AbortSignal is already aborted or aborts after prompting, including a promisify-wrapped scenario.

Changes

Bound Promise helper fixes

Layer / File(s)	Summary
Bind Promise.$reject and Promise.all to Promise `src/js/internal/fs/cp.ts`, `src/js/node/readline.ts`, `src/js/internal/primordials.js`	PromiseReject and PromiseAll are reassigned to bound versions (`.bind(Promise)`) of the respective native methods so they execute with the correct receiver.
Readline promises abort-signal rejection tests `test/js/node/readline/readline_promises.node.test.ts`	Adds imports for `node:readline` and `promisify`, and a new describe block testing that `question()` rejects with an AbortError (matching name, code, and cause) when the signal is already aborted or aborts mid-prompt, including via `util.promisify`.

Sequence Diagram(s)

See hidden artifact diagram for the readline abort-signal rejection flow.

Compact metadata

Type: Bug fix + test addition
Scope: Internal Promise binding correctness, readline/promises test coverage

Related issues: None specified
Related PRs: None specified
Suggested labels: bug, tests, node-compat
Suggested reviewers: Maintainers familiar with Bun's Node.js compatibility layer and readline implementation

Poem
A rabbit hopped through Promise code,
Binding this where methods strode,
No more lost receivers astray,
Reject and all now know their way,
Abort signals tested, come what may. 🐇

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: readline now rejects instead of throwing for aborted signals.
Description check	✅ Passed	The description covers what the PR changes and how it was verified, though it doesn't use the template's exact section headings.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-07-05T03:47:48Z

^{Updated 11:19 PM PT - Jul 4th, 2026}

❌ @robobun, your commit aefe760 has 2 failures in Build #68435 (All Failures):

test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js - SIGABRT on 🐧 13 x64-asan
test/js/node/tls/tls-syscall-fault.test.ts - crash reported on 🐧 13 x64-asan

🧪 To try this PR locally:

bunx bun-pr 33343

That installs a local version of the PR into your bun-33343 executable, so you can run:

bun-33343 --bun

github-actions · 2026-07-05T03:51:23Z

Found 1 issue this PR may fix:

stream/web TransformStream + Node Transform.fromWeb causes internal webstreams_adapters crash (this is not an object) #30939 - SafePromiseAll calls Promise.all as an unbound free function, causing |this| is not an object in stream/web TransformStream + Transform.fromWeb. This PR binds Promise.all to Promise in primordials.js, directly fixing the root cause.

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #30939

🤖 Generated with Claude Code

github-actions · 2026-07-05T03:53:50Z

This PR may be a duplicate of:

Bind Promise.all in primordials so SafePromiseAll works as a free function #30940 - Also fixes unbound Promise.$reject/Promise.all captures in the same three files (readline.ts, primordials.js, cp.ts) using .bind(Promise), and adds readline abort tests

🤖 Generated with Claude Code

claude

LGTM — mechanical .bind(Promise) fix matching the existing pattern across src/js, with tests covering both entry points.

Extended reasoning...

Overview

Three one-line source changes, each adding .bind(Promise) to a detached static Promise method capture:

src/js/node/readline.ts — PromiseReject (the reported bug: question() with a pre-aborted signal threw |this| is not an object instead of returning a rejected promise)
src/js/internal/fs/cp.ts — PromiseReject in pathExistsRejected
src/js/internal/primordials.js — PromiseAll used by SafePromiseAll

Plus three new test cases in test/js/node/readline/readline_promises.node.test.ts covering the readline/promises path, the util.promisify(rl.question) path, and the late-abort path.

Security risks

None. This is internal plumbing for how Node-compat built-ins construct rejected promises; no user input handling, auth, crypto, or filesystem semantics change.

Level of scrutiny

Low. Promise.reject / Promise.all use NewPromiseCapability(this) per spec, so calling them without a receiver throws — a well-known JS gotcha. Adding .bind(Promise) is strictly additive: it cannot break any call site that worked before, only fixes the ones that threw. I grepped src/js and confirmed (a) this exact .bind(Promise) pattern is already used in 10+ other captures (streams/operators.ts, diagnostics_channel.ts, vm.ts, webstreams_adapters.ts, etc.), and (b) no remaining unbound = Promise.$xxx; captures exist after this PR.

Other factors

The tests are well-constructed: they reuse the existing FakeInput fixture, use using for cleanup, assert specific error shape (name, code, cause) rather than bare toThrow(), and verify the negative contract (prompt not written when pre-aborted). The PR description clearly explains the root cause and verified the tests fail on the unfixed build. The two sibling fixes (cp.ts, primordials.js) are exactly the "fix the whole class" pattern the repo guidelines ask for.

robobun · 2026-07-05T04:01:35Z

On the two bot suggestions:

Fixes #30939 — leaving it off. The primordials.js hunk does fix the root cause named in that issue's stack trace (SafePromiseAll calling an unbound PromiseAll), but the reported repro already passes on current main: #31991 moved the three webstreams_adapters.ts call sites from SafePromiseAll to SafePromiseAllReturnVoid, which goes through safePromiseAllCollect and never touches PromiseAll.

$ bun --revision
1.4.0-canary.1+1498d7b77    # does not contain this PR's primordials fix
$ bun repro.cjs             # the Transform.fromWeb snippet from #30939
hellohellohellohellohello

Nothing in src/js calls SafePromiseAll today, so that hunk is a latent-correctness fix, not a behavior change, and I can't write a test that fails without it. #30939 itself looks already fixed on main and can be closed on its own; I would rather not claim it here.

Duplicate of #30940 — same three source lines, from an earlier run of mine that I missed when checking for an existing PR. I've closed #30940 in favor of this one. Its regression test (Transform.fromWeb + _writev) went vacuous after #31991 for the reason above, while the rl.question() tests here still fail on an unfixed build:

$ USE_SYSTEM_BUN=1 bun test test/js/node/readline/readline_promises.node.test.ts
(fail) readline question() with an aborted signal > rejects instead of throwing synchronously
       TypeError: |this| is not an object
(fail) readline question() with an aborted signal > rejects instead of throwing synchronously through util.promisify
       TypeError: |this| is not an object

claude

LGTM — three one-line .bind(Promise) fixes matching the existing pattern, with tests that exercise the previously-broken path.

Extended reasoning...

Overview

Three source lines changed, all the same shape: a Promise static captured as a free function without its receiver is now bound to Promise.

src/js/node/readline.ts — PromiseReject = Promise.$reject.bind(Promise)
src/js/internal/fs/cp.ts — same
src/js/internal/primordials.js — PromiseAll = Promise.all.bind(Promise)

Plus three new tests in test/js/node/readline/readline_promises.node.test.ts covering rl.question() with a pre-aborted signal (both readline/promises and util.promisify paths) and a late-aborting signal.

Security risks

None. No user input parsing changes, no auth/crypto, no new surface. Binding the receiver is strictly a correctness fix for the existing call sites.

Level of scrutiny

Low. This is a mechanical bug fix with an unambiguous root cause: Promise.reject / Promise.all use NewPromiseCapability(this) and throw |this| is not an object when called with an undefined receiver. The fix (.bind(Promise)) is the established idiom already used ~10 times across src/js — the very next line in primordials.js (PromiseResolve) was already bound this way. Each changed line has exactly one possible correct fix and this is it.

Other factors

Tests are well-structured: reuse the file's existing FakeInput helper, use using for cleanup, assert specific {name, code, cause} rather than bare toThrow, and the PR shows they fail on the unfixed build with the exact reported TypeError.
The primordials.js hunk (SafePromiseAll) has no live caller today, which the author acknowledges as a latent-correctness fix; that's fine — it's still obviously correct and prevents the same bug from reappearing when a caller is added.
No CODEOWNERS cover the touched files. No outstanding human review comments.

node runs the whole body of readline/promises question() inside a new Promise(...) executor, so an invalid options.signal surfaces as a rejected promise rather than a synchronous ERR_INVALID_ARG_TYPE. Move validateAbortSignal and the abort wiring inside the try so the promise capability absorbs them, the way the executor does. This also routes the already-aborted fast path through the capability's reject instead of the bare PromiseReject. The promisify.custom path still uses that unbound capture and is fixed separately in #33343.

robobun · 2026-07-05T06:24:13Z

CI status: red on unrelated lanes, diff is green

aefe7606 is 7dd4673d plus an empty commit, so builds #68420 and #68435 compiled identical source. They fail on disjoint lanes:

lane	#68420	#68435
`debian 13 x64-asan - test-bun` (20 shards)	20/20 pass	2 shards fail
`windows 2019 x64-baseline - test-bun`	fail	pass
`darwin 26 aarch64 - test-bun`	artifact download timeout	artifact download timeout

Same code, different results, so none of it is reproducible failure.

#68435

test/js/node/tls/tls-syscall-fault.test.ts (x64-asan): ✗ a failed per-loop TLS buffer allocation reports out of memory instead of faulting inside SSL_read
test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js (x64-asan): SIGABRT, ASSERTION FAILED: !scope.exception() || !hasSlot
darwin 26 aarch64: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun', before any test ran

#68420

test/napi/napi.test.ts (windows 2019 x64-baseline): napi_wrap > has the right lifetime failing with Condition was not met after 100 GC attempts. The same file flaked and passed after retry on the 2019 x64 lane in that build, and again in #68435.
the same darwin artifact timeout, same agent

This diff is three .bind(Promise) tokens in src/js/node/readline.ts, src/js/internal/fs/cp.ts and src/js/internal/primordials.js, plus tests. None of those modules are loaded by the napi, TLS, or worker-message-port fixtures, and binary size is unchanged on every target except +0.5 KB on windows-x64.

Local verification on the readline tests:

$ USE_SYSTEM_BUN=1 bun test test/js/node/readline/readline_promises.node.test.ts
 4 pass
 2 fail      # TypeError: |this| is not an object

$ bun bd test test/js/node/readline/readline_promises.node.test.ts
 6 pass
 0 fail

I've used my one re-roll (the empty commit), so I'm not pushing another. Ready for review.

robobun · 2026-08-13T05:14:56Z

Closing: the readline fix landed separately.

#31827 (merged 2026-07-24) replaced src/js/node/readline.ts with the Node v26 readline stack. readline.js now takes PromiseReject from internal/repl/node-primordials, where it is invoked with Promise as the receiver, and readline/promises question() rejects from inside the promise executor, so an already-aborted signal produces a rejected promise instead of a synchronous throw. The PromiseAll binding this PR also touched in src/js/internal/primordials.js was removed as dead code in #36318.

Verified on current main (bdb7382): test/js/node/readline/readline_promises.node.test.ts from this branch, run unmodified against a debug build of main, passes (6 pass, including the three question() with an aborted signal cases added here) on two consecutive runs.

The one remaining piece, the unbound Promise.$reject capture in src/js/internal/fs/cp.ts, is still present on main and is being tracked separately.

github-actions Bot added the claude label Jul 5, 2026

claude Bot reviewed Jul 5, 2026

View reviewed changes

robobun mentioned this pull request Jul 5, 2026

Bind Promise.all in primordials so SafePromiseAll works as a free function #30940

Closed

robobun mentioned this pull request Jul 5, 2026

readline: throw ERR_USE_AFTER_CLOSE from write(), pause() and resume() after close() #33344

Open

ci: retrigger

aefe760

claude Bot reviewed Jul 5, 2026

View reviewed changes

This was referenced Jul 5, 2026

fs: run node:fs callbacks from the event loop, not a promise reaction #33366

Open

readline/promises: reject instead of throwing synchronously from Interface#question #33742

Closed

robobun closed this Aug 13, 2026

Navigation Menu

Uh oh!

readline: reject instead of throwing when question() gets an aborted signal - #33343

readline: reject instead of throwing when question() gets an aborted signal#33343
robobun wants to merge 2 commits into
mainfrom
farm/10997b8a/readline-question-aborted-signal

robobun commented Jul 5, 2026

Uh oh!

coderabbitai Bot commented Jul 5, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 5, 2026

Uh oh!

github-actions Bot commented Jul 5, 2026

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Jul 5, 2026

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Jul 5, 2026

Uh oh!

robobun commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

robobun commented Jul 5, 2026

Repro

Cause

Fix

Verification

Uh oh!

coderabbitai Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

robobun commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 5, 2026

Uh oh!

github-actions Bot commented Jul 5, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Jul 5, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Jul 5, 2026

CI status: red on unrelated lanes, diff is green

Uh oh!

robobun commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jul 5, 2026 •

edited

Loading

robobun commented Jul 5, 2026 •

edited

Loading