Repository navigation
Conversation
Fixes #29126. After process.stdin.on('readable', fn) → removeListener('readable', fn), Bun kept polling fd 0, so a stdio:'inherit' child raced the parent for input. Node releases fd 0 via backpressure: tty.ReadStream extends net.Socket with readableHighWaterMark: 0, so push() returns false on every chunk and the handle's readStop() fires; _read() re-acquires only when a consumer pulls. Bun's tty.ReadStream extended fs.ReadStream (with default hwm) and the stdin wrapper had no backpressure-driven stop. This rewrites the TTY path to match Node's architecture: Native: - New Zig TTY handle (tty.classes.ts + TTY.zig) backed by bun.io.BufferedReader + StreamingWriter, exposing readStart/readStop/ setRawMode/getWindowSize/ref/unref/close/onread plus the usocket surface net.Socket expects (pause/resume/$write/$end/bytesWritten). JSRef for GC liveness; fd reopened O_NONBLOCK with original-fd fallback (libuv-style). - ProcessBindingTTYWrap.cpp: deleted C++ TTYWrapObject; the codegen TTY is now process.binding('tty_wrap').TTY. JS: - tty.ReadStream: extends net.Socket, _handle = new TTY(fd), readableHighWaterMark: 0, manualStart: true — line-for-line Node lib/tty.js. - net.Socket: accepts {handle, manualStart}; initSocketHandle wires onread/ondrain when _handle.readStart is callable; new onStreamRead/tryReadStart (Node stream_base_commons pattern). - ProcessObjectInternals.ts getStdinStream: TTY branch is now just new tty.ReadStream(fd) + readStop + 'pause' listener (Node is_main_thread.js parity). Pipe/file path unchanged. After removeListener('readable'), Readable stops calling _read(); the in-flight read delivers one chunk, push() returns false, readStop() unregisters the poll, and the child reads fd 0 exclusively. No listener tracking, no _readableState pokes, no instance method overrides.
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
github-actions
Bot
changed the title
|
Found 8 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
WalkthroughThis PR replaces the C++ TTY wrapper with a Zig-native Changes
Possibly related PRs
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}
| } | ||
| } | ||
| } | ||
|
|
||
| // Node lib/internal/stream_base_commons.js onStreamRead, adapted to the | ||
| // (buf | null | Error) encoding the Zig TTY handle uses. | ||
| function onStreamRead(buf) { | ||
| const self = this[owner_symbol]; | ||
| if (buf === null) { | ||
| self.push(null); | ||
| self.read(0); | ||
| return; | ||
| } | ||
| if (buf instanceof Error) { | ||
| self.destroy(buf); | ||
| return; | ||
| } | ||
| self.bytesRead += buf.length; | ||
| if (!self.push(buf)) { | ||
| this.reading = false; |
There was a problem hiding this comment.
🔴 After TTY stdin reaches EOF, onStreamRead(null) calls self.read(0) (net.ts:2596), which hits Socket.prototype.read (net.ts:1224) that unconditionally invokes this._handle?.resume() — aliased to readStart for TTY handles via tty.classes.ts:27 — before delegating to Duplex.prototype.read. This incorrectly calls readStart() post-EOF, re-activating the TTY handle's event loop keepalive and (since isDone() is false for TTY with close_handle=false) potentially re-registering the underlying poll, causing the process to hang after stdin EOF. Node.js's Socket.prototype.read does not call handle.resume() unconditionally; the fix is to guard the resume() call in Socket.prototype.read behind \!socket.readStart, matching the branching already in Socket.prototype._read.
Extended reasoning...
What the bug is and how it manifests
onStreamRead in net.ts (the new StreamBase commons callback added by this PR) handles EOF by calling self.push(null) then self.read(0). The intent of self.read(0) (borrowed from Node's stream_base_commons.js) is to trigger internal flow bookkeeping without consuming data. However, Bun's Socket.prototype.read does something Node's does not: it unconditionally calls this._handle?.resume() before delegating to Duplex.prototype.read:
Socket.prototype.read = function read(size) {
if (\!this.connecting) {
this._handle?.resume(); // ← called unconditionally for ALL handle types
}
return Duplex.prototype.read.$call(this, size);
};The specific code path that triggers it
For TTY handles, tty.classes.ts line 27 aliases resume → readStart. So the call chain is:
- TTY fd sends EOF →
onReaderDone()→callOnRead(jsNull()) onStreamRead(null)fires (net.ts:2591) →self.push(null)thenself.read(0)Socket.prototype.readis invoked —this._handle.resume()fires beforeDuplex.prototype.readresume=readStart(TTY.zig) is called post-EOF
Inside readStart's else-branch (reader already started):
this.reader.unpause()is calledwatch()is conditionally skipped only ifisDone(), but for TTY withclose_handle=false(set at TTY.zig:124),close_handle=falsemeanscloseWithoutReporting()setsclosed_without_reporting=truebut does NOT callhandle.close(), so the handle stays as.pollandisDone()returns false after EOF — meaningwatch()IS called, re-registering the pollupdateRef(true)is called unconditionally (noisDone()guard), incrementing the event loop's keepalive count
Why existing code doesn't prevent it
Socket.prototype._read correctly handles TTY vs usocket by branching on $isCallable(socket.readStart) and only calling tryReadStart if \!socket.reading. But Socket.prototype.read has no such branch — it calls handle.resume() for all handle types. This was harmless before this PR when TTY used fs.ReadStream, but now that TTY goes through the Socket path, the TTY handle's readStart is triggered post-EOF.
Addressing the refutations
One refutation argues that PosixBufferedReader.updateRef has an internal guard (getPoll() orelse return) and that isDone() is always true when handle is .closed. This is true for non-TTY readers, but TTY.zig explicitly sets close_handle=false (line 124), which means the handle never transitions to .closed — it stays .poll — and finish() returns early without setting is_done=true. So isDone() is false after EOF for TTY, and getPoll() returns the still-live poll handle, making updateRef(true) a real operation.
A second refutation argues socket.reading is still true after push(null) so tryReadStart is blocked. This is true for the _read path, but irrelevant here: Socket.prototype.read calls handle.resume() before ever reaching _read. The socket.reading guard only exists in Socket.prototype._read / tryReadStart, not in Socket.prototype.read.
Impact
Any program that creates a TTY process.stdin (i.e., runs with a terminal) and consumes stdin until EOF will hang indefinitely because the event loop stays alive after stdin EOF. This is a regression introduced by this PR — previously, tty.ReadStream extended fs.ReadStream and did not go through the Socket.prototype.read code path.
How to fix it
Match Node.js behavior: in Socket.prototype.read, guard the resume() call to only fire for usocket-style handles (those without a readStart method):
Socket.prototype.read = function read(size) {
if (\!this.connecting && \!($isCallable(this._handle?.readStart))) {
this._handle?.resume();
}
return Duplex.prototype.read.$call(this, size);
};For TTY handles, _read() → tryReadStart() is the correct path for re-arming reads; read() should not trigger readStart independently.
Step-by-step proof
- User runs
bun script.jsin a terminal withprocess.stdin.on('data', ...) - Terminal sends EOF (Ctrl+D)
BufferedReadercallsonReaderDone()→callOnRead(jsNull())- JS
onStreamRead(null)fires: callsself.push(null)thenself.read(0) Socket.prototype.readexecutesthis._handle.resume()(=readStart()) — this is the bugreadStartelse-branch:isDone()is false (close_handle=false), sowatch()re-registers the poll ANDupdateRef(true)increments event loop active count- The poll that was just re-registered will never fire (EOF fd), but the active count prevents the event loop from draining
- Process hangs — the event loop never exits even though all user work is done
| Object.defineProperty(ReadStream, "prototype", { | ||
| get() { | ||
| const Prototype = Object.create(fs.ReadStream.prototype); | ||
|
|
||
| // Add ref/unref methods to make tty.ReadStream behave like Node.js | ||
| // where TTY streams have socket-like behavior | ||
| Prototype.ref = function () { | ||
| // Get the underlying native stream source if available | ||
| const source = this.$bunNativePtr; | ||
| if (source?.updateRef) { | ||
| source.updateRef(true); | ||
| } | ||
| return this; | ||
| }; | ||
|
|
||
| Prototype.unref = function () { | ||
| // Get the underlying native stream source if available | ||
| const source = this.$bunNativePtr; | ||
| if (source?.updateRef) { | ||
| source.updateRef(false); | ||
| } | ||
| return this; | ||
| }; | ||
| const { Socket } = require("node:net"); | ||
| const Prototype = Object.create(Socket.prototype); | ||
|
|
||
| Prototype.setRawMode = function (flag) { | ||
| flag = !!flag; | ||
|
|
||
| // On windows, this goes through the stream handle itself, as it must call | ||
| // uv_tty_set_mode on the uv_tty_t. | ||
| // | ||
| // On POSIX, I tried to use the same approach, but it didn't work reliably, | ||
| // so we just use the file descriptor and use termios APIs directly. | ||
| if (process.platform === "win32") { | ||
| // Special case for stdin, as it has a shared uv_tty handle | ||
| // and it's stream is constructed differently | ||
| if (this.fd === 0) { | ||
| const err = ttySetMode(flag); | ||
| if (err) { | ||
| this.emit("error", new Error("setRawMode failed with errno: " + err)); | ||
| } | ||
| return this; | ||
| } | ||
|
|
||
| const handle = this.$bunNativePtr; | ||
| if (!handle) { | ||
| this.emit("error", new Error("setRawMode failed because it was called on something that is not a TTY")); | ||
| return this; | ||
| } | ||
|
|
||
| // If you call setRawMode before you call on('data'), the stream will | ||
| // not be constructed, leading to EBADF | ||
| // This corresponds to the `ensureConstructed` function in `native-readable.ts` | ||
| this.$start(); | ||
|
|
||
| const err = handle.setRawMode(flag); | ||
| if (err) { | ||
| this.emit("error", err); | ||
| return this; | ||
| } | ||
| } else { | ||
| const err = ttySetMode(this.fd, flag); | ||
| if (err) { | ||
| this.emit("error", new Error("setRawMode failed with errno: " + err)); | ||
| return this; | ||
| } | ||
| const err = this._handle?.setRawMode(flag); | ||
| if (err) { | ||
| this.emit("error", new Error("setRawMode failed with errno: " + err)); | ||
| return this; | ||
| } | ||
|
|
||
| this.isRaw = flag; | ||
|
|
||
| return this; | ||
| }; | ||
|
|
||
| Object.defineProperty(ReadStream, "prototype", { value: Prototype }); | ||
|
|
||
| Object.setPrototypeOf(ReadStream, Socket); | ||
| return Prototype; | ||
| }, | ||
| enumerable: true, |
There was a problem hiding this comment.
🔴 The lazy ReadStream.prototype getter creates Object.create(Socket.prototype) but never sets Prototype.constructor = ReadStream, so (new tty.ReadStream(fd)).constructor === Socket instead of ReadStream. Code that uses .constructor for type checks or reads .constructor.name for error reporting will see 'Socket' instead of 'ReadStream'; fix by adding Prototype.constructor = ReadStream; inside the lazy getter.
Extended reasoning...
What the bug is and how it manifests
In src/js/node/tty.ts, the lazy ReadStream.prototype getter (lines 50–70) does:
const Prototype = Object.create(Socket.prototype);
// ... methods added ...
Object.defineProperty(ReadStream, "prototype", { value: Prototype });Object.create(Socket.prototype) produces an object with no own constructor property. When code accesses Prototype.constructor, it walks up the prototype chain to Socket.prototype.constructor, which is Socket. So (new tty.ReadStream(0)).constructor === Socket — not ReadStream.
The specific code path
new ReadStream(fd) calls Socket.$call(this, ...), which sets up the instance correctly. But ReadStream.prototype (the object that new ReadStream() instances inherit from) was created via Object.create(Socket.prototype) without an own constructor property. The inherited value is Socket, not ReadStream.
Why existing code doesn't prevent it
The old code used $toClass(ReadStream, 'ReadStream', fs.ReadStream), which set up the constructor property correctly. The new PR dropped $toClass and replaced it with a manual lazy getter that omits the constructor assignment. Node.js itself is explicit about this — in Node's lib/tty.js:
ReadStream.prototype = Object.create(net.Socket.prototype, {
constructor: { value: ReadStream, ... }
});The codebase itself follows this pattern elsewhere — events.ts:89 has EventEmitterPrototype.constructor = EventEmitter — but the ReadStream getter does not.
Impact
instanceof checks still work (they use the prototype chain, not .constructor), so the primary use of ReadStream is unaffected. However:
someReadStream.constructor === tty.ReadStreamreturnsfalsesomeReadStream.constructor.namereturns'Socket'instead of'ReadStream'- Error reporting, debugging tools, and libraries that do constructor-based type discrimination (common in the Node.js ecosystem) will see the wrong class name.
process.stdin.constructor.namereturns'Socket'on a TTY, which is a Node.js compatibility regression.
Step-by-step proof
const tty = require('node:tty');- The lazy getter runs:
Prototype = Object.create(Socket.prototype) Prototypehas no ownconstructorproperty.Object.defineProperty(ReadStream, 'prototype', { value: Prototype })—Prototypeis nowReadStream.prototype.(new tty.ReadStream(0)).__proto__ === Prototype— the instance inherits fromPrototype.(new tty.ReadStream(0)).constructor— no own property, walks toPrototype.__proto__which isSocket.prototype, findsSocket.prototype.constructor === Socket.- Result:
(new tty.ReadStream(0)).constructor === Socket✓ (the bug), notReadStream.
How to fix
Add one line inside the lazy getter after Object.create(Socket.prototype):
Prototype.constructor = ReadStream;2 participants
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.