fix: close connection after async chunked response with Connection: close#28390
fix: close connection after async chunked response with Connection: close#28390bilby91 wants to merge 4 commits into
Conversation
…lose Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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)
WalkthroughPerform immediate connection-close handling after uncorking in both chunked and full-body response end paths; add an integration test verifying an HTTP/1.1 server sending delayed chunked responses closes the TCP connection when a client sends Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/http/node-http-async-chunked-close.test.ts`:
- Around line 44-116: The test "proxy with createConnection closes socket after
chunked upstream response" currently calls backend.close() and proxy.close()
after the assertions, which can leak servers if an assertion or await throws;
wrap the execution that starts/listens and the socket request/response logic in
a try/finally and move backend.close() and proxy.close() into the finally block
(referencing the backend and proxy server variables) so both servers are always
closed even on failure.
- Around line 5-42: The test "http.Server closes connection after async chunked
response with Connection: close" may leak the server if an error/timeout occurs
before server.close(); wrap the server lifecycle in a try/finally: call
server.listen(...) and run the client logic inside the try block, and ensure
server.close() is invoked in the finally block (referencing the local variable
server and the test body) so the server is always closed even on exceptions or
timeouts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6476bf28-395a-4949-8543-b691077a9e42
📒 Files selected for processing (2)
packages/bun-uws/src/HttpResponse.htest/js/node/http/node-http-async-chunked-close.test.ts
Address CodeRabbit review: wrap server cleanup in try/finally for exception safety. Remove proxy/createConnection test that depends on changes from another branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@test/js/node/http/node-http-async-chunked-close.test.ts`:
- Around line 22-34: The test's socket created by net.createConnection can emit
'error' and currently has no handler, causing hangs; add an error listener on
the socket returned by net.createConnection(...) that resolves the surrounding
promise immediately with pass: false and a body containing the error message
(and any collected chunks), then destroy the socket; ensure this handler lives
alongside the existing socket.on("data"), socket.on("end"), and
socket.setTimeout(...) handlers so connection failures fail fast and clean up
resources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e1f8a9d-1608-4f2b-9f41-9e33c5f8b2ce
📒 Files selected for processing (1)
test/js/node/http/node-http-async-chunked-close.test.ts
…r-free The non-chunked async uncork path called shutdown() + close() but did not return immediately afterward, allowing execution to fall through to `return success` where `this` is a dangling pointer. Add `return true` after close() to match the chunked path pattern. Found during review of oven-sh#28397 (robobun). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
http.Servernever closes the TCP connection after an async chunked response whenConnection: closeis set0\r\n\r\nterminator) but the socket FIN is never sentRoot cause
In uWebSockets'
HttpResponse::internalEnd(), whenwriteHead()is called outside the initial HTTP request handler (async), it corks the socket. Whenend()later runs:markDone()clearsHTTP_RESPONSE_PENDINGisCorked()returnstrue→ takes theelsebranch → callsuncork()to flush dataHTTP_CONNECTION_CLOSE— never callsshutdown()+close()In the sync case,
HttpContext'sonDatahandler performs the close check after uncorking. But in the async case, that handler already returned before the response was sent.Fix
Added a connection-close check after
uncork()in both the chunked and non-chunked paths ofinternalEnd(). After uncorking flushes the data, we checkHTTP_CONNECTION_CLOSE,!HTTP_RESPONSE_PENDING, andgetBufferedAmount() == 0— if all conditions are met, we callshutdown()+close().Test plan
Connection: close(setTimeout pattern)createConnectionpiping chunked upstream responseUSE_SYSTEM_BUN=1, pass withbun bd testserve.test.ts(190 pass),7471.test.ts(13 pass), transfer-encoding tests, backpressure tests🤖 Generated with Claude Code