node:net: route exceptions thrown by socket event listeners to uncaughtException - #34066
node:net: route exceptions thrown by socket event listeners to uncaughtException#34066robobun wants to merge 5 commits into
Conversation
|
Updated 7:15 AM PT - Jul 13th, 2026
✅ @robobun, your commit dd852ddaa32426d5fe77a8db3b9af5928dcd7f34 passed in 🧪 To try this PR locally: bunx bun-pr 34066That installs a local version of the PR into your bun-34066 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Checked the three suggestions:
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of either, but there is real overlap worth spelling out:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ughtException An exception thrown synchronously from a user listener invoked inside a native socket callback (e.g. a throwing 'data' listener) escaped into the native dispatch, which routes a throwing handler to the socket's error callback. That behavior is correct for the public Bun.listen/Bun.connect API, but for node:net it delivered the thrown exception to the socket's 'error' listeners as if it were a socket error, so it never reached uncaughtException the way it does in Node. Wrap the node:net handler tables so an exception escaping a handler is rethrown on process.nextTick, reaching the uncaughtException machinery, and the socket is torn down (its dispatch was interrupted mid-callback, matching the teardown the previous error routing performed). Genuine socket errors still flow through the error handler unchanged. Fixes #34064 Fixes #29761
… response uws_res_end_without_body unconditionally wrote "Connection: close" and a header terminator even when the chunked body had already started, so ServerResponse#destroy() on an in-flight chunked response injected "Connection: close\r\n\r\n" into the middle of the body. Clients parse that as an invalid chunk size (HPE_INVALID_CHUNK_SIZE). The bug was previously masked on the client side because the resulting parse error throw was swallowed by the socket error routing fixed in the previous commit. Skip the header writes once HTTP_WRITE_CALLED is set (the header section is terminated and body bytes are on the wire). HTTP_WROTE_CONTENT_LENGTH_HEADER must not gate this: the file-route HEAD path sets it for a plain header line and relies on this function to write the terminating CRLF.
7784c74 to
5f57e0f
Compare
WalkthroughThe changes guard native socket callbacks against escaping exceptions, adjust TLS client-error handling, and prevent HTTP response termination from writing headers after body output begins. Regression tests cover socket exception propagation, TLS events, and chunked response termination. ChangesSocket exception handling and TLS behavior
HTTP response termination
🚥 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: 3
🤖 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 `@src/js/node/net.ts`:
- Around line 263-270: Condense the explanatory comment above the native socket
callback handling to no more than three lines, retaining only that thrown
user-listener exceptions must surface as uncaughtException and that the socket
is torn down afterward. Remove the detailed API and dispatch rationale from the
comment.
In `@src/uws_sys/libuwsockets.cpp`:
- Around line 1341-1346: Trim the comment near the write/header-state logic in
src/uws_sys/libuwsockets.cpp lines 1341-1346 to no more than three lines while
preserving its essential rationale. In test/js/node/http/node-http.test.ts lines
3753-3755, remove all regression-test commentary except the issue URL.
In `@test/js/node/net/node-net.test.ts`:
- Around line 1067-1174: Update the “exceptions thrown from socket event
listeners” suite to run its independent subprocess tests concurrently, using the
repository’s supported concurrent test or describe API. Preserve each test’s
existing fixture setup, assertions, and subprocess behavior.
🪄 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: 6aa15294-75cd-4cc3-a564-03666f9f3461
📒 Files selected for processing (4)
src/js/node/net.tssrc/uws_sys/libuwsockets.cpptest/js/node/http/node-http.test.tstest/js/node/net/node-net.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/net.ts (1)
1492-1501: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not swallow
onreadcallback exceptions beforeprotectHandler.
onread.callback()is still converted intoself.emit("error", e), so an attached'error'listener bypassesrethrowUncaught()and socket teardown. Let the exception escape to the wrapper, or invoke the same rethrow/terminate helper here.Proposed fix
- try { - onread.callback(buffer.length, buffer); - } catch (e) { - self.emit("error", e); - } + onread.callback(buffer.length, buffer);🤖 Prompt for 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. In `@src/js/node/net.ts` around lines 1492 - 1501, Update the data handler’s onread.callback invocation inside protectHandler so callback exceptions are not converted to self.emit("error", e); let them propagate to protectHandler or invoke the established rethrow/termination helper, preserving socket teardown and uncaught-error behavior.
🤖 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.
Outside diff comments:
In `@src/js/node/net.ts`:
- Around line 1492-1501: Update the data handler’s onread.callback invocation
inside protectHandler so callback exceptions are not converted to
self.emit("error", e); let them propagate to protectHandler or invoke the
established rethrow/termination helper, preserving socket teardown and
uncaught-error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc97c24b-8bf9-432b-b22d-f301b55f881b
📒 Files selected for processing (4)
src/js/node/net.tssrc/uws_sys/libuwsockets.cpptest/js/node/http/node-http.test.tstest/js/node/net/node-net.test.ts
The onread data handler converted callback throws into a socket 'error' emit, bypassing the uncaughtException rerouting. Node invokes the onread callback from onStreamRead without catching, so a throw is fatal.
|
Addressed the outside-diff finding in 4bffee9: the onread data handler no longer converts callback throws into a socket 'error' emit, so they reach uncaughtException through the wrapper like every other listener (Node's onStreamRead does not catch them either). Added a subprocess test for the onread shape; it fails on an unfixed build (the error listener receives the throw) and passes with this branch. |
There was a problem hiding this comment.
No issues found on re-review — all three earlier inline nits and the CodeRabbit style comments are addressed in the latest commits. Deferring to a human because this wraps every node:net handler-table entry (every TCP socket's dispatch path), changes teardown semantics on listener throw, touches uws_res_end_without_body in the shared uWS layer, and overlaps with #29762 / #32036.
Checked this pass:
protectHandlerarity (socket, a, b) covers every handler in the three tables — no entry receives >3 args from native dispatch.- Wrapping the
errorhandler itself: a throwing user'error'listener now reachesuncaughtExceptioninstead of re-entering the error path — matches Node. - The
bodyStartedguard still setsHTTP_CONNECTION_CLOSE/HTTP_END_CALLEDstate flags, somarkDoneand connection-close accounting are unchanged; only the wire bytes are suppressed.
Extended reasoning...
Overview
Four files: src/js/node/net.ts adds protectHandler/protectHandlers and wraps SocketHandlers, ServerHandlers, SocketHandlers2, and the per-socket onread data handler so exceptions escaping a handler are rethrown on process.nextTick (reaching uncaughtException) and the native socket is terminated. src/uws_sys/libuwsockets.cpp gates the Connection: close header write and terminating CRLF in uws_res_end_without_body on !HTTP_WRITE_CALLED for both SSL and non-SSL branches. Two test files add five subprocess tests for the exception routing and one raw-TCP test for the mid-stream destroy wire bytes.
Security risks
None identified. No auth/crypto/permissions surface. The change routes user-thrown exceptions to a more visible channel (previously silently swallowed with an error listener present), and the uWS change strictly removes bytes that were being incorrectly injected into an already-started body.
Level of scrutiny
High. node:net handler tables are the dispatch boundary for every TCP socket in the Node compat layer — node:http, node:tls, pg, redis clients, etc. all sit on top. Wrapping every handler (including close, error, drain) in a try/catch that terminates the socket on throw is a real semantic change, and the "terminate on any escaping exception" decision — while justified in the description as matching the prior error-routing teardown — is a design call a maintainer should sign off on. uws_res_end_without_body is shared native code also used by Bun.serve, so the guard affects the native server abort path beyond node:http.
Other factors
- All three of my earlier inline findings are resolved in the current diff (
socket?.terminate?.()for theserverNamefirst-arg shape; the http test now destroys once the client has received the chunk frame; theonreadinner try/catch is removed with a covering test). - The PR explicitly overlaps with two open PRs: #29762 (lifecycle-listener subset of the same wrapping) and #32036 (near-identical
uws_res_end_without_bodyguard plus an additionalinternalEndfix). A human should decide sequencing/reconciliation. - Test coverage is good: five subprocess fixtures each exit with a distinct code depending on which path the exception took, so misrouting fails loudly; the http test asserts exact wire bytes.
- CI build #72437 was still building at last timeline update; the PR description notes the relevant test directories pass locally with remaining failures also present on main.
Given the breadth of the code path touched, the design decision on socket teardown, and the overlap with two other open PRs, this is not a candidate for auto-approval.
…ection ServerHandlers.handshake reported the client-cert verification error through 'tlsClientError' even when rejectUnauthorized was false and the connection proceeded. Node only reports tlsClientError when the connection is torn down; test-tls-sni-option.js asserts this with mustNotCall, and the assertion was previously swallowed by the error routing fixed earlier in this branch.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/node/net.ts (2)
266-275: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTerminate
socketHandleon SNI callback failure.protectHandler()callsterminate()on its first argument, butserverName(server, servername, socketHandle)receives the owningtls.Serverthere and the accepted connection handle as the third argument. If the callback throws, the handshake stays alive; terminatesocketHandleinstead.🤖 Prompt for 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. In `@src/js/node/net.ts` around lines 266 - 275, Update protectHandler to terminate the actual connection handle on callback failure. Preserve the existing behavior for callbacks whose first argument is the socket, but when invoked for serverName with the tls.Server as the first argument, use the third argument socketHandle for the optional terminate call so the failed handshake is closed.
271-271: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCapture
nextTickonce.process.nextTickis still called directly here, so userland can monkey-patch it and block the uncaught-exception rethrow. Use a module-captured primordial-safe alias throughoutsrc/js/node/net.ts.🤖 Prompt for 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. In `@src/js/node/net.ts` at line 271, Update the uncaught-exception rethrow path around rethrowUncaught to call the module-captured primordial-safe nextTick alias instead of process.nextTick. Replace direct process.nextTick usage throughout net.ts with that captured alias, preserving the existing scheduling and error propagation behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/js/node/net.ts`:
- Around line 266-275: Update protectHandler to terminate the actual connection
handle on callback failure. Preserve the existing behavior for callbacks whose
first argument is the socket, but when invoked for serverName with the
tls.Server as the first argument, use the third argument socketHandle for the
optional terminate call so the failed handshake is closed.
- Line 271: Update the uncaught-exception rethrow path around rethrowUncaught to
call the module-captured primordial-safe nextTick alias instead of
process.nextTick. Replace direct process.nextTick usage throughout net.ts with
that captured alias, preserving the existing scheduling and error propagation
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f5544a0-a0c9-4a58-81d1-8cf03c0f69ee
📒 Files selected for processing (3)
src/js/node/net.tstest/js/node/net/node-net.test.tstest/js/node/tls/node-tls-cert.test.ts
|
On the two outside-diff findings:
|
There was a problem hiding this comment.
No issues found; all earlier inline feedback has been addressed. Deferring to a human because this changes core Node-compat dispatch semantics across every node:net handler table, alters native uws_res_end_without_body wire behavior, and (in dd852dd) shifts when tlsClientError fires — plus it overlaps with #29762 and #32036.
What was reviewed:
protectHandler/protectHandlerswrapping ofSocketHandlers/ServerHandlers/SocketHandlers2and the onread path — genuine socket errors still flow through theerrorhandler unchanged;socket?.terminate?.()is tolerant of theserverNamefirst-arg shape.uws_res_end_without_bodybodyStartedguard on both SSL/non-SSL branches —HTTP_CONNECTION_CLOSEstate bit is still set even when the header write is skipped; the file-route HEAD path (WROTE_CONTENT_LENGTH without WRITE_CALLED) still gets its CRLF.- The
tlsClientErroremit relocation — checked that admitted-but-unauthorized connections now match Node (no event), and the rejected path still emits before destroy. - New subprocess tests — each awaits an observable condition and drains pipes concurrently; the http chunked-destroy test triggers destroy on receipt of the chunk frame rather than a timer.
Extended reasoning...
Overview
This PR spans four concerns: (1) wrapping all three node:net handler tables (SocketHandlers, ServerHandlers, SocketHandlers2) plus the onread data handler in a protectHandler guard that reroutes escaping exceptions to process.nextTick(throw) and then calls socket?.terminate?.(); (2) gating uws_res_end_without_body's Connection: close header and terminating CRLF on !HTTP_WRITE_CALLED so aborting a started chunked response no longer injects header bytes into the body; (3) moving the server-side tlsClientError emit inside the _rejectUnauthorized branch so an unauthorized-but-admitted TLS connection no longer fires it; (4) five new subprocess tests in node-net.test.ts, one raw-TCP test in node-http.test.ts, and a tlsClientError assertion in node-tls-cert.test.ts.
Security risks
None identified. The changes narrow what is written to the wire (fewer bytes on abort) and change where a user-thrown exception surfaces (uncaughtException vs. socket 'error'). No new input parsing, no auth/permission logic. The TLS change only affects which event fires for an already-admitted connection; the reject path is unchanged.
Level of scrutiny
High. src/js/node/net.ts is the Node-compat socket layer that every net/tls/http connection runs through, and protectHandlers wraps every callback in three tables — a broad blast radius where the design choice (catch → nextTick rethrow → terminate()) deserves maintainer sign-off. uws_res_end_without_body is on the response-termination path for Bun.serve and node:http; the guard is small but changes wire bytes. The tlsClientError relocation is a user-observable behavioral change that arrived in the last commit and is only lightly covered in the PR description.
Other factors
- All prior review feedback (mine and CodeRabbit's) is addressed and resolved; comments are within the 3-line limit.
- The PR explicitly overlaps with open PRs #29762 (lifecycle-listener subset of the same fix) and #32036 (nearly identical
uws_res_end_without_bodyguard) — a human should decide sequencing. - Test coverage is good: each new behavior has a subprocess fixture that would fail on the unfixed build, and the http test asserts exact wire bytes.
- I did not find bugs in the current revision, but the combination of broad handler-table wrapping, native wire-format change, and a ride-along TLS event semantics change is well outside "simple/mechanical".
1 participant
Fixes #34064
Fixes #29761
Problem
An exception thrown synchronously from a user listener invoked inside a native socket callback (e.g. a throwing
'data'listener on anet.Socket) escaped into the native dispatch, which routes a throwing handler to the socket'serrorcallback. That is the documented behavior for the publicBun.listen/Bun.connectAPI, but node:net is layered on those handlers, so the thrown exception was delivered to the socket's'error'listeners as if it were a socket error. With an'error'listener registered (aspgalways does), the exception silently disappeared instead of reachinguncaughtExceptionlike in Node.Fix
Wrap the node:net handler tables (
SocketHandlers,ServerHandlers,SocketHandlers2) so an exception escaping a handler is rethrown onprocess.nextTick, reaching the uncaughtException machinery, and the socket is torn down (the callback was interrupted mid-dispatch, so its stream state is unreliable; this matches the teardown the previous error routing performed). Genuine socket errors (ECONNRESET, ECONNREFUSED, ...) still flow through theerrorhandler tosocket.emit("error")unchanged. The publicBun.listen/Bun.connectAPI is not affected.Surfacing these exceptions unmasked a second bug, fixed in the second commit:
uws_res_end_without_bodyunconditionally wroteConnection: closeplus a header terminator even when the chunked body had already started, soServerResponse#destroy()on an in-flight chunked response injectedConnection: close\r\n\r\ninto the middle of the body. Clients parse that asParse Error: Invalid character in chunk size(HPE_INVALID_CHUNK_SIZE); previously that parse-error throw was itself swallowed by the routing bug above. Node sends nothing extra on destroy, and now Bun's wire bytes match Node's.Related: #29762 fixed the lifecycle-listener cases (
'connect'/'ready'/'connection') of the same class and deferred the'data'path; the handler-table wrapping here covers the dispatch boundary those paths also run through.Verification
New tests in
test/js/node/net/node-net.test.ts(each fails on an unfixed build):'data'listener with a socket'error'listener reachesuncaughtException; the'error'listener is not invokeduncaughtExceptionhandler the process exits 1 with the thrown errorpgshape from the issue: re-emitting'error'on a listenerlessEventEmitterfrom a'data'listener reachesuncaughtExceptionNew test in
test/js/node/http/node-http.test.ts: destroying a chunked response mid-stream sends exactly the written chunk frame, no header bytes in the body.test/js/node/net/,test/js/node/tls/,test/js/node/http/,test/js/bun/http/serve.test.tsand the abort/destroy/close/error/keepalive subset oftest/js/node/test/parallel/test-http-*pass with the change (remaining failures also fail on main in the same environment: broken localhost/DNS resolution, andtest-http-agent-keepalive.jswhich fails on an unmodified debug build).no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/net/node-net.test.ts