fix: let clientError handler send response before closing socket#28642
fix: let clientError handler send response before closing socket#28642
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughC++ and JS HTTP client-error flows were changed: the native path now invokes a user Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_server.ts`:
- Around line 810-815: The clientError handler currently sends a hardcoded "400
Bad Request" and uses bytesWritten to decide whether to write then destroy the
socket; update this to send "431 Request Header Fields Too Large" when the error
code is "HPE_HEADER_OVERFLOW" and use the socket's _httpMessage?.headersSent
flag instead of nodeSocket.bytesWritten to detect if a response has already
started; also replace the write()+destroy() pattern with end() so the response
is flushed before closing (then destroy(err) only if needed). Locate the logic
around emit("clientError", err, nodeSocket) and modify the branch that writes
the response to implement the 431 status for HPE_HEADER_OVERFLOW, check
_httpMessage?.headersSent, and call end() for graceful close.
- Around line 807-810: The error path currently always creates a new
NodeHTTPServerSocket and re-emits "connection", causing duplicate connection
events and state loss; change it to reuse the wrapper stored on handle.duplex
when present instead of unconditionally allocating a new NodeHTTPServerSocket:
look up handle.duplex before constructing NodeHTTPServerSocket, set nodeSocket
to that existing wrapper if found, only call self.emit("connection", nodeSocket)
when isSocketNew (and !reachedRequestsLimit) like the normal request path, and
then continue to call nodeSocket[kEnableStreaming](true) and
self.emit("clientError", err, nodeSocket) so the same wrapper and its
listeners/state are preserved.
In `@test/regression/issue/28641.test.ts`:
- Around line 32-41: The embedded client fixture only closes the server on the
'end' and 'error' events; add a single guarded completion helper (e.g., function
finish(reason) { if (finished) return; finished = true;
process.stdout.write(...); server.close(); }) and call it from the existing
client.on('end', ...), client.on('error', ...) handlers and also from
client.on('close', ...) so socket teardown cannot leave the subprocess hanging;
reference the client.on('end'), client.on('error'), client.on('close'), the
`data` accumulator and `server.close()`/process.stdout.write calls when making
the change and apply the same pattern to the other embedded script mentioned.
🪄 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: 804fba1b-787d-471e-a714-f3223fa9b075
📒 Files selected for processing (3)
packages/bun-uws/src/HttpContext.hsrc/js/node/_http_server.tstest/regression/issue/28641.test.ts
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed.
Extended reasoning...
Overview
The PR fixes a real Node.js compatibility gap: when clientError fires for oversized headers, Bun was immediately writing a default response and destroying the socket before user handlers could respond. The fix spans HttpContext.h (defer socket lifecycle to the JS handler, uncork after) and _http_server.ts (enable streaming on the error socket, reuse existing wrapper via handle.duplex, emit 431 for HPE_HEADER_OVERFLOW, use write()+destroy() to match Node.js semantics).
Security risks
No new attack surface introduced. The error path now correctly closes the socket after sending the response, matching Node.js behavior. The previous nodeSocket.end() call (which could leave half-open connections) was corrected to write()+destroy().
Level of scrutiny
This is a Node.js compatibility fix in HTTP server infrastructure — moderate scrutiny appropriate. The PR went through multiple rounds of review with the author actively addressing each issue found.
Other factors
All bugs found in prior review iterations have been fixed: the write/destroy semantic fix (dc7539d), the dead conditional removal in HttpContext.h (dc7539d), socket wrapper reuse via handle.duplex (c96ceb7), correct 431 status for HPE_HEADER_OVERFLOW (c96ceb7), guarded finish() helper in tests (c96ceb7), and the ASAN pipe deadlock fix via stderr: "ignore" (c0eb5d2). The remaining nit about handle.ondrain not being cleared in _destroy() is noted in the inline comment — it is structurally asymmetric but protected by C++ synchronous-close semantics and matches the existing CONNECT path behavior.
63f0491 to
1167b09
Compare
4701557 to
8e39434
Compare
8e39434 to
6c9cebd
Compare
454f8d5 to
c358f7f
Compare
22beef4 to
bd88539
Compare
When http.createServer receives a request with headers exceeding the size limit, the clientError event fires but uWebSockets immediately writes a default error response, shuts down the socket, and closes it. This prevents user code from sending a custom response (e.g. 431). - HttpContext.h: Skip automatic write/shutdown/close when onClientError handler is registered. Unref and uncork socket so handler-written data flushes and socket doesn't keep the event loop alive. - _http_server.ts: Enable streaming on error-path socket so writes work. Reuse existing socket wrapper on keep-alive connections. Send default 400/431 response only if handler didn't write anything. Use proper 431 for HPE_HEADER_OVERFLOW. Fixes #28641
fc5d69b to
a4bb934
Compare
Fixes #28641
Problem
When
http.createServerreceives a request with headers exceeding the size limit, theclientErrorevent fires but uWebSockets immediately writes a default error response, shuts down the socket, and closes it. This prevents user code from sending a custom response (e.g. 431 Header Too Large).Expected (Node.js): The
clientErrorhandler receives a writable socket, sends a custom response, and closes the socket itself.Actual (Bun): The socket is destroyed before the handler's
socket.end(...)can deliver data.Root Cause
In
packages/bun-uws/src/HttpContext.h, after calling theonClientErrorcallback, the code unconditionally:Additionally, the
NodeHTTPServerSocketcreated in the error path never had streaming enabled, so its_write()method was a no-op (it checkshandle.ondrainbefore writing).Fix
HttpContext.h: When an
onClientErrorhandler is registered, call it and skip the automatic write/shutdown/close. The handler takes responsibility for the socket lifecycle. Uncork the socket so any data written by the handler gets flushed._http_server.ts: Enable streaming on the
NodeHTTPServerSocketcreated in the error path so_write()can send data. When noclientErrorlistener is attached, send a default 400 response matching Node.js behavior.Verification