Conversation
|
Updated 8:06 AM PT - May 30th, 2026
❌ @robobun, your commit 4751cff has some failures in 🧪 To try this PR locally: bunx bun-pr 29139That installs a local version of the PR into your bun-29139 --bun |
|
@robobun adopt |
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
This PR modifies core net.Socket behavior (pause/resume/read, constructor options, removing hardcoded readable:true, writable:true) as part 2 of a 5-part stack — worth a human look given the breadth of changes to the socket lifecycle.
Extended reasoning...
Overview
This PR adds Socket({handle, manualStart}) support to Bun's node:net by wiring _handle.onread = onStreamRead for stream-wrap handles (those with a callable readStart). It touches src/js/internal/shared.ts (moving owner_symbol), src/js/node/net.ts (core socket logic), and adds a new test file.
Security risks
No security-sensitive code paths (auth, crypto, permissions). The changes are confined to stream reading mechanics. No obvious injection or privilege escalation vectors.
Level of scrutiny
This deserves human review because it:
- Removes hardcoded
readable: true, writable: truefrom the Duplex constructor call — a behavioral change that affects allSocketinstances, not just those with stream-wrap handles. - Rewrites
pause,resume, andreadprototype methods with newkBuffer-gating logic. - Adds a new
_writeguard that returnsERR_METHOD_NOT_IMPLEMENTEDfor stream-wrap handles. - Is explicitly part 2 of a 5-part stack, so the full picture of how these pieces interact is not yet visible in this diff alone.
Other factors
The bug report identifies kBufferGen as set but never used. The PR author is aware this is intentionally deferred to parts 3/4 (native TTY/Pipe handles), but there is no comment in the code noting this, which could confuse future readers. The test suite verifies the mock-handle path and reports 143 passing / 0 failing. No prior human review has been posted.
|
Rebased, conflict resolved, What was done
CI status Diff is green on everything that touches this change; just needs a maintainer to merge (or re-run the macOS shard). I |
|
@robobun Rebase main before building |
f8e3a43 to
8f98c94
Compare
8f98c94 to
967c6bb
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js/node/net.ts:758-766— In the Socket constructor,this.read(0)is called (line ~747) beforethis[kBuffer]is set totrue(line ~761), so if a user-provided handle'sreadStart()firesonreadsynchronously,onStreamReadruns withkBufferstill falsy and routes data throughself.push()instead ofself[kBufferCb]— the user'sonread.callbackis never invoked for that first chunk. Fix by moving thekBuffer/kBufferCb/kBufferGensetup block to before thethis.read(0)call.Extended reasoning...
The bug: In the Socket constructor in
src/js/node/net.ts, the block that callsthis.read(0)(the\!options.manualStartbranch) executes at approximately line 747, butthis[kBuffer] = trueis set at approximately line 761. This means whennew Socket({handle, onread: {buffer, callback}})is called withoutmanualStart: true, reading is started before the kBuffer flag is established.The code path that triggers it: (1) Constructor reaches the handle block and calls
this.read(0). (2)Socket.prototype.readchecksthis[kBuffer]but it is still undefined/false, so it skips itstryReadStartfast-path and falls through toDuplex.prototype.read.(this, 0). (3) Duplex eventually callsSocket.prototype._read, which does NOT guard onkBuffer— it unconditionally callstryReadStart(this)whensocket.readStartis callable. (4)tryReadStartsetsreading=trueand callshandle.readStart(). (5) IfreadStart()synchronously invokesthis.onread = onStreamRead, thenonStreamReadruns before step 6. (6) Only afterread(0)returns does line 761 finally setkBuffer=true.Why existing code doesn't prevent it:
Socket.prototype.readhas akBufferguard, butSocket.prototype._readdoes not — it callstryReadStartunconditionally when the handle hasreadStart. This asymmetry means the guard inread()can be bypassed via the_read→tryReadStartpath.Impact: For
new Socket({handle, onread:{buffer, callback}})with a custom JS handle whosereadStart()firesonreadsynchronously, the first data chunk is routed throughself.push(arrayBuffer)rather thanself[kBufferCb](nread, userBuf). The user'sonread.callbackis never called for that initial chunk. The PR introduces this exact use case (custom JS mock handles), and all tests work around the bug by always usingmanualStart: true.Addressing the refutation: The refutation argues that native handles (libuv, Bun TCP, pipe, TLS) are asynchronous and never fire
onreadsynchronously fromreadStart(), making the bug implausible in practice. This is correct for native handles. However, the PR's stated primary use case is custom JS mock handles ("The native TTY/Pipe handles arrive in parts 3/4"), and these pure-JS objects CAN callonreadsynchronously fromreadStart(). All existing tests usemanualStart: trueprecisely because of this ordering issue — that's a workaround, not a design choice. The ordering is objectively incorrect regardless of whether native handles trigger it.Step-by-step proof: Given:
const myBuf = Buffer.alloc(16); const calls = []; const socket = new Socket({ handle, onread: { buffer: myBuf, callback: (n, b) => calls.push({n, b}) } });wherehandle.readStart()synchronously callshandle.onread(5, Buffer.from('hello')). Trace: (1) constructor callsthis.read(0)— at this pointthis[kBuffer]is stillundefined. (2)Socket.prototype.readskips tryReadStart (kBuffer falsy) but callsDuplex.prototype.read. (3) Duplex calls_read, which callstryReadStart. (4)tryReadStartcallshandle.readStart(). (5)handle.readStartcallsonStreamRead.call(handle, 5, handleBuf). (6) InonStreamRead:self[kBuffer]is falsy → takes theself.push(arrayBuffer)branch. (7) Constructor resumes;this[kBuffer] = trueis set. Result:calls.length === 0. The callback was never called for the first chunk.Fix: Move the
if (onread) { ... this[kBuffer] = true; this[kBufferCb] = ...; this[kBufferGen] = ...; }block to before theif (this._handle && $isCallable(this._handle.readStart) && options.readable \!== false)block that triggersread(0).
bf5e6a9 to
3448ab4
Compare
d430101 to
ab0c9c6
Compare
477d9a0 to
03a89b3
Compare
|
@robobun rebase and get this mergable |
679ec4b to
f940460
Compare
85262d2 to
11bc1c4
Compare
11bc1c4 to
dbc7793
Compare
Node's net.Socket accepts a {handle} option (a stream-wrap with
readStart/readStop/onread) and wires onStreamRead per
lib/internal/stream_base_commons.js. This adds the same plumbing:
- Constructor accepts {handle, manualStart, pauseOnCreate}
- initSocketHandle wires _handle.onread = onStreamRead when readStart
is callable
- onStreamRead(nread, buf): nread>0 → push, UV_EOF → push(null),
nread<0 → destroy(ErrnoException)
- pause/resume/read are kBuffer-gated (Node's pattern) and no longer
call _handle.pause()/resume() unconditionally — usocket backpressure
is handled separately by SocketHandlers.data + _read
- _read branches on readStart → tryReadStart for stream-wrap handles
- Drop hardcoded readable:true/writable:true so {writable:false} works
- _write/endNT guard for stream-wrap handles (write support is part 5)
owner_symbol moves to internal/shared so the handle's [owner_symbol]
back-ref is consistent.
Part 2/5 of the process.stdin Node-parity stack (#29126).
Reverts the kBufferGen removal from the previous commit. The symbol
isn't dead — Node's onread.{buffer,callback} contract is that the
callback receives the user's buffer. onStreamRead now copies the
handle's chunk into the user buffer before invoking the callback.
(Restores 8f98c9444d which was overwritten by the prior force-push.)
…rder
Socket.prototype.{pause,resume,read} were kBuffer-gated, which skipped
_handle.pause()/resume() for regular Bun TCP sockets (no onread option)
and broke pauseOnConnect. Branch on readStop/readStart instead: stream-
wrap handles use the kBuffer-gated readStop/readStart path (Node's
lib/net.js), usocket handles keep the unconditional native pause/resume.
Also:
- Move kBuffer/kBufferCb/kBufferGen assignment before the constructor's
read(0) so a handle whose readStart fires onread synchronously routes
through kBufferCb instead of push().
- Check readStop() return in the pauseOnCreate branch (matches pause()).
- Clamp nread to userBuf.byteLength in onStreamRead's copy path so the
callback's (nread, buf) contract holds.
Fixes test-net-server-pause-on-connect.js timeout across all platforms.
AF_UNIX DGRAM/SEQPACKET sockets now return PIPE instead of UNKNOWN, matching uv_guess_handle.
Match Node's stream_base_commons.js onStreamRead: - call _unrefTimer() on every invocation so an actively-reading stream-wrap socket doesn't fire 'timeout' - in the non-onread branch, push arrayBuffer.subarray(0, nread) when the handle reports fewer bytes than the buffer it allocated, instead of pushing trailing garbage
fb211d7 to
cbbf986
Compare
…-as-ptr Move the SAFETY comment directly above each getsockname/getsockopt unsafe block and pass the socklen_t out-params through core::ptr::from_mut instead of an implicit &mut→*mut coercion.
Match Node's lib/net.js (truthy this._handle.reading) and stay symmetric with resume()/read() which gate on !handle.reading. A stream-wrap handle that never started reading (no reading property) is no longer readStop()'d by an early pause().
|
Heads-up from #42313 (approved, not merged yet). It turns
|
Part 2/5 of the
process.stdinNode-parity stack (#29126). Stacked on #29137.Adds
net.Socket({handle, manualStart})support per Node's stream-wrap contract:{handle, manualStart, pauseOnCreate};initSocketHandlewires_handle.onread = onStreamReadwhenreadStartis callableonStreamRead(nread, buf):nread>0→ push,UV_EOF→ push(null),<0→ destroypause/resume/readarekBuffer-gated (Node'slib/net.jspattern) — usocket backpressure is independently handled bySocketHandlers.data+_read_readbranches onreadStart→tryReadStartreadable:true, writable:trueso{writable:false}is honoredowner_symbolmoves tointernal/sharedTested with a JS mock handle (the native
TTY/Pipehandles arrive in parts 3/4). net/stdin/readline/stream suites: 143 pass / 0 fail (3 net failures pre-existing on system bun).Stack:
guessHandleType(node_util_binding: add guessHandleType(fd) #29137)TTYhandle +tty.ReadStream extends net.SocketPipehandle +createHandlewriteBuffer/shutdown