usockets: keep READABLE off a half-open socket whose on_end already fired - #35939
usockets: keep READABLE off a half-open socket whose on_end already fired#35939robobun wants to merge 5 commits into
Conversation
With allowHalfOpen, the eof branch drops READABLE and dispatches on_end. If that handler's socket.write() is only partially accepted, the backpressure re-arm (us_internal_rearm_writable) put READABLE back, so the next epoll tick re-derived recv()==0 -> eof and re-fired on_end, forever. Track the "peer FIN delivered" state and have the re-arm / resume / shutdown paths respect it.
|
Updated 2:04 PM PT - Jul 26th, 2026
❌ @robobun, your commit 30274ea has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35939That installs a local version of the PR into your bun-35939 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
WalkthroughChangesThe socket layer adds Half-open EOF handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… test - us_socket_resume: the is_shut_down arm keeps READABLE regardless of readable_ended, mirroring raw_shutdown (loop.c checks is_shut_down before readable_ended, so re-deriving eof there is what closes us on kqueue). - loop.c: reword the readable_ended guard comment to name the triggers that actually bypass the rearm_writable/resume guards. - test: clamp SO_SNDBUF via setSocketOptions so the write from end() is a partial write on every kernel, and assert drainCount >= 1.
setSocketOptions is a POSIX-only no-op on Windows, and Windows loopback auto-tuning accepts 256 KiB in one send(). The primary assertion (endCount == 1 with the full payload delivered) already proves the fix there.
There was a problem hiding this comment.
Both prior findings are addressed — the is_shut_down arm of us_socket_resume keeps READABLE unconditionally again, and the drainCount assertion is gated on !isWindows with the stale comment fixed. I didn't find anything further, but this touches the core socket poll-mask / eof dispatch path across all three backends (epoll/kqueue/libuv), so it's worth a human look.
What was reviewed:
readable_endedis explicitly zeroed at all five socket-init sites (listen, connect, start_connections, accept, from_fd);us_create_pollusesus_mallocnot calloc, so this is load-bearing.- The loop.c
is_shut_downcheck runs before the newreadable_endedguard, so re-arming READABLE inraw_shutdown/resume's shut-down arm still reachesclose_raw, not the re-dispatch guard. - The new bitfield sits in the existing pad-to-pointer gap alongside the
ssl_*bits — struct size unchanged.
Extended reasoning...
Overview
Adds a readable_ended bit to us_socket_t that latches the first time the half-open eof branch dispatches on_end, then consults it at the three sites that would otherwise re-add LIBUS_SOCKET_READABLE to the poll mask (us_internal_rearm_writable, us_socket_resume, and the loop.c eof re-entry). us_internal_socket_raw_shutdown gains a compensating branch that explicitly arms READABLE when readable_ended is set, so kqueue/libuv still wake the SHUT_DOWN close path (epoll got this via unmaskable EPOLLHUP). A regression test in tcp-server.test.ts reproduces the infinite end() re-dispatch on Linux by clamping SO_SNDBUF and issuing a partial write from the end handler.
Security risks
None identified. This is event-loop poll-mask bookkeeping on already-authenticated local sockets; no untrusted input parsing, no auth/crypto surface.
Level of scrutiny
High. loop.c's eof handling and us_poll_change mask decisions run on every TCP connection in the runtime, and the semantics differ across epoll (level-triggered, unmaskable EPOLLHUP), kqueue (one-shot EVFILT_WRITE, per-filter EV_EOF), and libuv/AFD (UV_DISCONNECT). A mistake here manifests as a hang or busy-spin that only reproduces on one platform. That's beyond the "simple/mechanical" bar for auto-approval.
Other factors
- Two earlier review rounds surfaced (1) an internal inconsistency where
us_socket_resume'sis_shut_downarm would set the poll to 0 on kqueue, and (2) a Windows test flake from assertingdrainCountwheresetSocketOptionsis a no-op. Both were fixed (f158cfe, 9a53f43, 7ce958e) and re-verified in this pass. - I confirmed
us_create_pollusesus_malloc(not calloc), so the explicitreadable_ended = 0at each init site is required; all five creation paths are covered. - The PR overlaps with #34487 (Windows UV_DISCONNECT trigger of the same re-dispatch) — a maintainer should decide how the two land relative to each other.
- The new test wires
closeon both sides to resolve the awaited promises and terminates on the secondend()re-entry, so the pre-fix failure mode is a clean assertion rather than a hang.
There was a problem hiding this comment.
Both of my earlier concerns are addressed (f158cfe keeps READABLE in us_socket_resume's shut-down arm to mirror raw_shutdown; 9a53f43/7ce958e gate the drainCount assertion on !isWindows and fix the stale 4 MiB comment). No new issues found.
I'm not auto-approving because this is core usockets poll-rearm / eof-dispatch C code with backend-specific semantics (the raw_shutdown change specifically compensates for kqueue/libuv lacking epoll's unmaskable EPOLLHUP), and it overlaps mechanically with open #34487 — a maintainer should decide how the two land.
What was reviewed:
readable_endedinitialized at everyus_socket_tconstruction site (listen/connect/accept/from_fd/start_connections);us_socket_adoptcarries it viaus_poll_resizememcpy, which is correct.- loop.c ordering:
is_shut_downcheck precedesreadable_ended, so a shut-down socket still reachesclose_rawwhen eof is re-derived — consistent with theraw_shutdown/resumeREADABLE-on-shut-down invariant. - Test wires
closeon both sides to resolve, terminates on the secondend()so the buggy build fails on the assertion instead of hanging, and the SO_SNDBUF clamp forces a partial write on POSIX.
Extended reasoning...
Overview
Adds a readable_ended bit to us_socket_t (in the existing pad-to-pointer bitfield gap; struct size unchanged) that latches the first half-open on_end dispatch. Four call sites now consult it: us_internal_rearm_writable and the non-shut-down arm of us_socket_resume skip re-adding READABLE; the loop.c eof branch drops READABLE and skips re-dispatch if already set; us_internal_socket_raw_shutdown explicitly arms READABLE when readable_ended so kqueue/libuv still wake the SHUT_DOWN close path. The bit is zero-initialized at all five socket construction sites. A new tcp-server.test.ts case reproduces the infinite end() re-dispatch on POSIX by clamping SO_SNDBUF and writing 256 KiB from the end handler.
Security risks
None. This is event-loop poll-mask bookkeeping on an already-authenticated TCP fd; no untrusted-input parsing, auth, or crypto is touched. The failure mode being fixed is a CPU spin (see linked #32600), and the fix only narrows when READABLE is armed.
Level of scrutiny
High. us_internal_dispatch_ready_poll and the poll-rearm helpers are on the hot path of every TCP/TLS connection, and the correctness of the raw_shutdown change depends on backend-specific poll semantics (unmaskable EPOLLHUP vs. kqueue EV_DELETE vs. libuv's UV_DISCONNECT OR-in). The change is small and well-reasoned, but a mistake here hangs or spins sockets on one platform only. The test's platform-gated drainCount assertion and the PR footer's "deferring to CI" both underline that this needs cross-platform CI green before merge.
Other factors
- I left two prior rounds of inline feedback; both were addressed exactly as suggested and the threads are resolved.
- github-actions flagged this as mechanically overlapping with open #34487 (same
readable_endedbit, same guarded call sites, Windows vs. Linux trigger). A human should decide whether to merge one, both, or fold them together. - CI build #82509 was still in progress at the time of the last robobun update; cross-platform pass (especially macOS/kqueue and Windows) is the remaining gate.
Dropping READABLE from rearm_writable when readable_ended truncated Windows/TLS half-close drains (serve.test.ts and node-http-backpressure https cases): libuv.c poll_cb's UV_DISCONNECT handling relies on READABLE being registered to keep the writable dispatch flowing after the eof branch drops it. loop.c's readable_ended guard already absorbs the 0-byte read without re-dispatching on_end, so rearm_writable can keep its original behaviour. Verified on windows-x64: node-http-backpressure.test.ts 8/8, serve.test.ts half-close 5/5, tcp-server.test.ts allowHalfOpen pass.
There was a problem hiding this comment.
Both earlier findings are addressed and I didn't spot new issues in 30274ea, but this is core usockets poll-mask handling across epoll/kqueue/libuv with an overlapping open PR (#34487) touching the same mechanism — worth a maintainer's eyes to reconcile the two and sanity-check the cross-backend behavior.
What was reviewed:
readable_endedinit at all four socket-creation sites (accept, connect, from_fd, listen) — all covered.us_socket_resume's shut-down arm keeps READABLE unconditionally (matchesraw_shutdown; the earlier kqueue hole is closed).- 30274ea's revert of
rearm_writableto keep READABLE, relying on loop.c's newreadable_endedguard to absorb the 0-byte read — traced that the guard runs before theallow_half_openre-dispatch, and theis_shut_downcheck precedes it so the SHUT_DOWN close path is unaffected. - Test:
drainCountgated on!isWindows, stale 4 MiB comment fixed, both close events wired to resolve.
Extended reasoning...
Overview
Adds a readable_ended : 1 bit to us_socket_t (in the existing pad-to-pointer bitfield gap) and uses it to prevent the half-open eof branch in loop.c from re-dispatching on_end on every tick when a partial write inside the end handler re-arms READABLE. Touches internal.h (struct field), context.c + loop.c + socket.c (init + guards at the eof branch, rearm_writable comment, resume, raw_shutdown), plus a new regression test in tcp-server.test.ts.
Security risks
None identified — this is event-loop poll-mask bookkeeping on already-open sockets; no untrusted input parsing, no auth/crypto surface.
Level of scrutiny
High. packages/bun-usockets/ is the lowest layer of Bun's networking stack and every change here fans out across three backends (epoll, kqueue, libuv/Windows) whose edge/level-trigger and HUP semantics differ. The PR itself went through a design iteration (30274ea reverted an earlier per-caller READABLE guard in rearm_writable in favor of a central absorb in loop.c), and there is an open sibling PR #34487 implementing the same readable_ended mechanism for the Windows UV_DISCONNECT trigger — a maintainer should decide how the two land relative to each other.
Other factors
- Both of my earlier inline findings (the
resumeshut-down arm setting poll=0 on kqueue, and the WindowsdrainCountflake / stale comment) were addressed in f158cfe / 9a53f43 / 7ce958e and are resolved. - The new test wires
error/close paths to promise resolvers, usesport: 0, clamps SO_SNDBUF viabun:internal-for-testing, and gates the platform-dependentdrainCountassertion on!isWindows. - The
raw_shutdownchange (arming READABLE whenreadable_ended) is a behavioral change on kqueue/libuv for the "peer FIN then our shutdown()" sequence — the reasoning in the comment is sound, but this is exactly the kind of cross-backend subtlety a maintainer should confirm against the macOS/Windows CI lanes.
|
CI on 30274ea: 193/196 jobs passed. The only hard failure is Ready for review. |
…ackpressure pauses from holding the loop (#33974) ### Problem - A `net.Socket` that has already sent its FIN (`end()`) and then pauses under backpressure loses the tail of the peer's stream. The eof-while-paused deferral on main (#32488) exempts shut-down sockets, so the `is_shut_down` arm in `us_internal_dispatch_ready_poll` (`packages/bun-usockets/src/loop.c`) closes the socket while bytes are still queued in the kernel: kqueue reports `EV_EOF` on the readable event carrying the final data, epoll latches `EPOLLHUP` once both directions are down. Deterministic on Linux with a 1 MiB reply (`end 586752` of 1048576 on main); about a third of runs on macOS. - The exemption's original reason was uws HTTP, but every uws HTTP socket is `allow_half_open` and pairs `shutdown()` with `close()`, so it never reaches this arm; what the exemption (and the earlier `allow_half_open` version of this PR) actually left truncating were `Bun.connect`/`Bun.listen` without `allowHalfOpen` and `us_socket_from_fd` sockets. - Deferring the FIN exposes a loop-hold bug in `src/js/node/net.ts`: the three `push()`-returned-false pause sites and the two onread-mode ones (callback returned `false`) pause the native handle but, unlike `Socket.prototype.pause`, keep the process alive. With a deferred FIN, a program that never reads a reply or a request sits forever; node (whose `readStop`'d handles are inactive) exits. The not-shut-down deferral already on main has this hang today (`net.connect()` to a server that writes and goes away, never read: hangs on main, exits in node). ### Fix - `loop.c`: defer the eof hint whenever the socket is paused and `read_eof` is not set (a delivered FIN means nothing is left to drain, so the close stays prompt); shut-down sockets included. The existing epoll `us_poll_stop` applies to every deferral, so an AF_UNIX peer close (`EPOLLHUP` on a socket we did not shut down) cannot spin either. A second arm covers the batch race on epoll: an entry carrying `EPOLLHUP` collected while paused still dispatches after an earlier entry in the same batch resumed the socket, with no `READABLE` bit; it is left for the next poll, which re-reports it with `READABLE` and lets the read loop drain to `recv()==0`. - `us_poll_change` returns the result of re-registering a parked fd (it goes through `us_poll_start_rc`, so `EPOLL_CTL_ADD` failure is handled and fault-injectable); `us_socket_resume` closes the socket with that errno instead of leaving it registered nowhere. - `net.ts`: all five stop-reading sites drop the hold the way `Socket.prototype.pause` does (`kPausedUnref`; `read()`/`_read()`/`resume()` and the onread tail drain restore it), except while a write is waiting for drain, which re-refs in `_write` and is released by the drain handlers. `kPausedUnref` is cleared on every resume path and on `ref()`, so a drain never gives up a hold the user re-took. - Why this is right: a paused socket not observing the peer's FIN until it resumes, and not keeping the process alive while it is not reading, are both node's behavior (readStop); every fixture below was checked against node v26 and matches it. - Windows is affected too: `libuv.c` maps AFD `DISCONNECT` on a shut-down socket to the same eof hint, so it takes the new arm; the truncation tests now run there (they pass on current main there only because AFD's single forced `recv()` happens to cover the remainder at these sizes, and they exercise the resume re-report path on the branch). ### Tests `test/js/node/net/node-net.test.ts` (red on main, green with the change, unless noted): - delivers every buffered byte before `'end'` when the data handler pauses (main: truncated) - stays parked without spinning the loop, then delivers the tail on resume (main: `close 0`; also bounds the CPU of the parked second) - client that never reads the reply and whose peer goes away exits (main: hangs) - onread client whose callback returns `false`, with and without `end()` (both hang on main here: the bare onread pause held the loop whether or not the FIN made it through the buffers) - client that `end()`s and never reads / server that `end()`s without reading the request exit (pass on main because main closes them; hang with the `loop.c` change alone, which is the regression the `net.ts` part prevents) - a write still waiting for drain keeps the process alive until it completes; a drain does not give up a hold re-taken with `ref()` (both fail with the naive versions of the `net.ts` change) - its named-pipe leak check is now awaited with a warmed baseline: left dangling, its assertion landed inside whichever test was running ~1s later and counted that test's sockets (this is what an earlier CI run hit once these tests shifted the timing); the pipe transport keeps exactly one wrapper reachable regardless of connection count, on main as well, which is what the warm-up absorbs. `test/js/bun/net/socket-syscall-fault.test.ts`: resume of a parked socket whose re-registration fails surfaces `read ENOMEM` + `close`, instead of a deaf socket (epoll only; main closes the socket before the resume, so it is red there too). Verified: full `node-net.test.ts`, `socket.test.ts`, `tcp-server.test.ts`, `socket-syscall-fault.test.ts`, `fetch-backpressure.test.ts`, `node-http.test.ts`, `node-tls-server.test.ts`, `child_process.test.ts` have failure sets identical to main in this environment (the shared failures are no-network / `localhost` dual-stack ones); the 327 `test-net-*`/`test-tls-*` node tests show the same two pre-existing failures as main. On Windows x64 (debug build): `node-net.test.ts` 77 pass / 0 fail, `node-http.test.ts`, `fetch-backpressure.test.ts`, `socket-syscall-fault.test.ts` clean, the pause-related `test-net-*`/`test-http-pause*` node tests pass. (`net.Socket write > should allow reconnecting after end()` is flaky on Windows debug builds on main as well: 1/12 there vs 4/24 here, a pre-existing 3ms race in that test, unrelated to this change.) ### Background - eof hint: the `eof` argument to the dispatcher. Where it comes from differs per backend (kqueue `EV_EOF`, epoll `EPOLLHUP`, AFD `DISCONNECT`, or `recv()` returning 0 inside the read loop); only the last one proves the buffer is empty. - parked socket: on epoll `EPOLLHUP` is level-triggered and cannot be masked, so a paused socket that hung up is removed from the epoll set (`us_poll_stop`) and added back on resume; that re-add is a fresh registration. - `read_eof`: set once the peer's FIN has been delivered as `on_end` on a half-open socket. - onread mode: the `onread` connect option delivers reads into a caller-supplied buffer through a callback instead of the stream's `push()`; returning `false` from the callback is that mode's way of stopping reads. - `kPausedUnref` / `kUserUnrefed`: `net.ts` bookkeeping for the handle's hold on the event loop. `kUserUnrefed` records an explicit `socket.unref()`; `kPausedUnref` records that a pause dropped the hold on the user's behalf and the next resume should restore it. Related: #35939 fixes a different problem in the same block (`on_end` re-dispatch after a partial write); the two are independent.
2 participants
What does this PR do?
Problem
With
Bun.listen({ allowHalfOpen: true }), if the server'sendhandler does asocket.write()that the kernel only partially accepts,on_endis re-dispatched on every loop iteration forever.drainfires at most once between re-entries, so the payload never finishes flushing.Cause
The half-open eof branch at
loop.csets the poll toWRITABLEonly, then dispatcheson_end. A partial write inside that handler callsus_internal_rearm_writable, which re-adds READABLE. On the next tick epoll reports the fd readable again (the peer's FIN is still there),recv()returns 0, eof is re-derived, and the half-open branch re-runs:on_endis re-dispatched, and the handler's partial write re-adds READABLE again.Fix
Add a
readable_endedbit tous_socket_t(in the existing bitfield gap; struct size unchanged) and set it the first time the half-open eof branch dispatcheson_end. Then:loop.ceof branch: ifreadable_endedis already set, drop READABLE and skip the re-dispatch instead of re-firingon_end. A backpressured write'srearm_writablestill re-adds READABLE (the Windows/TLS half-close drain inlibuv.c'sUV_DISCONNECThandling relies on it being registered to keep the writable dispatch flowing), so this guard is what absorbs the 0-byte read.us_internal_socket_raw_shutdown: when the peer FIN was already delivered, the poll can sit at WRITABLE-only (or 0), soevents & READABLEis 0 and on kqueue/libuv nothing would wake the SHUT_DOWN close path. Arm READABLE explicitly so the next tick reads 0 bytes and closes via the existingis_shut_downbranch. epoll got this for free via unmaskable EPOLLHUP; this makes the other backends match.us_socket_resume: skip READABLE in the still-writable arm whenreadable_ended(the loop.c guard would catch it anyway; this saves one 0-byte read). Theis_shut_downarm keeps READABLE unconditionally, same asraw_shutdown.The bit is zero-initialized at every socket construction site (
us_create_pollusesus_malloc, not calloc).#34487 introduces the same
readable_endedbit for the WindowsUV_DISCONNECTlevel-triggered trigger of the same re-dispatch. This PR's loop.c guard covers both triggers and adds a Linux-reproducible test; the field placement is identical so whichever lands first, the other rebases cleanly.How did you verify your code works?
New test in
test/js/bun/net/tcp-server.test.tsclampsSO_SNDBUF(POSIX) so the write fromend()is a partial write on every kernel, and assertsendCount == 1with the full payload delivered.Linux gate proof:
Verified on windows-x64 at 30274ea:
node-http-backpressure.test.ts"client FIN right after" 8/8,serve.test.ts"client half-close after the request" 5/5,tcp-server.test.tsallowHalfOpen pass.test/js/node/net/node-net-allowHalfOpen.test.js, thetest-net-*half*/test-net-*end*node parallel tests,node-net.test.tsandsocket.test.tsall have the same pass/fail as main.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/tcp-server.test.ts