Repository navigation
Conversation
|
Updated 7:39 AM PT - Oct 17th, 2026
✅ @robobun, your commit b0794344183dae338453071a784dbe9ef06ddf95 passed in 🧪 To try this PR locally: bunx bun-pr 45006That installs a local version of the PR into your bun-45006 --bun |
|
Status Reproduced on main from the kernel state of the accepted connection, read with PR: #45006 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughHTTP servers now accept keep-alive options and apply them to eligible connections. Socket keep-alive operations use native handles. Socket transport types also govern ChangesHTTP server keep-alive
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: enabling setKeepAlive() and the keepAlive option on node:http server sockets. |
| Description check | |
The description explains the problem, implementation, scope limits, behavior differences, and verification results. It does not use the template headings exactly, but it provides the required informat… |
Full details: Description check
-
Explanation
The description explains the problem, implementation, scope limits, behavior differences, and verification results. It does not use the template headings exactly, but it provides the required information and is substantially complete.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the new setKeepAlive host function in JSNodeHTTPServerSocketPrototype.cpp for the usual coercion hazards: toNumber is followed by RETURN_IF_EXCEPTION and this is re-downcast and checked for isClosed() after coercion, so a valueOf that destroys the socket cannot reach us_socket_keepalive with a stale pointer, and the ms-to-seconds conversion handles NaN, negatives and values above UINT_MAX before the cast. The net.Socket.prototype.setKeepAlive fallback to kSocketOptionHandle keeps the existing dedupe state and the _handle-less early-return path unchanged for ordinary net sockets.
Extended reasoning...
The change wires TCP keep-alive through to node:http/https server sockets across JS (_http_server.ts, net.ts, symbols.ts), one new C++ host function, docs, and a new ~800-line test file; it touches no auth, crypto or injection surface, only socket options. Inline findings already flag the Proxy/resetAndDestroy regression, the user-overridable prototype call on the plain-TCP path, and test-convention issues, so a human look is already signalled; this note only records the C++ coercion and net.ts fallback checks that were examined and found sound.
c8cedf4 to
b44a8fe
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
b44a8fe to
939c80b
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Thanks for the follow-up push: the four points from the earlier round (direct handle call instead of the public prototype method, bun:test with static imports, /proc/self/fd enumeration in fdOf, and the narrowed Proxy comment) are all addressed in the new commits. Beyond the inline nit, I also checked the new native setKeepAlive host function: the extern declaration matches us_socket_keepalive in packages/bun-usockets/src/libusockets.h, toBoolean cannot enter JS so the single RETURN_IF_EXCEPTION after toNumber covers both coercions and the liveness re-check after it is sound, and the NaN/negative/overflow handling happens in double before the unsigned cast.
Extended reasoning...
The PR adds a native setKeepAlive on the node:http server socket handle, routes net.Socket#setKeepAlive to it via a symbol getter, and makes http.Server read keepAlive/keepAliveInitialDelay per accepted connection, plus a 600-line kernel-state test suite using bun:ffi getsockopt. No security-sensitive surface beyond a new host function whose argument coercion and liveness checks I verified. The remaining inline finding is a style nit, but the change spans C++ bindings and Node-compat semantics with a deliberate design choice (JS-side accept hook vs uSockets listener field), so a maintainer should still weigh it.
setKeepAlive() of a node:http server socket (req.socket, and the socket of 'connection', 'connect' and 'upgrade') was a stub that returned the socket. The native handle of the connection now has setKeepAlive(enable, ms). The socket class no longer overrides the method: net.Socket#setKeepAlive() finds the handle through an internal getter, so the call that changes nothing is skipped as for any net.Socket. A socket of a unix listener gives no handle: it has no keep-alive, as in Node.js.
…t accepts http.Server keeps keepAlive and keepAliveInitialDelay as own data properties, with the check of Node's net.Server. The accept callback reads them for each connection and sets keep-alive on the native handle, as net.Server's onconnection does. A plain http socket starts its setKeepAlive() state from them. An https socket starts with no state, as Node's TLSSocket.
939c80b to
b079434
Compare
There was a problem hiding this comment.
I reviewed the two new commits and found no bugs; the points raised on the earlier revision (server option routed through the public prototype method, conditional kNoKeepAlive field, the Proxy claim on resetAndDestroy(), the 0..1023 fd scan, and the node:test/dynamic-import test shape) are all addressed in this version. Because it adds a native host function and changes the shared net.Socket.prototype.setKeepAlive path, a human look is still worthwhile.
What was reviewed:
jsFunctionNodeHTTPServerSocketSetKeepAlive:toNumberis followed byRETURN_IF_EXCEPTIONandthisis re-downcast andisClosed()-checked after the coercion; NaN/negative/Infinity are clamped in double before the cast to unsigned, andus_socket_keepaliveitself skips a shut-down socket.- All three
NodeHTTPServerSocketcreation sites (kRealListenlazy path,onServerConnection,onServerClientError) callsetKeepAliveOfServer; the unix-listener case clearshandle.setKeepAliveso thenet.tsguard early-returns;net.Socket#setKeepAliveonly consultskSocketOptionHandlewhen_handleis absent, so plainnet.Sockets are unaffected. - Option parsing matches Node's
net.Server(validateNumber, clamp <0,~~(ms/1000)stored in seconds); the ms-vs-seconds conversion back to the handle is consistent with thekSetKeepAliveInitialDelaycache thatnet.tscompares in ms. - Tests use
bun:testwith static imports,port: 0, resolve fds via/proc/self/fdor/dev/fd, drain the Node subprocess pipes concurrently, and skip the kernel-option cases on Windows.
Extended reasoning...
The change adds a DontEnum setKeepAlive host function on the node:http server socket handle (src/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpp), moves two keep-alive symbols into internal/net/symbols.ts, lets net.Socket.prototype.setKeepAlive fall back to a symbol-keyed handle getter, and makes http.Server parse and apply keepAlive/keepAliveInitialDelay at every socket-creation site, plus a docs line and ~830 lines of tests. It touches no auth, crypto, or injection surface; the kernel option is set via the existing us_socket_keepalive helper. No CODEOWNERS entry covers the changed paths and the earlier inline nits are addressed in the code. Deferred rather than approved because it introduces a new native binding and alters a shared net.Socket prototype method used by every socket type, which is beyond a mechanical change.
Problem
socket.setKeepAlive()does nothing on the sockets of anode:httpserver, andcreateServer({ keepAlive })is ignored.SO_KEEPALIVEstays 0, so a peer that vanishes under an open response or a tunnel is never closed.setKeepAlive()ofNodeHTTPServerSocket(src/js/node/_http_server.ts:2145on main). The server never reads the option.Fix
setKeepAlive(). The stub is gone:net.Socket#setKeepAlive()finds that handle through a symbol getter and keeps its dedup.http.ServerkeepskeepAliveandkeepAliveInitialDelayas data properties.onServerConnectionreads them for each connection, as Node'snet.Serverdoes at accept.test/js/node/http/node-http-server-keepalive.test.ts(35 of 46 tests fail on main). Also thenode:http,wsandnode:netsuites in Notes.Background
net.Socketsubclass whose I/O goes through a private native handle, not_handle.Downsides
createServer({ keepAliveInitialDelay: "5" })now throwsERR_INVALID_ARG_TYPE, as in Node.keepAlive: true, +1 host call. Binary: +2,147 B text.noDelay: falseandsetNoDelay(false)have no effect.Notes
How it was found. A comparison of kernel state with Node (
getsockopt(2)on the accepted fd). No user report exists for it.Dead peer. Linux, root, inside
unshare -n. The client reads the first bytes, thenip link set lo downdrops every packet. Seconds until the server socket closes:keepAlive: true, keepAliveInitialDelay: 1000req.socket.setKeepAlive(true, 1000)keepAlive: true, keepAliveInitialDelay: 1000The time is the delay plus 10 probes of 1 s. A caller that gives no delay keeps the idle time of the kernel (7200 s on stock Linux).
Callers.
@nestjs/corecallsreq.socket.setKeepAlive(true)for each SSE stream (packages/core/router/sse-stream.ts:122).http-proxycallssocket.setKeepAlive(true, 0)on the upgrade socket (lib/http-proxy/common.js:129, frompasses/ws-incoming.js:98).The two commits. The first makes
socket.setKeepAlive()act and can land alone. The second adds the server option.Unix sockets and Proxy. The handle of a unix socket gets no
setKeepAlive(), as Node's Pipe handle has none, so the socket class has no new field.setKeepAlive()also acts through aProxyof the socket, as in Node: the getter reads no private field.resetAndDestroy()is not changed.Why the option is read in JS. Node's
http.Serverhas no keep-alive code. It gives the options tonet.Server, which reads two data properties in its accept callback (lib/net.js#L2361-L2365at v26.3.0).onServerConnectionis that callback here, and it already appliesmaxConnectionsandblockListthe same way. An earlier shape of this PR set the option in the uSockets accept loop through a new listener field. Against Node it differed in three ways (accessor properties, a class field, andObject.defineProperty()afterlisten()), and it needed listener state thatbun --hotcarries to the next server. The JS read differs in one way: on https it runs when the handshake completes. A handshake that never completes is closed after 12 s on main and on this branch, with or without keep-alive, by the timer for a connection that has no request yet. If that timer goes away, the listener field is the way to set keep-alive earlier.Measurements. Release builds, linux x64, main ff8ad61 against this branch, Node v26.3.0.
valgrind,perf,straceandbloatyare not installed here, so the counts come from anLD_PRELOADshim forsetsockopt, gdb breakpoints,nm,sizeandBUN_JSC_dumpGeneratedBytecodes.setsockoptsequence, 23 scenarios: 14 rows equal to Node (main: 6). Of the 13 keep-alive rows 11 are equal (main: 3). In the other 2,setKeepAlive(true, -5000)andsetKeepAlive(true, 1e12), Node issues one moreTCP_KEEPIDLEthat the kernel rejects. The kernel state is the same.noDelayrows.server.keepAliveInitialDelay, 16 rows: 15 equal to Node. The other istrue(Node sets 1 s, this PR keeps the time of the kernel).keepAlive: true, http or https: 1 (the handle'ssetKeepAlive()).onServerConnection, 3 in the socket constructor). No allocation.req.socket: 265 B shallow and 32 own symbols, before and after.http.Server: +2 own properties.net.Socket#setKeepAlive(): +2 executed ops per call on a connected socket.Behaviour changes for the release notes.
keepAliveandkeepAliveInitialDelayofhttp.createServer()andhttps.createServer()act.socket.setKeepAlive()acts on a server socket.keepAliveInitialDelaythat is not a number throwsERR_INVALID_ARG_TYPE.server.keepAliveandserver.keepAliveInitialDelayexist (the delay in seconds, as in Node).Differences from Node that stay.
docs/runtime/nodejs-compat.mdxsays so.req.socketis. In Node it is the rawnet.Socket. SosetKeepAlive(false)in a 'connection' listener of an https server withkeepAlive: truedoes nothing here, as it does on Node'sTLSSocket.net.Socketcompares the delay in milliseconds, Node in seconds.setKeepAlive(true, 30500)aftersetKeepAlive(true, 30000)sets the same 30 s again.Not in this PR.
noDelay: falseandsetNoDelay(false). Bun sends a response in two writes. With Nagle's algorithm on, an earlier draft of this change took 52 ms (p50) for a keep-alive response, against 0.7 ms on main and 0.9 ms on Node. The writer has to send one segment first. node:net: default to Nagle on (TCP_NODELAY=0) and make setNoDelay(false) reach the kernel #33724 has the same question fornode:net.setTypeOfService()andgetTypeOfService()on a server socket (node:net: honor typeOfService, keep the type of service for unix and TLS-over-stream sockets #42472 is thenode:nethalf).setKeepAlive()aftersocket.end():us_socket_keepaliveskips a socket that sent its FIN. Node still sets the option there.setKeepAlive()through atls.TLSSocketthat wraps another socket. The wrap does not forward the call, for a 'connect' socket of this server or for any other socket.net.Serverandtls.Serverinsrc/js/node/net.ts. They differ from Node in four ways that this PR does not touch:keepAliveInitialDelayreads back in milliseconds, sockets oftls.Serverstart with keep-alive state, a unix listener setsSO_KEEPALIVE, and a bad assigned delay throws at accept.highWaterMarkthat follows in Node'snet.Server(fix(http): honor HTTPS server half-open policy #42855 edits the same lines).ref()andunref()(node:http: a tunnel holds the event loop only while it reads or has bytes left to send #44885),bytesRead(Fix req.socket.bytesRead returning 0 in node:http server #28710),bytesWritten(counts the body only),address()(gives the remote address), the fd (Expose socket fd for Bun.serve and node:http server sockets #29599),req.socketas atls.TLSSocket(node:https: make req.socket a tls.TLSSocket with getPeerCertificate() #37255).installSocketStubs()and applies them toNodeHTTPServerSocket. If it keeps thesetKeepAlivestub there, it undoes this change, and the new tests fail.Suites run on the debug build of this branch.
node-http.test.ts(292 pass),node-http-connect,node-http-with-ws,node-http-upgrade-body,node-https-server,node-http-maxHeadersCount,node-http-server-timeouts,node-http-incoming-socket,node-http-req-socket-pause,node-http-server-socket-end-drain,node-http-server-close-drain,node-http-server-abort-events,node-http-agent-free-socket,first_party/ws(3 files),node-net-server,node-net-allowHalfOpen, the--hottest ofserve.test.ts, and 21 upstreamtest/js/node/test/parallelfiles for keep-alive, nodelay and server options.Tests. The tests that need no socket option are in
node-http-server-keepalive.node.mts, which usesnode:test. The test file imports it, so they run under Bun, and its last test runs them under Node. 11 one-line mutants of the JavaScript and 2 of the C++ function each fail at least one test. The two calls to the helper from the lazy construction sites have no test: no connection of a listening server reaches them.Self-review. Two passes. The first (28 concerns) took Nagle's algorithm, the type of service and a change to
us_socket_keepaliveout of an earlier draft. The second (18 concerns) replaced the uSockets listener field with the read inonServerConnection. Not addressed:setKeepAlive()through atls.TLSSocketwrap (above). It is anode:tlschange.internal-for-testingfor this test file andsocket.test.ts. It is a follow-up.Review on this PR. Four optional findings, all taken:
resetAndDestroy()through aProxyhalf-destroyed the socket after an earlier revision. That revision is gone: the method is main's again.net.Socket.prototype.setKeepAlive. It writes the two state symbols and calls the handle, asnet.Serverdoes innet.ts./proc/self/fdor/dev/fd, not in the first 1024 numbers.bun:testand static imports.The comment check asked for five two-line comments to go. Each is one line now.
A later finding asked for the unix marker to be a class field with a default. As a field it cost 29 B for each socket (265 B to 294 B shallow), so the marker is gone: see the paragraph on unix sockets.
Not run here. Windows and macOS: CI runs them. The tests that read kernel state skip on Windows, where nothing gives the
SOCKETof a server connection to JavaScript. The other tests of the file run there.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http/node-http-server-keepalive.test.ts