Enable TCP_DEFER_ACCEPT for HTTP listeners on Linux#28617
Enable TCP_DEFER_ACCEPT for HTTP listeners on Linux#28617Jarred-Sumner wants to merge 2 commits intomainfrom
Conversation
For short-lived HTTP/1.1 connections (Connection: close), the server currently has to return to epoll after accept() just to learn the request is readable. With TCP_DEFER_ACCEPT, the kernel holds the accept until the client has actually sent data, so the request (or ClientHello for TLS) is guaranteed to be in the buffer when accept() returns. The accept loop now dispatches readable immediately for sockets from a deferred listener, skipping one epoll round-trip per connection. This is the same pattern nginx uses (rev->ready = 1 after deferred accept). The option is opt-in via LIBUS_LISTEN_DEFER_ACCEPT and only set by the uWS HTTP listener — raw TCP via Bun.listen/net.createServer is unchanged since those may serve protocols where the server sends first. SO_ACCEPTFILTER "dataready" is set on FreeBSD for the same effect. No-op on macOS and Windows.
|
Updated 2:13 AM PT - Mar 28th, 2026
❌ @robobun, your commit aab3cc9 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 28617That installs a local version of the PR into your bun-28617 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdded a TCP defer-accept implementation (platform-specific setsockopt + listen/accept handling) and wiring to enable it for HTTP listeners; also changed JS stack-trace setup to eagerly set Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 the current code and only fix it if needed.
Inline comments:
In `@packages/bun-usockets/src/bsd.c`:
- Around line 990-1006: The strcpy call in bsd_set_defer_accept uses a fixed
literal for afa.af_name; replace it with a bounded copy to avoid any future
overflow risk (use a safe alternative like strncpy/strlcpy or memcpy) when
populating struct accept_filter_arg afa.af_name, keeping the zeroed memset and
preserving the "dataready" value and sizeof(afa) passed to setsockopt; ensure
the chosen function always NUL-terminates afa.af_name after copy.
🪄 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: cfb2b43b-24d5-417c-a6f2-c221b4602182
📒 Files selected for processing (7)
packages/bun-usockets/src/bsd.cpackages/bun-usockets/src/context.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/internal/networking/bsd.hpackages/bun-usockets/src/libusockets.hpackages/bun-usockets/src/loop.cpackages/bun-uws/src/HttpContext.h
| int bsd_set_defer_accept(LIBUS_SOCKET_DESCRIPTOR listenFd) { | ||
| #if defined(TCP_DEFER_ACCEPT) | ||
| /* nginx uses 1 second here: there's no way to know how long a connection sat in the | ||
| * defer queue (or whether it bypassed it via syncookies), so a short timeout lets the | ||
| * application's own idle timeout take over for clients that connect but never send. */ | ||
| int timeout = 1; | ||
| return setsockopt(listenFd, IPPROTO_TCP, TCP_DEFER_ACCEPT, &timeout, sizeof(timeout)) == 0; | ||
| #elif defined(SO_ACCEPTFILTER) | ||
| struct accept_filter_arg afa; | ||
| memset(&afa, 0, sizeof(afa)); | ||
| strcpy(afa.af_name, "dataready"); | ||
| return setsockopt(listenFd, SOL_SOCKET, SO_ACCEPTFILTER, &afa, sizeof(afa)) == 0; | ||
| #else | ||
| (void) listenFd; | ||
| return 0; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm bsd_set_defer_accept is used in context.c
rg -n 'bsd_set_defer_accept' --type cRepository: oven-sh/bun
Length of output: 377
🏁 Script executed:
rg -n 'struct accept_filter_arg' --type c -A 5Repository: oven-sh/bun
Length of output: 493
🏁 Script executed:
head -50 packages/bun-usockets/src/bsd.c | grep -n "include"Repository: oven-sh/bun
Length of output: 226
🏁 Script executed:
sed -n '990,1010p' packages/bun-usockets/src/bsd.cRepository: oven-sh/bun
Length of output: 1013
🏁 Script executed:
# Check if there are any system header references or struct definitions visible
rg -n "accept_filter_arg" packages/bun-usockets/src/ -B 5 -A 5Repository: oven-sh/bun
Length of output: 1004
🏁 Script executed:
# Look for any BSD-specific includes or defines around SO_ACCEPTFILTER
sed -n '950,1010p' packages/bun-usockets/src/bsd.c | head -70Repository: oven-sh/bun
Length of output: 2202
🌐 Web query:
accept_filter_arg struct af_name buffer size BSD FreeBSD
💡 Result:
The struct accept_filter_arg in FreeBSD (and BSD systems like NetBSD) is defined in sys/sys/socket.h as: struct accept_filter_arg { char af_name[16]; char af_arg[256-16]; }; The af_name field is a fixed-size char array buffer of 16 bytes for the accept filter name. The af_arg field is a char array of 240 bytes (256 - 16 = 240) for filter-specific arguments. The total struct size is 256 bytes. This structure is used with setsockopt(2) and SO_ACCEPTFILTER to attach an accept filter to a listening socket. Examples from accf_http(9) and accf_dns(9) man pages show usage like: struct accept_filter_arg afa; bzero(&afa, sizeof(afa)); strcpy(afa.af_name, "httpready"); // or "dnsready" setsockopt(sok, SOL_SOCKET, SO_ACCEPTFILTER, &afa, sizeof(afa)); The accept filter mechanism originated in FreeBSD 4.0 and is used to preprocess incoming connections in the kernel before passing to accept(2). It remains in current FreeBSD versions (e.g., FreeBSD 15/16 as of 2024 man pages).
Citations:
- 1: https://github.com/freebsd/freebsd-src/blob/main/sys/sys/socket.h
- 2: https://man.freebsd.org/cgi/man.cgi?manpath=FreeBSD+4.10-RELEASE&query=accf_http&sektion=9
- 3: https://man.freebsd.org/cgi/man.cgi?apropos=0&manpath=FreeBSD+14.4-RELEASE+and+Ports&query=accf_dns&sektion=9
- 4: https://man.freebsd.org/cgi/man.cgi?manpath=FreeBSD+14.4-RELEASE+and+Ports&query=accf_http&sektion=9
- 5: https://man.freebsd.org/cgi/man.cgi?query=accept_filter&sektion=9
Implementation is correct; consider using safer string copy for defensive coding.
The function correctly handles:
- Linux with
TCP_DEFER_ACCEPT(1-second timeout matching nginx) - FreeBSD with
SO_ACCEPTFILTER"dataready" - Other platforms return 0 (feature disabled)
The strcpy on line 1000 is safe because afa is zeroed and "dataready" (9 characters + null) fits in af_name (16 bytes). However, for defense-in-depth:
💡 Optional: Use bounded string copy
`#elif` defined(SO_ACCEPTFILTER)
struct accept_filter_arg afa;
memset(&afa, 0, sizeof(afa));
- strcpy(afa.af_name, "dataready");
+ strncpy(afa.af_name, "dataready", sizeof(afa.af_name) - 1);
return setsockopt(listenFd, SOL_SOCKET, SO_ACCEPTFILTER, &afa, sizeof(afa)) == 0;The function is properly called from context.c:406 when LIBUS_LISTEN_DEFER_ACCEPT is set.
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 990-990: The function 'bsd_set_defer_accept' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bun-usockets/src/bsd.c` around lines 990 - 1006, The strcpy call in
bsd_set_defer_accept uses a fixed literal for afa.af_name; replace it with a
bounded copy to avoid any future overflow risk (use a safe alternative like
strncpy/strlcpy or memcpy) when populating struct accept_filter_arg afa.af_name,
keeping the zeroed memset and preserving the "dataready" value and sizeof(afa)
passed to setsockopt; ensure the chosen function always NUL-terminates
afa.af_name after copy.
|
Found 7 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR makes a meaningful behavioral change to how all HTTP connections are accepted in Bun, touching low-level networking code across 7 files — it warrants a human review given the complexity of the recursive dispatch path and the unverified SSL code flow.
Extended reasoning...
Overview
The PR adds TCP_DEFER_ACCEPT (Linux) / SO_ACCEPTFILTER (FreeBSD) to all HTTP listen sockets via a new LIBUS_LISTEN_DEFER_ACCEPT flag. Changes span: a new bsd_set_defer_accept() helper in bsd.c, a deferred_accept field added to us_listen_socket_t, flag propagation in context.c, and an immediate recursive call to us_internal_dispatch_ready_poll() inside the accept loop in loop.c. HttpContext::listen unconditionally OR-s the new flag, affecting every Bun.serve() HTTP/HTTPS server.
Security risks
No direct security risks. TCP_DEFER_ACCEPT with a 1-second timeout (matching nginx) is a well-understood kernel feature. The timeout prevents SYN-flood half-open connections from occupying queue slots indefinitely. No auth, crypto, or permission surface is touched.
Level of scrutiny
High scrutiny is warranted. This is not a config or comment change — it changes the accept-loop control flow for every HTTP server in Bun. Key concerns: (1) The recursive us_internal_dispatch_ready_poll() call from within the POLL_TYPE_SEMI_SOCKET branch adds a new re-entrancy pattern that needs careful review. (2) The SSL code path (us_internal_ssl_socket_context_listen) is not shown in the diff; it is unclear whether the deferred_accept flag is correctly propagated to the underlying listen socket for TLS servers. (3) The interaction with the low-priority SSL handshake queue and socket adoption (flags.adopted / s->prev) after the deferred dispatch adds complexity. (4) The benchmark (oha --disable-keepalive) is explicitly listed as not yet done in the test plan.
Other factors
No bugs were found by the automated system, and the 189/189 serve.test.ts pass rate is reassuring. The implementation mirrors nginx's approach (rev->ready = 1) and the comments are thorough. However, given the scope (all HTTP servers on Linux/FreeBSD), the unfinished benchmark, and the un-audited SSL path, a human reviewer should sign off before this ships.
Error.captureStackTrace called setStackFrames() after materializeErrorInfoIfNeeded(), leaving m_errorInfoMaterialized=true with a non-null m_stackTrace. When GC's finalizeUnconditionally found unmarked frames it called computeErrorInfo() which asserts !m_errorInfoMaterialized, causing SIGILL under ASAN. When error info is already materialized, eagerly compute and set the .stack property instead of storing new frames via setStackFrames().
There was a problem hiding this comment.
This PR bundles two unrelated changes — a TCP_DEFER_ACCEPT networking optimization and a JSC GC-assertion fix — both of which touch critical, production-path code. A human reviewer should validate the recursive us_internal_dispatch_ready_poll call in the accept loop and the hasMaterializedErrorInfo branch in errorConstructorFuncCaptureStackTrace before merging.
Extended reasoning...
Overview
The PR modifies eight files across two independent concerns: (1) enabling TCP_DEFER_ACCEPT / SO_ACCEPTFILTER on HTTP listen sockets to skip an unnecessary epoll round-trip on accept, and (2) fixing a JSC GC assertion that fires when Error.captureStackTrace is called on an already-materialized ErrorInstance.
Security risks
No apparent security risks. TCP_DEFER_ACCEPT is opt-in and scoped exclusively to HttpContext::listen; Bun.listen() and net.createServer() are not affected. The flag is gated by successful setsockopt() return, so a kernel that does not support it degrades gracefully. The JSC change is internal error bookkeeping with no trust-boundary implications.
Level of scrutiny
Both changes warrant careful human review despite no bugs being found:
Networking (loop.c): After on_open fires, the code now makes a recursive call to us_internal_dispatch_ready_poll with LIBUS_SOCKET_READABLE. This re-enters the same dispatch function under a different poll type (POLL_TYPE_SOCKET vs POLL_TYPE_SEMI_SOCKET), which is architecturally unusual. The comment acknowledges EWOULDBLOCK as a rare-but-possible outcome (defer timeout). Under high connection rates, the stack depth of this recursion needs human validation.
JSC (FormatStackTraceForJS.cpp): The fix introduces a new branch for hasMaterializedErrorInfo() that eagerly computes and sets the stack property. This path bypasses the lazy-getter mechanism that the rest of the error-stack machinery relies on. While the logic appears correct for preventing the GC assertion, JSC internals around ErrorInstance lifecycle are subtle enough to warrant human sign-off.
Other factors
The benchmark item in the test plan is unchecked ('[ ] Benchmark oha --disable-keepalive on Linux'). The two changes are conceptually unrelated and would be easier to review independently. CodeRabbit's only comment was a trivial strcpy nitpick (safe as written). No prior human or bot reviews exist.
2 participants
What
Sets
TCP_DEFER_ACCEPTon the listen socket forBun.serve()(Linux) andSO_ACCEPTFILTER "dataready"(FreeBSD). When the kernel defers accept, the accept loop dispatches the socket as readable immediately instead of returning to epoll first.Why
For short-lived connections (HTTP/1.1 with
Connection: close), the server currently does:accept()→ register new socket with epollrecv()→ process → respond → closeWith
TCP_DEFER_ACCEPT, the kernel holds the accept until data arrives, so step 3–4 collapses into step 2. Same pattern nginx uses (rev->ready = 1after deferred accept).Works for TLS too — the ClientHello is the first data packet.
Scope
LIBUS_LISTEN_DEFER_ACCEPT, only set by uWSHttpContext::listenBun.listen()/net.createServer()unchanged (may serve server-sends-first protocols)Test plan
test/js/bun/http/serve.test.tspasses (189/189)zig:check-allpasses on all platformsoha --disable-keepaliveon Linux to quantify improvement