Repository navigation
Conversation
…ut() docs Socket.end() previously issued close(2) once its buffer was flushed, and Socket.shutdown(true) issued shutdown(SHUT_RD). Both discard the peer's reply after a request/reply half-close, which is what the documented contract (and Node's net.Socket.end/setTimeout) promise to keep readable. end() and shutdown() (with any argument) now both flush and issue shutdown(SHUT_WR). The socket stays readable until the peer closes, at which point uSockets' existing eof-on-shut-down branch closes it and on_close runs the teardown. node:net's _final already relied on the no-arg shutdown() being SHUT_WR, so its behavior is unchanged; the server drop path (blockList / maxConnections) switches to close() since it wants an immediate close, not a half-close. Also rewrite the timeout() JSDoc: it is notify-only (the handler must close the socket itself) and rounds up to the 4-second sweep granularity.
|
Updated 12:42 PM PT - Jul 26th, 2026
❌ @robobun, your commit c3fe786 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35947That installs a local version of the PR into your bun-35947 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Status: ready for review; diff is green. Reproduced with Review follow-ups: CI (build 82542): no socket/net/tls/http/http2 failures. The red is:
|
WalkthroughChangesThe PR updates Socket half-close behavior
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bun/net/socket.test.ts`:
- Around line 3394-3400: Extend the half-close call matrix used by the
concurrent tests around halfClose to include end("final") alongside the existing
end() and shutdown variants. Ensure this case uses the same assertion,
confirming the full PAYLOAD reply is received while writeAfter remains -1 and
readyState remains 1.
🪄 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: 5f2c29ca-8746-4e1b-be8c-655e3662e677
📒 Files selected for processing (4)
packages/bun-types/bun.d.tssrc/js/node/net.tssrc/runtime/socket/socket_body.rstest/js/bun/net/socket.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/socket/socket_body.rs:2983-2991—Socket[Symbol.dispose]is still wired toend(sockets.classes.ts:164-166), so withend()now a SHUT_WR half-close,using socket = await Bun.connect(...)no longer unconditionally releases the fd/poll_ref on scope exit — teardown is deferred until the peer sends FIN back, and a hung orallowHalfOpenpeer leaves the socket alive with the event-loop ref held. This PR already made the exact same fix for thenet.tsdrop-path callers (clientHandle.end()→clientHandle.close(), "since it wants an immediate close");@@disposeneeds the same treatment — point it atclose.Extended reasoning...
What changed and what didn't
src/runtime/socket/sockets.classes.ts:164-166(untouched by this PR) bindsSocket[Symbol.dispose]to theendhost function:"@@dispose": { fn: "end", length: 0, },
Before this PR,
end()setEND_AFTER_FLUSH, and the end-after-flush arm ofinternal_flush()calledself.mark_inactive()→close_and_detach(Normal). That is a fullclose(2): it clearsIS_ACTIVE, downgrades the Strongthis_value, decrementsactive_connections, and — critically —poll_ref.unref(js_loop_ctx())(socket_body.rs:1330). Sousing client = await Bun.connect(...)unconditionally released the fd and dropped the event-loop ref on scope exit, satisfying the Explicit Resource Management contract.This PR rewires that arm (socket_body.rs:2983-2991) to
self.socket.get().shutdown()(SHUT_WR) instead ofmark_inactive().IS_ACTIVEstays set,poll_refstays reffed, and teardown is deferred until the peer's FIN arrives and uSockets'eof && is_shut_downbranch inloop.ccloses the socket, at which pointon_close→mark_inactive()finally runs.Why this is a regression for
Symbol.disposeThe half-close is correct for the explicit
socket.end()request-then-read-reply use case the PR targets. ButSymbol.disposehas a different contract: it is unconditional resource release on scope exit. After this change,usingcleanup is contingent on peer cooperation:- A peer with
allowHalfOpen: truethat never answers the FIN leaves the client socket open indefinitely. - A hung/partitioned peer leaves the fd allocated and
poll_refheld, so the process cannot exit. test/js/bun/net/socket.test.tsalone has ~8+ tests written asusing client = await Bun.connect(...)(e.g. lines 578, 1887, 2526, 2578, 3002, 3072, 3145, 3188) whose cleanup now depends on the server closing its side.
Step-by-step proof
using client = await Bun.connect({ hostname, port, socket: {...} })— connects to a server withallowHalfOpen: truethat never callsend().- Scope exits →
client[Symbol.dispose]()→endhost fn (sockets.classes.ts:165) →end()at socket_body.rs:3107 setsEND_AFTER_FLUSHand callsinternal_flush(). internal_flush()seescan_end_after_flush()true, clears the flag, callsself.socket.get().shutdown()— kernel sends FIN (SHUT_WR). Returns without callingmark_inactive().IS_ACTIVEis still set,poll_refis still reffed, the Strongthis_valueis still upgraded.- Server has
allowHalfOpen: trueand never sends FIN back → uSockets never seeseofon the client fd → theeof && is_shut_downbranch in loop.c never fires →on_closenever runs. - The socket, its fd, and its event-loop ref survive the
usingblock indefinitely. Before this PR, step 3 calledmark_inactive()and the resource was gone regardless of what the peer did.
Sibling-caller precedent already in this PR
The PR description explicitly calls out the analogous fix in
src/js/node/net.ts:The server
onconnectiondrop path (blockList/maxConnections) switches fromclientHandle.end()toclientHandle.close()since it wants an immediate close, matching Node'sclientHandle.close().@@disposehas the identical requirement — immediate, unconditional release — and is another caller of the changedend()helper. Per REVIEW.md: "Fix the whole class in the same PR — grep for every sibling site sharing the pattern … every caller of a changed helper." Note also that the third@@disposein the same file (line 358, the UDP socket) already maps toclose.Fix
One line in
src/runtime/socket/sockets.classes.ts:"@@dispose": { fn: "close", // was "end" length: 0, },
This keeps the new half-close semantics for explicit
socket.end()calls while restoring the unconditional-release guarantee forusing. - A peer with
end() is now a write-side half-close, so dispose must call close() for unconditional release on scope exit (same reasoning as the net.ts drop path). Also fix the shutdown() JSDoc: after SHUT_WR the peer's FIN is delivered as close, not end.
There was a problem hiding this comment.
Beyond the two doc nits inline, the main correctness question — whether the new end() → shutdown(SHUT_WR) path leaks the fd/poll_ref when the peer's FIN arrived first on an allowHalfOpen socket (uSockets' own TODO at socket.c:706 flags this case) — was examined for both client and server sides and ruled out: the re-armed readable poll re-delivers EOF and loop.c:845 closes it.
Extended reasoning...
The two findings this run are docs-only nits on text this PR added or made stale; they're posted inline. The substantive runtime concern for this change is the peer-FIN-first ordering on half-open sockets, since internal_flush() no longer reaches close_and_detach() directly and instead relies on uSockets' eof && is_shut_down branch to run teardown. That path was traced for both client-initiated and server-accepted sockets and holds. Not approving because this is a semantic change to Socket.end() and @@dispose on a core networking primitive — worth a human look.
close() described itself as a wrapper around end()/shutdown(), which now read as half-close-only. timeout() recommended end() to close on timeout (now a half-close that waits on the peer) and overstated the sweep granularity as a lower bound.
There was a problem hiding this comment.
All three rounds of feedback are addressed and the diff looks clean, but I'm deferring: this changes the user-visible semantics of Socket.end() from an immediate close to a half-close whose fd/poll_ref/Strong stay live until the peer FINs, drops the shutdown(true) → SHUT_RD path entirely, and rebinds @@dispose. That's a Bun-native API contract change a maintainer should sign off on.
What was reviewed:
internal_flushend-after-flush arm and its other callers (drain, write,flush()) — the flag is cleared beforeshutdown(), so repeat calls are idempotent and the deletedflush()guard comment's scenario no longer applies.node:net_finalalready usedshutdown()directly, so Node-compat half-close is unchanged; the twoonconnectiondrop sites correctly wantclose().- JSDoc for
shutdown()/timeout()/close()/@@disposenow matches the runtime paths after c3fe786.
Extended reasoning...
Overview
The PR retargets Bun.connect/Bun.listen Socket.end() and Socket.shutdown() to be true write-side half-closes (SHUT_WR) instead of the previous close(2) / SHUT_RD behavior, updates bun.d.ts JSDoc for end/shutdown/timeout/close/@@dispose to match, rebinds [Symbol.dispose] from end to close, swaps two clientHandle.end() calls in node:net's onconnection drop path to close(), and adds a 5-cell half-close test matrix. Five files touched; the runtime change is ~10 lines in socket_body.rs.
Security risks
None identified. No untrusted-input parsing, no auth/TLS verification changes; the TLS path's shutdown() already meant SHUT_WR so its wire behavior is unchanged.
Level of scrutiny
High — this is a behavior change to a public Bun-native API. Previously end() reached mark_inactive() → close_and_detach(Normal), releasing the fd, unreffing the poll keepalive, and dropping the Strong this_value immediately. Now it issues SHUT_WR and defers all of that to on_close, which only fires when the peer's FIN arrives. On a hung or vanished peer that can be much later (or effectively never without TCP keepalive). The PR compensates for the two in-tree callers that wanted an immediate release (@@dispose, the net.ts drop path), and node:net _final was already on shutdown(), but third-party code that relied on end() fully closing will see a lifetime change. Dropping shutdown(true) → SHUT_RD and making halfClose a documented no-op is also an API-surface decision. These are reasonable calls that align with the docs and Node semantics, but they're the kind of contract change a maintainer should approve.
Other factors
All prior review threads (CodeRabbit's end(data) matrix, comment-cop's three long-comment flags, and my three JSDoc-accuracy nits) are resolved in 490f009/ae6d4fc/79a9da2/c3fe786, and the current diff reflects each fix. The bug-hunting pass on c3fe786 found nothing. The new test awaits real events (server end handler, client close), asserts the exact {received, writeAfter, readyState} triple, uses it.concurrent, and per robobun fails on released bun / passes on the branch. CI build 82515's failures are unrelated per robobun's triage. I checked the other internal_flush() call sites (on_writable/drain, post-write, JS flush()): clearing END_AFTER_FLUSH before shutdown() prevents a second SHUT_WR on a later drain, and on_close still runs mark_inactive() so the teardown ordering is preserved.
|
Additional evidence for this change: the Setup: a Plain TCP, bun 1.4.0-canary, Linux, 3/3: TLS ( In every lossy run bun's Why this happens: This PR fixes it by construction: Repro, plain TCP: import { spawn } from "node:child_process";
const TOTAL = 8 << 20, CH = new Uint8Array(65536).fill(97); let off = 0;
const pump = s => {
while (off < TOTAL) {
const want = Math.min(65536, TOTAL - off);
const n = s.write(CH.subarray(0, want));
if (n < 0) return;
off += n;
if (n < want) return;
}
if (!s.ended) { s.ended = 1; console.log("server: all", off, "accepted; end()"); s.end(); }
};
const server = Bun.listen({ hostname: "127.0.0.1", port: 0, socket: {
open: pump, drain: pump, data() {},
close() { console.log("server: close (clean)"); },
error(s, e) { console.log("server: error", String(e)); } } });
const py = `import socket,time
s=socket.create_connection(("127.0.0.1",${server.port})); n=0; last=time.time(); err=None; s.settimeout(0.05)
while True:
try:
d=s.recv(65536)
if not d: break
n+=len(d); time.sleep(len(d)/2e6)
except socket.timeout: pass
except OSError as e: err=repr(e); break
if time.time()-last>0.1:
try: s.send(b"p"); last=time.time()
except OSError as e: err=repr(e); break
print("client: received", n, "of", ${TOTAL}, "err=", err)`;
spawn("python3", ["-c", py], { stdio: "inherit" }).on("exit", () => { server.stop(true); process.exit(0); });I closed #35932 (the docs-only approach) in favour of this PR. One note on the |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-26, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
What
Socket.end()andSocket.shutdown()onBun.connect/Bun.listensockets did not match their documented contract:end()close(2), nothing receivedshutdown()shutdown(SHUT_WR)shutdown(SHUT_WR)shutdown(false)shutdown(SHUT_WR)shutdown(SHUT_WR)shutdown(true)shutdown(SHUT_RD), nothing receivedshutdown(SHUT_WR)So an app that follows the docs for a request-then-read-reply protocol loses the peer's entire reply with
end()orshutdown(true).Cause
Socket.prototype.shutdownmappedarg.to_boolean()toshutdown_read()(SHUT_RD) vsshutdown()(SHUT_WR) (socket_body.rs).Socket.prototype.endsetEND_AFTER_FLUSH, andinternal_flush()turned that intomark_inactive()which callsclose_and_detach(Normal)(close(2)), not a half-close.Fix
shutdown()ignores its argument and always issuesSHUT_WR.SHUT_RDhas no safe user-facing use (the peer cannot observe it and uSockets fabricates a localend).internal_flush()replacesmark_inactive()withsocket.shutdown()for the end-after-flush arm. The socket stays readable; when the peer's FIN arrives uSockets' existingeof && is_shut_downbranch inloop.ccloses it andon_closeruns the same teardown that used to run here.node:net's_finalalready calledshutdown()directly, so its half-close behavior is unchanged. The serveronconnectiondrop path (blockList/maxConnections) switches fromclientHandle.end()toclientHandle.close()since it wants an immediate close, matching Node'sclientHandle.close().bun.d.ts: rewrite theshutdown()andtimeout()JSDoc to describe the actual behavior; delete the self-contradictoryend(): void"Close the socket immediately" overload.timeout()is notify-only (the handler must close the socket itself) and rounds up to the 4-secondLIBUS_TIMEOUT_GRANULARITYsweep.Verification
Fail-before (released bun):
Pass-after (
bun bd test):test/js/bun/net/,test/js/node/net/,test/js/node/tls/,test/js/node/http/,test/js/node/http2/all matchmain(pre-existing local failures only).test-net-server-drop-connections.js,test-net-server-max-connections.js,test-net-server-blocklist.js,test-net-server-max-connections-close-makes-more-available.jspass. bun-types integration test passes.Supersedes #35932 (docs-only approach).
no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/socket.test.ts